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)?
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?
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.
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...
(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.
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?
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?
Sharing that is not a bad idea, yeah...
Perhaps just move it to a header in network/base/public?
Created attachment 608019 [details] [diff] [review] Use a shared shim object Here is a shared implementation of such shim object.
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]
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.
Created attachment 622820 [details] [diff] [review] Updated patch against current master Carrying forward review.
Created attachment 622831 [details] [diff] [review] Forgot to add new files.
https://tbpl.mozilla.org/?tree=Try&rev=6d1a3db98cc9 Some oranges, but all of them seems to be intermittent failures.
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 on attachment 622831 [details] [diff] [review] Forgot to add new files. https://hg.mozilla.org/integration/mozilla-inbound/rev/880e0d0a902d