392338 - [FIX]Request created via document.load causes Component does not have requested interface arg 0 [nsIHttpChannel.notificationCallbacks]

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Colin Blake]

This is somewhat similar to bug 317448. The difference is that instead of creating the request via
XMLHTTPRequest it is created via document.implementation.createDocument. Any, a request for an XML
document is created but when we try to access channel.notificationCallbacks (for example in 
http-on-modify-request observer) we get:

Component does not have requested interface arg 0 [nsIHttpChannel.notificationCallbacks]

Comment 1

[Boris Zbarsky [:bzbarsky]]

createDocument() doesn't do HTTP requests.  What's this bug actually about?

Comment 2

[Colin Blake]

I was referring to client-side JS which initiates XML requests using this technique:

  var doc = document.implementation.createDocument("", "", null);
  doc.addEventListener("load", myListener, false);
  doc.load(aURL, "application/xml");

Comment 3

[Boris Zbarsky [:bzbarsky]]

Care to attach a testcase that shows the problem, then?  I'm working on a fix, but I need a way to test.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ideally, a chrome mochitest.  That is a file to be run as -chrome that performs the test and follows <http://developer.mozilla.org/en/docs/Mochitest>.  But if it's easier, just a JS snippet to run in the Firefox window (see http://developer.mozilla.org/en/docs/Browser_chrome_tests).

Comment 5

[Colin Blake]

Attachment: Observer/logger code.

Sorry this is not a self contained example, but our builds don't have mochitest. Heck, we don't even pull the
testing directory. The attached file contains some js which I just appended to one of our js components so that
it gets loaded when our product starts up. The js just installs an http-on-modify-request observer which then
attempts to get nsIXMLHttpRequest from the notification callbacks, logging progress to the js error console.

Fire up venkman and create an XMLHTTPRequest and you'll see (on the error console) that we can
access the nsIXMLHttpRequest:

 xmlhttp = Components.classes["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Components.interfaces.nsIXMLHttpRequest);
 xmlhttp.open("GET","http://localhost/xmlnote1.xml",true); // insert valid URL
 xmlhttp.send(null);
 
Create an XML request using createDocument/load and you'll see the exception:

  xmlDoc = document.implementation.createDocument("","",null);
  xmlDoc.load("http://localhost/xmlnote2.xml");

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix plus test

Comment 7

[Johnny Stenback (:jst)]

bz, care to explain this bug and how your patch fixes it? In general, I'm really not keen on flagging more things as noAccess, at least not unless there's no other way...

Comment 8

[Boris Zbarsky [:bzbarsky]]

The bug here is that the XMLDocument is set as the notification callbacks on the channel, and implements nsIInterfaceRequestor, and QIs to it, but does NOT expose it in classinfo.  Which means XPConnect throws when you try to get the callbacks from the channel, because it thinks it can't create the wrapper.

So the fix is the classinfo change.  The other changes are just there for reasons similar to bug 317448.  If we think it's safe to let web script call getInterface in this case, we can not add the noAccess lines.

The other option would be to use a shim interface requestor object here with no classinfo (so that untrusted script couldn't get at it no matter what), and leave nsIInterfaceRequestor out of the XMLDocument classinfo...

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 294926 [details] [diff] [review]
Fix plus test

Ok, let's do this w/o the noAccess changes then. sr=jst with that removed.

Comment 10

[Colin Blake]

FYI: I applied the patch to our code base here and it fixed the problem. Thanks!

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 294926 [details] [diff] [review]
Fix plus test

Looking for approval.  This makes it possible to hook properly into XMLDocument.load loads from extensions.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Checked in without the all.js changes.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

FWIW. I'm planning on killing the test added here. It no longer works since we have no need to add any .notificationCallbacks any more since we can enforce same-origin through other means.

Looking at the patch, the bug was in code that's dead these days anyway?