Closed Bug 780529 Opened 10 years ago Closed 10 years ago

XMLHttpRequest getInterface weirdness


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

Not set



Tracking Status
firefox16 --- fixed
firefox17 --- fixed


(Reporter: simon, Assigned: bzbarsky)




(2 files)

Consider the code:

var req = Components.classes[";1"]
    .createInstance();"GET", "", true);
    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.
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).
Component: XPCOM → XPConnect
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?
Component: XPConnect → DOM
Ever confirmed: true
And incidentally, that might let us remove getInterface from XMLHttpRequest.webidl.
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?
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....
Assignee: nobody → bzbarsky
Whiteboard: [need review]
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.
Attachment #649973 - Flags: review?(peterv) → review+
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla18
Blocks: 740069
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
String or UUID changes made by this patch: None.
Attachment #659258 - Flags: approval-mozilla-beta?
Attachment #659258 - Flags: approval-mozilla-aurora?
Closed: 10 years ago
Resolution: --- → FIXED
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.
Attachment #659258 - Flags: approval-mozilla-beta?
Attachment #659258 - Flags: approval-mozilla-beta+
Attachment #659258 - Flags: approval-mozilla-aurora?
Attachment #659258 - Flags: approval-mozilla-aurora+
Please mark it as [qa-] since there is a test verifying this bug.

Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.