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

Status: RESOLVED FIXED

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

Description

[dstaudigel]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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)?

Comment 2

[dstaudigel]

Attachment: 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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 5

[dstaudigel]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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...

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

(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.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Use a shim object

Blake gently provided me a patch for it. 
So I packaged it up with a unit test!

Comment 10

[Boris Zbarsky [:bzbarsky]]

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?

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: 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?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Sharing that is not a bad idea, yeah...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Perhaps just move it to a header in network/base/public?

Comment 14

[Alexandre Poirot [:ochameau]]

Attachment: Use a shared shim object

Here is a shared implementation of such shim object.

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: 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]

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: Updated patch against current master

Carrying forward review.

Comment 18

[Alexandre Poirot [:ochameau]]

Attachment: Forgot to add new files.

Comment 19

[Alexandre Poirot [:ochameau]]

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

Comment 20

[Mozilla RelEng Bot]

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

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 622831 [details] [diff] [review]
Forgot to add new files.

https://hg.mozilla.org/integration/mozilla-inbound/rev/880e0d0a902d

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/880e0d0a902d