Last Comment Bug 780529 - XMLHttpRequest getInterface weirdness
: XMLHttpRequest getInterface weirdness
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla18
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
:
Mentors:
Depends on:
Blocks: 740069
  Show dependency treegraph
 
Reported: 2012-08-05 17:32 PDT by Simon Kornblith
Modified: 2012-11-15 06:15 PST (History)
8 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Make people poking XHR via random XPCOM interfaces actually work as long as they stick to GetInterface. (12.82 KB, patch)
2012-08-07 23:08 PDT, Boris Zbarsky [:bz] (still a bit busy)
peterv: review+
Details | Diff | Splinter Review
What I actually landed (12.25 KB, patch)
2012-09-07 07:59 PDT, Boris Zbarsky [:bz] (still a bit busy)
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Simon Kornblith 2012-08-05 17:32:55 PDT
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 2012-08-05 18:00:51 PDT
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 [:bz] (still a bit busy) 2012-08-05 18:56:18 PDT
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 [:bz] (still a bit busy) 2012-08-05 18:57:31 PDT
And incidentally, that might let us remove getInterface from XMLHttpRequest.webidl.
Comment 4 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-08-05 23:23:10 PDT
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 [:bz] (still a bit busy) 2012-08-07 23:08:56 PDT
Created attachment 649973 [details] [diff] [review]
Make people poking XHR via random XPCOM interfaces actually work as long as they stick to GetInterface.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2012-08-07 23:11:03 PDT
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] 2012-09-07 07:23:28 PDT
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 [:bz] (still a bit busy) 2012-09-07 07:52:50 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f994df8c6c4
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-09-07 07:59:18 PDT
Created attachment 659258 [details] [diff] [review]
What I actually landed
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2012-09-07 08:01:17 PDT
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] 2012-09-07 16:51:44 PDT
https://hg.mozilla.org/mozilla-central/rev/0f994df8c6c4
Comment 12 Lukas Blakk [:lsblakk] use ?needinfo 2012-09-10 15:23:37 PDT
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 14 Mihai Morar, (:MihaiMorar) 2012-11-15 06:15:11 PST
Please mark it as [qa-] since there is a test verifying this bug.

content/base/test/chrome/test_bug780529.xul

Note You need to log in before you can comment on or make changes to this bug.