Last Comment Bug 682305 - Certain kinds of XMLHttpRequests fail to operate with JS implementations of nsIChannel
: Certain kinds of XMLHttpRequests fail to operate with JS implementations of n...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 6 Branch
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Alexandre Poirot [:ochameau]
:
:
Mentors:
: 736046 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-26 09:05 PDT by dstaudigel
Modified: 2012-05-16 03:56 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
The plugin that demonstrates the bug. (491.83 KB, text/plain)
2011-08-26 10:02 PDT, dstaudigel
no flags Details
Use a shim object (8.19 KB, patch)
2012-03-17 16:50 PDT, Alexandre Poirot [:ochameau]
no flags Details | Diff | Splinter Review
Use a shim object (7.41 KB, patch)
2012-03-19 04:47 PDT, Alexandre Poirot [:ochameau]
no flags Details | Diff | Splinter Review
Use a shared shim object (11.04 KB, patch)
2012-03-21 10:44 PDT, Alexandre Poirot [:ochameau]
no flags Details | Diff | Splinter Review
Use a shared shim object (10.92 KB, patch)
2012-03-21 14:15 PDT, Alexandre Poirot [:ochameau]
jonas: review+
Details | Diff | Splinter Review
Updated patch against current master (7.20 KB, patch)
2012-05-10 11:49 PDT, Alexandre Poirot [:ochameau]
poirot.alex: review+
Details | Diff | Splinter Review
Forgot to add new files. (13.73 KB, patch)
2012-05-10 12:01 PDT, Alexandre Poirot [:ochameau]
poirot.alex: review+
mrbkap: checkin+
Details | Diff | Splinter Review

Description dstaudigel 2011-08-26 09:05:09 PDT
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 [:bz] (still a bit busy) 2011-08-26 09:12:31 PDT
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 2011-08-26 10:02:31 PDT
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.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2011-08-26 10:29:43 PDT
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 2011-08-26 19:54:28 PDT
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 2011-08-27 07:40:19 PDT
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 [:bz] (still a bit busy) 2011-08-29 20:50:00 PDT
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 7 Blake Kaplan (:mrbkap) 2012-03-15 07:15:22 PDT
*** Bug 736046 has been marked as a duplicate of this bug. ***
Comment 8 Blake Kaplan (:mrbkap) 2012-03-15 07:21:48 PDT
(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] 2012-03-17 16:50:47 PDT
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!
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2012-03-18 22:34:40 PDT
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] 2012-03-19 04:47:34 PDT
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?
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2012-03-19 06:46:34 PDT
Sharing that is not a bad idea, yeah...
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2012-03-19 06:47:07 PDT
Perhaps just move it to a header in network/base/public?
Comment 14 Alexandre Poirot [:ochameau] 2012-03-21 10:44:23 PDT
Created attachment 608019 [details] [diff] [review]
Use a shared shim object

Here is a shared implementation of such shim object.
Comment 15 Alexandre Poirot [:ochameau] 2012-03-21 14:15:01 PDT
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 16 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-04-01 22:54:23 PDT
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] 2012-05-10 11:49:35 PDT
Created attachment 622820 [details] [diff] [review]
Updated patch against current master

Carrying forward review.
Comment 18 Alexandre Poirot [:ochameau] 2012-05-10 12:01:41 PDT
Created attachment 622831 [details] [diff] [review]
Forgot to add new files.
Comment 19 Alexandre Poirot [:ochameau] 2012-05-11 12:51:44 PDT
https://tbpl.mozilla.org/?tree=Try&rev=6d1a3db98cc9
Some oranges, but all of them seems to be intermittent failures.
Comment 20 Mozilla RelEng Bot 2012-05-11 16:08:16 PDT
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) 2012-05-15 03:08:14 PDT
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] 2012-05-16 03:56:29 PDT
https://hg.mozilla.org/mozilla-central/rev/880e0d0a902d

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