318489 - Unable to create new XMLHttpRequests from an XPCNativeWrapped window

Status: RESOLVED FIXED

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

Description

[Blake Kaplan (:mrbkap) (inactive)]

This is apparently biting GreaseMonkey pretty badly. The problem is that we currently implement the various class constructors using a magical sDOMJSClass (in nsDOMClassInfo.cpp). When we try to extract the property out of the XPCNativeWrapper, we try to wrap it, but since it's one of these special classes, we are unable to wrap it, and refuse access to it.

I have a patch that fixes this. We should try to get it in as quickly as possible (given its size) and shoot for the branch.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix, v1

This makes the constructors implement a real, live C++ interface that you can QI to, so that XPCNativeWrapper is happy.

I had to fix XPCNativeWrapper's construct hook to forward to the scriptable helper's request. Note that my patch also fixes a problem where stuff like: new (new XPCNativeWrapper(window)).location() would botch a JS assertion because |obj| in that case is not an XPCNativeWrapper, but is instead a new object.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 204643 [details] [diff] [review]
Fix, v1

Boris, if you have spare minutes to glance through this, it'd be appreciated.

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 204643 [details] [diff] [review]
Fix, v1

- In nsDOMConstructor::ToString():

+  aResult.Assign(PRUnichar('['));
+  aResult.Append(mClassName);
+  aResult.Append(PRUnichar(']'));

At some point we should change this to return "[object <classname>]" instead of just "[<classname>]" to match what the JS engine does. But that's probably worth doing in a separate bug...

- In XPC_NW_Construct():

+  JSObject *realobj = JSVAL_TO_OBJECT(argv[-2]);
+  XPCWrappedNative *wrappedNative =
+    XPCNativeWrapper::GetWrappedNative(cx, realobj);
+  if (wrappedNative) {
+    JSBool retval = JS_TRUE;
+
+    if (NATIVE_HAS_FLAG(wrappedNative, WantConstruct)) {
+      nsresult rv = wrappedNative->GetScriptableInfo()->
+        GetCallback()->Construct(wrappedNative, cx, realobj, argc, argv, rval,
+                                 &retval);
+      if (NS_FAILED(rv)) {
+        return ThrowException(rv, cx);
+      }
+    }
+
+    return retval;
+  }

Seems worth explaining this a bit :)

   XPC_NW_BYPASS_TEST(cx, obj, construct, (cx, obj, argc, argv, rval));

And we saw this hit a JS engine assert if obj wasn't a native wrapper, wanna stick some more graceful error checking into ShouldBypassNativeWrapper() while you're here (separate patch/bug is fine)?

r=jst

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 204643 [details] [diff] [review]
Fix, v1

What jst said (please file that bug on [object <domclassname>] instead of [<domclassname>] -- wonder what that change will break :-P).  Additional comments below.

> JS_STATIC_DLL_CALLBACK(JSBool)
> XPC_NW_Construct(JSContext *cx, JSObject *obj, uintN argc, jsval *argv,
>                  jsval *rval)
> {
>+  JSObject *realobj = JSVAL_TO_OBJECT(argv[-2]);

wn_obj is the name used elsewhere in this file, and realobj is an unusual name (I guess XPCNativeWrappers are a bit unreal, but... ;-).

>+  XPCWrappedNative *wrappedNative =
>+    XPCNativeWrapper::GetWrappedNative(cx, realobj);
>+  if (wrappedNative) {
>+    JSBool retval = JS_TRUE;
>+
>+    if (NATIVE_HAS_FLAG(wrappedNative, WantConstruct)) {
>+      nsresult rv = wrappedNative->GetScriptableInfo()->
>+        GetCallback()->Construct(wrappedNative, cx, realobj, argc, argv, rval,
>+                                 &retval);
>+      if (NS_FAILED(rv)) {
>+        return ThrowException(rv, cx);
>+      }
>+    }
>+
>+    return retval;
>+  }
>+

As you pointed out to me, and per this comment early in the file:

// If one of our class hooks is ever called from a non-system script, bypass
// the hook by calling the same hook on our wrapped native, with obj reset to
// the wrapped native's flat JSObject, so the hook and args macro parameters
// can be simply:
//
//      convert, (cx, obj, type, vp)
//
// in the call from XPC_NW_Convert, for example.

the following needs to pass realobj, er, wn_obj:

>   XPC_NW_BYPASS_TEST(cx, obj, construct, (cx, obj, argc, argv, rval));
> 
>   return JS_TRUE;
> }

sr=me with all this.

/be

Comment 5

[Aaron Boodman]

I was wondering, will this patch address the other xmlextras objects?
Like DOMParser and XMLSerializer?

What about other special global javascript objects, like Image? In the
case of Image, I would argue that it doesn't really even make sense to
create an Image outside of the context of HTMLDocument.

Therefore I guess I'm saying that xmlextras should be in, but Image,
Option, etc should be out.

Just my .02.

Comment 6

[Brendan Eich [:brendan]]

Image, Option, and XMLHttpRequest are treated the same, and should be.  There's no difference in kind that XPCNativeWrapper can detect.  What's more, there is no reason I know of to break symmetry.  Image, for example, could be used from a sandbox to preload images that are then parented by a document.  Ok, what am I missing?

/be

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better XPCNativeWrapper changes

My last patch didn't wrap the returned object from the constructor. This patch does.

Note that even with my patch XPInstall is still b0rked wrt XPCNativeWrapper in the same way. E.g., |new XPCNativeWrapper(window).InstallTrigger| returns null. I think that XPInstall is going to need some amount of whacking in order to get this to work correctly.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

I filed bug 318770 on the "[object <domclassname>]" fix.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better XPCNativeWrapper changes, v2

RewrapIfDeepWrapper is my friend (if a mite misnamed). Also, this throws if the given WrappedNative is not constructable.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 204836 [details] [diff] [review]
Better XPCNativeWrapper changes, v2

r=jst

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 204836 [details] [diff] [review]
Better XPCNativeWrapper changes, v2

> JS_STATIC_DLL_CALLBACK(JSBool)
> XPC_NW_Construct(JSContext *cx, JSObject *obj, uintN argc, jsval *argv,
>                  jsval *rval)
> {
>+  // The object given to us by the JS engine is actually a stub object (the
>+  // "new" object). This isn't any help to us, so instead use the function
>+  // object of the constructor that we're calling (which is the native
>+  // wrapper).
>+  if (!(obj = JSVAL_TO_OBJECT(argv[-2]))) {
>+    return JS_TRUE;
>+  }

The callee can never be null, so just set obj (assert if you must).

>+    if (NATIVE_HAS_FLAG(wrappedNative, WantConstruct)) {

Invert this test and do the short else, which returns, in the then part.  That avoids overindenting, avoids the if (c) s1; else return; s2 arbitrary control flow (why are s1 and s2 split apart?), and most of all puts the odd or uninteresting case first and casts it out with return.

>+      nsresult rv = wrappedNative->GetScriptableInfo()->
>+        GetCallback()->Construct(wrappedNative, cx, obj, argc, argv, rval,
>+                                 &retval);
>+      if (NS_FAILED(rv)) {
>+        return ThrowException(rv, cx);
>+      }
>+
>+      if (!retval) {
>+        return JS_FALSE;
>+      }
>+
>+      // Returning a primitive from a constructor is invalid, per ECMA-262.
>+      if (JSVAL_IS_PRIMITIVE(*rval)) {
>+        return ThrowException(NS_ERROR_ILLEGAL_VALUE, cx);
>+      }
>+
>+      if (!RewrapIfDeepWrapper(cx, obj, *rval, rval)) {
>+        return JS_FALSE;
>+      }

So all the above unindents one level, and in fact (s2 is just return retval, and we know retval is true here) you can just return RewrapIfDeepWrapper's result as the last statement.

>+    } else {
>+      return ThrowException(NS_ERROR_INVALID_ARG, cx);
>+    }
>+
>+    return retval;
>+  }

Similar argument applies to the outer if (c0) {...} after which comes this return:

>+
>   return JS_TRUE;
> }

Fix that and sr=me.

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 204836 [details] [diff] [review]
Better XPCNativeWrapper changes, v2

Marking sr=brendan per comment 11.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk. Nominating for security releases since this makes it much harder to write safe Greasemonkey userscripts, since things like DOMParser and XMLHttpRequest have to be constructed through the unsafeWindow.

Comment 14

[cls]

Yesterday's checkin caused the mingw build to break.

c:/root/obj-ff-opt/dom/src/base/../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp: In member function `nsresult nsDOMConstructor::Install(JSContext*, JSObject*, jsval)':
c:/root/obj-ff-opt/dom/src/base/../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:4592: error: invalid conversion from `const PRUnichar*' to `const jschar*'
c:/root/obj-ff-opt/dom/src/base/../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp: In static member function `static nsresult nsWindowSH::GlobalResolve(nsGlobalWindow*, JSContext*, JSObject*, JSString*, PRUint32, PRBool*)':
c:/root/obj-ff-opt/dom/src/base/../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:5218: error: no matching function for call to `nsDOMConstructor::nsDOMConstructor(jschar*)'
c:/root/obj-ff-opt/dom/src/base/../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:4568: note: candidates are: nsDOMConstructor::nsDOMConstructor(const nsDOMConstructor&)
c:/root/obj-ff-opt/dom/src/base/../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:4571: note:                 nsDOMConstructor::nsDOMConstructor(const PRUnichar*) <near match>
c:/root/obj-ff-opt/dom/src/base/../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:5277: error: invalid conversion from `jschar*' to `const PRUnichar*'

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: mingw fix

cls, does this fix the bustage for you?

Comment 16

[cls]

Comment on attachment 205500 [details] [diff] [review]
mingw fix

Yep.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

MinGW bustage fix checked in. Marking this bug as FIXED, again.

Comment 18

[Daniel Veditz [:dveditz]]

This set of patches is scaring drivers for 1.8.0.1, could you comment on the risks (and which patches, exactly, would need to go in)?

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Rolled-up patch

This is the rolled up patch. While creating it, I found and fixed bug 322971 (which is a trivial fix that won't be exercised by our code at the moment).

Comment 20

[Mike Schroepfer]

Jesse - can you confirm that this fixes the greasemonkey issues?

Comment 21

[Jesse Ruderman]

I wrote tests for XMLHttpRequest, DOMParser, and Image:
http://www.squarefree.com/bug318489/index2.html

Today's trunk build passes these tests.

Things I noticed:

* Sometimes, when switching from one trunk build to another, Greasemonkey wouldn't work at all on the first launch.  I saw some error in the JavaScript console about "Config" not being defined.

* To use XMLHttpRequest from Greasemonkey, you have to give it an absolute URL.

Comment 22

[Jesse Ruderman]

My "Valid XHTML" script, which uses DOMParser, works correctly on trunk with the unsafeWindow hack removed :)

Btw, I'm testing on Windows.

Comment 23

[Aaron Boodman]

The config thing is an existing GM bug. Disregard.

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 208122 [details] [diff] [review]
Rolled-up patch

a=dveditz/shrep with Jesse's trunk verification.

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

I checked a mildly modified version of the rolled up patch into the branches last night.

Comment 26

[Marcia Knous [:marcia]]

Jesse: Since you have the GM scripts, can you please verify this on the branch as well?

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 204643 [details] [diff] [review]
Fix, v1

I'll never actually get to this...  I skimmed and it looks ok, but a review it's not.