Note: There are a few cases of duplicates in user autocompletion which are being worked on.

XMLHttpRequest getInterface weirdness

RESOLVED FIXED in Firefox 16

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Simon Kornblith, Assigned: bz)

Tracking

Trunk
mozilla18
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox16 fixed, firefox17 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
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).

Updated

5 years ago
Component: XPCOM → XPConnect
(Assignee)

Comment 2

5 years ago
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?
Status: UNCONFIRMED → NEW
Component: XPConnect → DOM
Ever confirmed: true
(Assignee)

Comment 3

5 years ago
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?
(Assignee)

Comment 5

5 years ago
Created attachment 649973 [details] [diff] [review]
Make people poking XHR via random XPCOM interfaces actually work as long as they stick to GetInterface.
Attachment #649973 - Flags: review?(peterv)
(Assignee)

Comment 6

5 years ago
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)

Updated

5 years ago
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+
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f994df8c6c4
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla18
(Assignee)

Updated

5 years ago
Blocks: 740069
(Assignee)

Comment 9

5 years ago
Created attachment 659258 [details] [diff] [review]
What I actually landed
(Assignee)

Comment 10

5 years ago
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.
Attachment #659258 - Flags: approval-mozilla-beta?
Attachment #659258 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/0f994df8c6c4
Status: NEW → RESOLVED
Last Resolved: 5 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+
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/b0d065b8572a
https://hg.mozilla.org/releases/mozilla-beta/rev/89f97910df28
status-firefox16: --- → fixed
status-firefox17: --- → fixed
Please mark it as [qa-] since there is a test verifying this bug.

content/base/test/chrome/test_bug780529.xul
You need to log in before you can comment on or make changes to this bug.