The default bug view has changed. See this FAQ.

Certain kinds of XMLHttpRequests fail to operate with JS implementations of nsIChannel

RESOLVED FIXED in mozilla15

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: dstaudigel, Assigned: ochameau)

Tracking

6 Branch
mozilla15
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_1) AppleWebKit/534.48.3 (KHTML, like Gecko) Version/5.1 Safari/534.48.3

Steps to reproduce:

We have written a wrapper for nsIHttpChannel-s in JS - that hides information about the underlying channel from the external environment.


Actual results:

When we pass this JS wrapper to an XHR object for it to use, the call silently fails on XHR.send() (NS_FAILURE).  Poking around in XMLHttpRequest.cpp, the failure happens at AsyncOpen(listener,nsnull) in Start() around line 2470 for me (6.0 base src).  When I replace listener with nsnull, the call goes through the the JS code.  I believe that XPConnect is barfing (and returning NS_FAILURE) when trying to wrap the listener object to pass a proxy to the JS side of things, because everything works as expected (or as well as can be expected when passing null as the listener) when no listener is passed.


Expected results:

The call should have gone through like normal.
That's quite odd.  Nothing in there should be a problem for XPConnect, at a glance...

Could you please attach the extension you're using (or at least enough of it to reproduce the problem)?
(Reporter)

Comment 2

6 years ago
Created attachment 556059 [details]
The plugin that demonstrates the bug.

Show the toolbar and attempt to login, any username and password will cause the bug to appear.  With dump() enabled, the following is printed to the console:
VWCUtils.jsm 13:00:24 -- State - undefined
VWCUtils.jsm 13:00:24 -- content: [xpconnect wrapped (nsISupports, nsIMultiplexInputStream, nsIInputStream, nsISeekableStream)]
VWCUtils.jsm 13:00:24 -- Warning: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE)"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://vwc_modules/VWCUtils.jsm :: <TOP_LEVEL> :: line 170"  data: no] stack undefined
VWCUtils.jsm 13:00:24 -- Status: 
Some exception [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE)"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://vwc_modules/VWCUtils.jsm :: <TOP_LEVEL> :: line 170"  data: no] : undefined

The NS_ERROR_FAILURE is coming out of XHR.cpp in the way described.
So what's happening here is that the listener is nsXMLHttpRequest itself, and it has the "classinfo interfaces only" flag set, and does not expose nsIStreamListener in classinfo, so XPConnect refuses to create a wrapper for that interface.

See similar bug 317448.

I suppose we could try ignoring the classinfo interfaces flag for system code, or we could use some sort of shim object which is not restricted to classinfo interfaces as the listener...  Jonas, thoughts?
Status: UNCONFIRMED → NEW
Component: XPConnect → DOM
Ever confirmed: true
QA Contact: xpconnect → general
The problem with letting chrome QI to non-classinfo interfaces is that once the QI has executed, the properties remain on the JS object, no? Don't know if this could be a security issue, but it seems like it could be a web compat issue if chrome code ever touches xhr's handed to content.
(Reporter)

Comment 5

6 years ago
The interesting thing is that most XHRs work in user-land, this is primarily a bug with XHRs sent from chrome code (which is the only case in which the original XHR is passed as a listener to AsyncOpen).  We have seen a few web glitches that we believe come from the same bug, but we haven't confirmed it.  The only way that would make sense is if those web XHRs are multipart's and the multipart listener in XHR.cpp also has the same problem of not wanting to QI to nsIStreamListener.
The multipart proxy listener doesn't have classinfo, so should not be causing a problem here.

Depending on your http implementation, it's also possible that there's just some corner case you're not handling somewhere...

Updated

5 years ago
Duplicate of this bug: 736046
(In reply to Jonas Sicking (:sicking) from comment #4)
> The problem with letting chrome QI to non-classinfo interfaces is that once
> the QI has executed, the properties remain on the JS object, no? Don't know
> if this could be a security issue, but it seems like it could be a web
> compat issue if chrome code ever touches xhr's handed to content.

Yeah, I had the same reaction to this as well. I think Boris's first suggestion of a shim object that we expose to JS is the way to go here. It should even be a pretty easy patch to write.
(Assignee)

Comment 9

5 years ago
Created attachment 606914 [details] [diff] [review]
Use a shim object

Blake gently provided me a patch for it. 
So I packaged it up with a unit test!
Using NS_FORWARD_NSISTREAMLISTENER(mListener->) instead of NS_DECL_NSISTREAMLISTENER would let you drop the manual forwarding you have now...

Past that, is this ready for review in general?  Want to take the bug?
(Assignee)

Comment 11

5 years ago
Created attachment 607117 [details] [diff] [review]
Use a shim object

Updated patch in order to use magic macro suggested by bz.
In mozilla, there is always a macro for that!!

While searching more info about these kind of macros, I've found exactly same trick here:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#1434
So that I'm wondering if we should use same class for both components, or if it's just fine to duplicate such shim class?
Attachment #606914 - Attachment is obsolete: true
Attachment #607117 - Flags: review?(jonas)
Sharing that is not a bad idea, yeah...
Perhaps just move it to a header in network/base/public?
(Assignee)

Comment 14

5 years ago
Created attachment 608019 [details] [diff] [review]
Use a shared shim object

Here is a shared implementation of such shim object.
Assignee: nobody → poirot.alex
Attachment #607117 - Attachment is obsolete: true
Attachment #607117 - Flags: review?(jonas)
Attachment #608019 - Flags: review?(jonas)
(Assignee)

Comment 15

5 years ago
Created attachment 608089 [details] [diff] [review]
Use a shared shim object

Ok I think that's the last change. 
I learned how inline works and it allowed me to simplify even more the .cpp in order to contain only NS_IMPL_ISUPPORTS2()!

[sorry for bugmail and all these r? requests]
Attachment #608019 - Attachment is obsolete: true
Attachment #608019 - Flags: review?(jonas)
Attachment #608089 - Flags: review?(jonas)
Comment on attachment 608089 [details] [diff] [review]
Use a shared shim object

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

Hmm.. not terribly happy with this since it adds complexity and (a little bit of) slowness to the common codepath, even though it's not generally needed. But I don't have any better ideas.
Attachment #608089 - Flags: review?(jonas) → review+
(Assignee)

Comment 17

5 years ago
Created attachment 622820 [details] [diff] [review]
Updated patch against current master

Carrying forward review.
Attachment #608089 - Attachment is obsolete: true
Attachment #622820 - Flags: review+
(Assignee)

Comment 18

5 years ago
Created attachment 622831 [details] [diff] [review]
Forgot to add new files.
Attachment #622820 - Attachment is obsolete: true
Attachment #622831 - Flags: review+
(Assignee)

Updated

5 years ago
Whiteboard: [autoland:622831]
(Assignee)

Updated

5 years ago
Whiteboard: [autoland:622831] → [autoland-try:622831]

Updated

5 years ago
Whiteboard: [autoland-try:622831] → [autoland-in-queue]
(Assignee)

Comment 19

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=6d1a3db98cc9
Some oranges, but all of them seems to be intermittent failures.

Comment 20

5 years ago
Autoland Patchset:
	Patches: 622831
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=2d27a9611dfc
Try run started, revision 2d27a9611dfc. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=2d27a9611dfc
(Assignee)

Updated

5 years ago
Whiteboard: [autoland-in-queue] → checkin-needed
Comment on attachment 622831 [details] [diff] [review]
Forgot to add new files.

https://hg.mozilla.org/integration/mozilla-inbound/rev/880e0d0a902d
Attachment #622831 - Flags: checkin+

Updated

5 years ago
Whiteboard: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/880e0d0a902d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.