Closed Bug 318489 Opened 16 years ago Closed 16 years ago

Unable to create new XMLHttpRequests from an XPCNativeWrapped window

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: mrbkap, Assigned: mrbkap)

References

Details

(Keywords: fixed1.8.0.1, fixed1.8.1)

Attachments

(4 files, 1 obsolete file)

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.
Status: NEW → ASSIGNED
Priority: -- → P2
Attached patch Fix, v1Splinter Review
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.
Attachment #204643 - Flags: superreview?(brendan)
Attachment #204643 - Flags: review?(jst)
Comment on attachment 204643 [details] [diff] [review]
Fix, v1

Boris, if you have spare minutes to glance through this, it'd be appreciated.
Attachment #204643 - Flags: review?(bzbarsky)
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
Attachment #204643 - Flags: review?(jst) → review+
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
Attachment #204643 - Flags: superreview?(brendan) → superreview+
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.
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
Attached patch Better XPCNativeWrapper changes (obsolete) — Splinter Review
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.
Attachment #204818 - Flags: superreview?(brendan)
Attachment #204818 - Flags: review?(jst)
I filed bug 318770 on the "[object <domclassname>]" fix.
RewrapIfDeepWrapper is my friend (if a mite misnamed). Also, this throws if the given WrappedNative is not constructable.
Attachment #204818 - Attachment is obsolete: true
Attachment #204836 - Flags: superreview?(brendan)
Attachment #204836 - Flags: review?(jst)
Attachment #204818 - Flags: superreview?(brendan)
Attachment #204818 - Flags: review?(jst)
Comment on attachment 204836 [details] [diff] [review]
Better XPCNativeWrapper changes, v2

r=jst
Attachment #204836 - Flags: review?(jst) → review+
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 on attachment 204836 [details] [diff] [review]
Better XPCNativeWrapper changes, v2

Marking sr=brendan per comment 11.
Attachment #204836 - Flags: superreview?(brendan) → superreview+
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.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: blocking1.8.1?
Flags: blocking1.8.0.1?
Resolution: --- → FIXED
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*'
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch mingw fixSplinter Review
cls, does this fix the bustage for you?
Attachment #205500 - Flags: superreview?(brendan)
Attachment #205500 - Flags: review?(cls)
Comment on attachment 205500 [details] [diff] [review]
mingw fix

Yep.
Attachment #205500 - Flags: review?(cls) → review+
MinGW bustage fix checked in. Marking this bug as FIXED, again.
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
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)?
Flags: blocking1.8.0.2?
Depends on: 322971
Attached patch Rolled-up patchSplinter Review
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).
Attachment #208122 - Flags: superreview+
Attachment #208122 - Flags: review+
Attachment #208122 - Flags: approval1.8.1?
Attachment #208122 - Flags: approval1.8.0.1?
Jesse - can you confirm that this fixes the greasemonkey issues?
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.
My "Valid XHTML" script, which uses DOMParser, works correctly on trunk with the unsafeWindow hack removed :)

Btw, I'm testing on Windows.
The config thing is an existing GM bug. Disregard.
Comment on attachment 208122 [details] [diff] [review]
Rolled-up patch

a=dveditz/shrep with Jesse's trunk verification.
Attachment #208122 - Flags: approval1.8.1?
Attachment #208122 - Flags: approval1.8.1+
Attachment #208122 - Flags: approval1.8.0.1?
Attachment #208122 - Flags: approval1.8.0.1+
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1+
I checked a mildly modified version of the rolled up patch into the branches last night.
Jesse: Since you have the GM scripts, can you please verify this on the branch as well?
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.
Attachment #204643 - Flags: review?(bzbarsky)
Attachment #205500 - Flags: superreview?(brendan) → superreview+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.