471395 - "Illegal Value" exception when accessing XHR request within an extension

Status: RESOLVED FIXED

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

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Test case

I am experiencing an exception when accessing onreadystatechange of a XHR object.
[Exception... "Illegal value"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"

Tested with Firefox 3.1b2 : EXCEPTION
Tested with Firefox 3.0.5 : WORKS

This problem has been originally reported for Firebug, see more details here:
http://code.google.com/p/fbug/issues/detail?id=1307

I am also attaching a test-case extension that can be used to reproduce the problem. See line 25 (xhrTest.js)

var onreadystatechange = xhrRequest.onreadystatechange; // EXCEPTION

Comment 1

[Boris Zbarsky [:bzbarsky]]

So... what exactly are the steps to reproduce?  I install the extension... and then what?

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

1) Install the extension 
2) Run Firefox with -console command line parameter
3) Check the console. The exception is displayed.

Notice that the exception is different with 3.1b3pre:

xhrTest.httpObserver FAILS [Exception... "Component returned failure code: 0x800
04002 (NS_NOINTERFACE) [nsIInterfaceRequestor.getInterface]"  nsresult: "0x80004
002 (NS_NOINTERFACE)"  location: "JS frame :: chrome://xhrtest/content/xhrTest.j
s :: anonymous :: line 24"  data: no]

Honza

Comment 3

[Boris Zbarsky [:bzbarsky]]

So...  I installed the extension in a 2008-11-01 Mac nightly.  Then I started it.  I see no exceptions in the JS console, which is not surprising since I don't expect it's doing any XHRs.

So what are the actual steps to reproduce?  Do I need to load a particular HTML file or something?

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I have been yet testing 3.2a1pre now. I can confirm that the bug is still there.

Right, here are better steps (tested on Windows XP):

1) Install the xhrtest extension 
2) Run Firefox with -console command line parameter
3) Use a page that generates a XHR 
(e.g: http://www.janodvarko.cz/firebug/tests/601/Issue601.htm, just press "Post" button on this page) 
4) Check the console. The exception is there:

xhrTest.httpObserver FAILS [Exception... "Illegal value"  nsresult: "0x80070057
(NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://xhrtest/content/xhrTe
st.js :: anonymous :: line 25"  data: no]

Honza

Comment 5

[Michal Novotny [:michal]]

It can be reproduced on linux too.

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, so with that same 2008-11-01 build. I install the extension.  Run with -console.  This just starts the browser, so I open the Error Console myself.  Then I load http://www.janodvarko.cz/firebug/tests/601/Issue601.htm and click the "POST" button, and nothing happens other than some dates appearing in the page in bold.  I assume this is the correct behavior?  That's also the behavior I get with a 2009-01-16 build.

If you can reproduce this, do you mind hunting down a one-day regression range?  If I could reproduce on trunk I'd also just debug, but whatever I'm doing is clearly not the same as what you're doing...

Comment 7

[Michal Novotny [:michal]]

Honza meant normal console, not error console. The error message is written by dump() and since it isn't displayed in nightly builds I had to change xhrTest.js as follows:

         {
             // Use this if you have Firebug installed.
             //FBTrace.dumpProperties("xhrTest.httpObserver FAILS", exc);
-            dump("---\n");
-            dump("xhrTest.httpObserver FAILS " + exc + "\n");
-            dump("---\n");
+            if (/NS_ERROR_ILLEGAL_VALUE/.test(exc))
+              alert(exc);
         }
     }
 };


First failing revision is http://hg.mozilla.org/mozilla-central/rev/326d60321ec7

There is a difference in xhrRequest between rev 326d60321ec7 and 8c92737298f6 (the last working rev)
8c92737298f6: xhrRequest = [object XMLHttpRequest @ 0xee11ff80 (native @ 0xee18ae00)]
326d60321ec7: xhrRequest = [object XPCNativeWrapper [object XMLHttpRequest @ 0xee9b2500 (native @ 0xee5e2e00)]]


Backtrace of working revision at nsXMLHttpRequest::GetOnreadystatechange is:
#0  nsXMLHttpRequest::GetOnreadystatechange (this=0xee18ae00, aOnreadystatechange=0xffb6bb54)
    at /opt/moz/hg/src/content/base/src/nsXMLHttpRequest.cpp:1240
#1  0x013f797b in NS_InvokeByIndex_P () from ./libxul.so
#2  0x0038929e in XPCWrappedNative::CallMethod (ccx=@0xffb6bda4, mode=XPCWrappedNative::CALL_GETTER)
    at /opt/moz/hg/src/js/src/xpconnect/src/xpcwrappednative.cpp:2405
#3  0x00398f99 in XPCWrappedNative::GetAttribute (ccx=@0xffb6bda4) at /opt/moz/hg/src/js/src/xpconnect/src/xpcprivate.h:2296
#4  0x00395f4e in XPC_WN_GetterSetter (cx=0xf46d6600, obj=0xf6020260, argc=0, argv=0xef7b3110, vp=0xffb6beb0)
    at /opt/moz/hg/src/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1509
#5  0x03c3833b in js_Invoke (cx=0xf46d6600, argc=0, vp=0xef7b3108, flags=2) at /opt/moz/hg/src/js/src/jsinterp.cpp:1306
#6  0x03c38bd4 in js_InternalInvoke (cx=0xf46d6600, obj=0xf6020260, fval=-169256480, flags=0, argc=0, argv=0x0, rval=0xffb6cc64)
    at /opt/moz/hg/src/js/src/jsinterp.cpp:1381
#7  0x03c38e36 in js_InternalGetOrSet (cx=0xf46d6600, obj=0xf6020260, id=-138896596, fval=-169256480, mode=JSACC_READ, argc=0, 
    argv=0x0, rval=0xffb6cc64) at /opt/moz/hg/src/js/src/jsinterp.cpp:1442
#8  0x03c48b18 in js_NativeGet (cx=0xf46d6600, obj=0xf6020260, pobj=0xf5e958a0, sprop=0xee316a50, vp=0xffb6cc64)
    at /opt/moz/hg/src/js/src/jsobj.cpp:3664
#9  0x03c4b874 in js_GetPropertyHelper (cx=0xf46d6600, obj=0xf6020260, id=-138896596, vp=0xffb6cc64, entryp=0xffb6cb98)
    at /opt/moz/hg/src/js/src/jsobj.cpp:3813


Backtrace of failing revision is:
#0  nsXMLHttpRequest::GetOnreadystatechange (this=0xeeab1500, aOnreadystatechange=0xffef74f4)
    at /opt/moz/hg.326d60321ec7/src/content/base/src/nsXMLHttpRequest.cpp:1230
#1  0x013f798f in NS_InvokeByIndex_P () from ./libxul.so
#2  0x003892d2 in XPCWrappedNative::CallMethod (ccx=@0xffef7744, mode=XPCWrappedNative::CALL_GETTER)
    at /opt/moz/hg.326d60321ec7/src/js/src/xpconnect/src/xpcwrappednative.cpp:2422
#3  0x0039904d in XPCWrappedNative::GetAttribute (ccx=@0xffef7744)
    at /opt/moz/hg.326d60321ec7/src/js/src/xpconnect/src/xpcprivate.h:2296
#4  0x00396002 in XPC_WN_GetterSetter (cx=0xf46efa00, obj=0xf2ff16a0, argc=0, argv=0xee5ea110, vp=0xffef7850)
    at /opt/moz/hg.326d60321ec7/src/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1509
#5  0x03f9033b in js_Invoke (cx=0xf46efa00, argc=0, vp=0xee5ea108, flags=2)
    at /opt/moz/hg.326d60321ec7/src/js/src/jsinterp.cpp:1306
#6  0x03f90bd4 in js_InternalInvoke (cx=0xf46efa00, obj=0xf2ff16a0, fval=-218162720, flags=0, argc=0, argv=0x0, rval=0xffef7a58)
    at /opt/moz/hg.326d60321ec7/src/js/src/jsinterp.cpp:1381
#7  0x03efe396 in JS_CallFunctionValue (cx=0xf46efa00, obj=0xf2ff16a0, fval=-218162720, argc=0, argv=0x0, rval=0xffef7a58)
    at /opt/moz/hg.326d60321ec7/src/js/src/jsapi.cpp:5242
#8  0x003a5d90 in XPCWrapper::GetOrSetNativeProperty (cx=0xf46efa00, obj=0xf2ff1900, wrappedNative=0xeea38640, id=-138900692, 
    vp=0xffef87e4, aIsSet=0, isNativeWrapper=1) at /opt/moz/hg.326d60321ec7/src/js/src/xpconnect/src/XPCWrapper.cpp:717
#9  0x0039e925 in XPC_NW_GetOrSetProperty (cx=0xf46efa00, obj=0xf2ff1900, id=-138900692, vp=0xffef87e4, aIsSet=0)
    at /opt/moz/hg.326d60321ec7/src/js/src/xpconnect/src/XPCNativeWrapper.cpp:527
#10 0x0039e990 in XPC_NW_GetProperty (cx=0xf46efa00, obj=0xf2ff1900, id=-138900692, vp=0xffef87e4)
    at /opt/moz/hg.326d60321ec7/src/js/src/xpconnect/src/XPCNativeWrapper.cpp:533
#11 0x03fa0ba8 in js_NativeGet (cx=0xf46efa00, obj=0xf2ff1900, pobj=0xf2ff1900, sprop=0xf2d6c7d0, vp=0xffef87e4)
    at /opt/moz/hg.326d60321ec7/src/js/src/jsobj.cpp:3664
#12 0x03fa3874 in js_GetPropertyHelper (cx=0xf46efa00, obj=0xf2ff1900, id=-138900692, vp=0xffef87e4, entryp=0xffef8718)
    at /opt/moz/hg.326d60321ec7/src/js/src/jsobj.cpp:3813

And the NS_ERROR_INVALID_ARG error is thrown here:
#0  XPCNativeWrapper::GetNewOrUsed (cx=0xf46efa00, wrapper=0xf0d248c0, aObjectPrincipal=0x0)
    at /opt/moz/hg.326d60321ec7/src/js/src/xpconnect/src/XPCNativeWrapper.cpp:1109
#1  0x0039d97b in XPC_NW_RewrapIfDeepWrapper (cx=0xf46efa00, obj=0xf2ff1900, v=-218162912, rval=0xffef87e4)
    at /opt/moz/hg.326d60321ec7/src/js/src/xpconnect/src/XPCNativeWrapper.cpp:401
#2  0x003a76c8 in XPCWrapper::RewrapIfDeepWrapper (cx=0xf46efa00, obj=0xf2ff1900, v=-218162912, rval=0xffef87e4, 
    isNativeWrapper=1) at /opt/moz/hg.326d60321ec7/src/js/src/xpconnect/src/XPCWrapper.h:245
#3  0x003a5e0f in XPCWrapper::GetOrSetNativeProperty (cx=0xf46efa00, obj=0xf2ff1900, wrappedNative=0xeea38640, id=-138900692, 
    vp=0xffef87e4, aIsSet=0, isNativeWrapper=1) at /opt/moz/hg.326d60321ec7/src/js/src/xpconnect/src/XPCWrapper.cpp:730
#4  0x0039e925 in XPC_NW_GetOrSetProperty (cx=0xf46efa00, obj=0xf2ff1900, id=-138900692, vp=0xffef87e4, aIsSet=0)
    at /opt/moz/hg.326d60321ec7/src/js/src/xpconnect/src/XPCNativeWrapper.cpp:527
#5  0x0039e990 in XPC_NW_GetProperty (cx=0xf46efa00, obj=0xf2ff1900, id=-138900692, vp=0xffef87e4)
    at /opt/moz/hg.326d60321ec7/src/js/src/xpconnect/src/XPCNativeWrapper.cpp:533
#6  0x03fa0ba8 in js_NativeGet (cx=0xf46efa00, obj=0xf2ff1900, pobj=0xf2ff1900, sprop=0xf2d6c7d0, vp=0xffef87e4)
    at /opt/moz/hg.326d60321ec7/src/js/src/jsobj.cpp:3664
#7  0x03fa3874 in js_GetPropertyHelper (cx=0xf46efa00, obj=0xf2ff1900, id=-138900692, vp=0xffef87e4, entryp=0xffef8718)
    at /opt/moz/hg.326d60321ec7/src/js/src/jsobj.cpp:3813

1104	  // Prevent wrapping a double-wrapped JS object in an
1105	  // XPCNativeWrapper!
1106	  nsCOMPtr<nsIXPConnectWrappedJS> xpcwrappedjs(do_QueryWrappedNative(wrapper));
1107	
1108	  if (xpcwrappedjs) {
1109	    XPCThrower::Throw(NS_ERROR_INVALID_ARG, cx);
1110	
1111	    return nsnull;
1112	  }

That's propabably all I can do since I don't know this code at all.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Honza meant normal console, not error console.

Oh.  Would have been clearer to say that dump() needs to be enabled in the steps to reproduce...

Thank you for hunting down the regression range!

Over to the right component, nominating for blocking.  Obvious questions:

1)  Why did the wrapper cache change affect whether the extension gets an
    XPCNativeWrapper?  That looks really suspicious to me.
2)  Is throwing the right behavior given that this is an XPCWrappedJS for a
    function and we _do_ know how to XPCNativeWrapper functions?
3)  Should we perhaps just not wrap XPCWrappedJS in general?  Or can it happen
    that content can get at the double-wrapping JSObject and modify it?

Comment 9

[Peter Van der Beken [:peterv]]

We do seem to create the XPCNativeWrapper though:

Content accessed from chrome, wrapping wrapper ([object XMLHttpRequest @ 0x15a0d5b0 (native @ 0x15a0d4b0)]) in XPCNativeWrapper
Created new XPCNativeWrapper 0xe97b680 for wrapped native [object XMLHttpRequest @ 0x15a0d5b0 (native @ 0x15a0d4b0)]
Calling getter for onreadystatechange
Rewrapping for deep implicit wrapper
---
xhrTest.httpObserver FAILS [Exception... "Illegal value"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://xhrtest/content/xhrTest.js :: anonymous :: line 25"  data: no]
---

Comment 10

[Boris Zbarsky [:bzbarsky]]

Peter, which build was that in?  Michal's testing shows that we didn't use to get an XPCNativeWrapper and that after the wrapper cache checkin we do...

Comment 11

[Peter Van der Beken [:peterv]]

Yes, should have been clearer. I do see us creating a XPCNativeWrapper in trunk, probably because we now have a PreCreate hook that returns a parent for XMLHttpRequests.

Comment 12

[Peter Van der Beken [:peterv]]

So if we want to keep the old behaviour, we're going to have to reintroduce a way for XMLHttpRequest to keep its wrappers alive when somebody sets an expando on them. If we want to reuse the wrapper cache for that we need to be able to ensure that there's only ever one XPCWrappedNative for them, which means XPCNativeWrappers around that XPCWrappedNative when touching content XMLHttpRequests from chrome.

The check for nsIXPConnectWrappedJS in XPCNativeWrapper::GetNewOrUsed was introduced as part of the fix for bug 352124.

Comment 13

[Peter Van der Beken [:peterv]]

Talking this over with Blake, we really do want to wrap the XHR in a XPCNativeWrapper. We should probably return a XPCSafeJSObjectWrapper instead of throwing.

Comment 14

[Boris Zbarsky [:bzbarsky]]

That would make a lot of sense to me, yes.  Alternately, we could detect the case of a wrappedJS for a function object and return the XPCNativeWrapper for the function.  Not sure which is safer.

Comment 15

[Peter Van der Beken [:peterv]]

Attachment: v1

With this the test case shows

xhrTest.httpObserver SUCCEEDS [xpconnect wrapped nsIDOMEventListener @ 0x14bb4de0 (native @ 0x1454d660)]

Wrt comment 14, I think this is better than using the XPCNativeWrapper for the function (because we don't lose the nsIDOMEventListener wrapper), but not sure which is safer either.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 362094 [details] [diff] [review]
v1

I worry a little bit about GC safety here. Do we need to pass a strong root into XPC_SJOW_CONSTRUCT? Also, if _Construct returns false, we should just return nsnull, not throw a new exception.

Comment 17

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 18

[John J. Barton]

This bug has been marked FIXED but I see no evidence of a commit. Should this be fixed in FF 3.5.0?