Make people poking XHR via random XPCOM interfaces actually work as long as they stick to GetInterface.
12.82 KB, patch
|Details | Diff | Splinter Review|
12.25 KB, patch
|Details | Diff | Splinter Review|
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.
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).
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?
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?
Created attachment 649973 [details] [diff] [review] Make people poking XHR via random XPCOM interfaces actually work as long as they stick to GetInterface.
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 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 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 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.
Please mark it as [qa-] since there is a test verifying this bug. content/base/test/chrome/test_bug780529.xul