780529 - XMLHttpRequest getInterface weirdness

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Simon Kornblith]

Consider the code:

var req = Components.classes["@mozilla.org/xmlextras/xmlhttprequest;1"]
    .createInstance();
req.open("GET", "http://www.example.com/", true);
req.send(null);
req.channel.notificationCallbacks
    .getInterface(Components.interfaces.nsIChannelEventSink)
    instanceof Components.interfaces.nsIChannelEventSink;

I think the last line should either throw an XPCOM exception (NS_ERROR_NO_INTERFACE) or evaluate to true. Instead, the last line evaluates to false: getInterface() returns an XMLHttpRequest object, and this object fails the instanceof check.

I get this behavior with both Firefox 14 and the current (8/5) Nightly.

Comment 1

[Simon Kornblith]

I looked at this more closely. My understanding is that XMLHttpRequest is no longer reflected into JS as an XPCOM component as of bug 740069, so the instanceof check might be expected to fail. However, it also looks like asyncOnChannelRedirect is not available to JS with the new bindings, which seems problematic for code that expects channel.notificationCallbacks.getInterface(Components.interfaces.nsIChannelEventSink) to yield an object with a asyncOnChannelRedirect method (in this case, HTTPS Everywhere).

Comment 2

[Boris Zbarsky [:bzbarsky]]

We are moving away from XPCOM in general for objects that are exposed to web pages.  That means no weird QI/GetI behavior if we can avoid it.  In particular, the old behavior would make asyncOnChannelRedirect visible to web content in some cases when chrome code touched it, which is ... clearly suboptimal.

We should probably move the nsIChannelEventSink stuff (and in fact all of the notification callbacks bits) off the XHR object onto a separate object altogether.  That will probably also allow us to remove the random workarounds for XHR being used as a channel event sink in the HTTP code.  Of course that will break extensions that expect to QI the notification callbacks to nsIXMLHttpRequest (though we could make GetI to that interface still work, of course).

Peter, Jonas, thoughts?

Comment 3

[Boris Zbarsky [:bzbarsky]]

And incidentally, that might let us remove getInterface from XMLHttpRequest.webidl.

Comment 4

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

Sounds good to me. I'd like to eventually remove XPCOM goop from the XHR object which I know means breaking some extensions in the process.

The one thing that might be worth doing is to try to do the break just once. But I'm not entirely sure what that entails here, maybe removing nsIXMLHttpRequest entirely?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Make people poking XHR via random XPCOM interfaces actually work as long as they stick to GetInterface.

Comment 6

[Boris Zbarsky [:bzbarsky]]

This keeps the XHR as the notification callbacks, because extensions might well be relying on that.  If we wanted to, though, we could also implemnt all the XHR DOM interfaces on this new object and use _it_ as the notification callbacks.  That way extensions to QI the callbacks to XHR would still work.  What would fail is equality compares of the callbacks to an XHR object....

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 649973 [details] [diff] [review]
Make people poking XHR via random XPCOM interfaces actually work as long as they stick to GetInterface.

Review of attachment 649973 [details] [diff] [review]:
-----------------------------------------------------------------

Please make mXPCOMifier a raw pointer and clear if from the nsXMLHttpRequestXPCOMifier destructor and unlink method.

Comment 8

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f994df8c6c4

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: What I actually landed

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 659258 [details] [diff] [review]
What I actually landed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 740069
User impact if declined: Some extensions may not work right
Testing completed (on m-c, etc.): Passes attached test
Risk to taking this patch (and alternatives if risky): Risk is low, we think. 
   Any cases that might get broken due to the two things no longer being
   object-identical are already broken because the methods they expect are not
   there.  The alternative to checking this in is to just let this ride the
   trains.
String or UUID changes made by this patch: None.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0f994df8c6c4

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 659258 [details] [diff] [review]
What I actually landed

it's been on central for a while now with no apparent fallout so let's go ahead and uplift, we still have 3 weeks with the larger beta audience to shake out any potential issues.

Comment 13

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b0d065b8572a
https://hg.mozilla.org/releases/mozilla-beta/rev/89f97910df28

Comment 14

[Mihai Morar, (:MihaiMorar)]

Please mark it as [qa-] since there is a test verifying this bug.

content/base/test/chrome/test_bug780529.xul