462389 - XPCVariant used in nsXPCException::SetThrownJSVal can cause cycle collection on non-main threads

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

XPCVariants are cycle collected yet exceptions can be used from any thread. This means that any non-main thread that runs script that throws an exception can trigger cycle collection off the main thread which will most certainly violate a ton of the collector's assumptions.

Comment 1

[Ben Newman (:bnewman) (:benjamn)]

Attachment: proposed patch

Replaced the XPCVariant with an nsAutoJSValHolder, which is a slightly more general replacement for bent's nsAutoJSObjectHolder.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 345810 [details] [diff] [review]
proposed patch

>+        (mHeld = JS_AddNamedRootRT(aRt, &mGCThing, "nsAutoRootedJSObject")))

You'll want to change that to nsAutoJSValHolder.

>-      mObj = NULL;
>-    }
>+    if (mHeld && !(mHeld = !JS_RemoveRootRT(mRt, &mGCThing)))
>+      mRt = NULL;

The old code used to null the object on release to make sure that you didn't hand potentially dangling pointers around. I'd like to keep that.

>+nsXPCException::StealThrownJSVal(jsval *vp)
> {
>+    if(mThrownJSVal.IsHeld()) {
>+        *vp = mThrownJSVal;
>+        mThrownJSVal.Release();
>         return PR_TRUE;
>     }
>     return PR_FALSE;

Hm, maybe Release() should just return the jsval before nulling it, like forget() on nsAutoPtr. What do you think?

>+nsXPCException::StowThrownJSVal(JSContext *cx, jsval v)
> {
>-    mThrownJSVal = JSVAL_IS_TRACEABLE(v)
>-        ? new XPCTraceableVariant(nsXPConnect::GetRuntimeInstance(), v)
>-        : new XPCVariant(v);
>+    mThrownJSVal.Hold(cx);

That can (in theory, maybe not once we have infallible malloc?) fail, and we'd be holding a dangling object if it did. I think you should adjust the signature and check for failure accordingly.

Comment 3

[Ben Newman (:bnewman) (:benjamn)]

Attachment: implementing bent's suggestions

(In reply to comment #2)
> (From update of attachment 345810 [details] [diff] [review])
> >+        (mHeld = JS_AddNamedRootRT(aRt, &mGCThing, "nsAutoRootedJSObject")))
> 
> You'll want to change that to nsAutoJSValHolder.

Done.
 
> >-      mObj = NULL;
> >-    }
> >+    if (mHeld && !(mHeld = !JS_RemoveRootRT(mRt, &mGCThing)))
> >+      mRt = NULL;
> 
> The old code used to null the object on release to make sure that you didn't
> hand potentially dangling pointers around. I'd like to keep that.

No problem.  Also nullifying mGCThing, while we're at it.

> >+nsXPCException::StealThrownJSVal(jsval *vp)
> > {
> >+    if(mThrownJSVal.IsHeld()) {
> >+        *vp = mThrownJSVal;
> >+        mThrownJSVal.Release();
> >         return PR_TRUE;
> >     }
> >     return PR_FALSE;
> 
> Hm, maybe Release() should just return the jsval before nulling it, like
> forget() on nsAutoPtr. What do you think?

I like that idea.

> >+nsXPCException::StowThrownJSVal(JSContext *cx, jsval v)
> > {
> >-    mThrownJSVal = JSVAL_IS_TRACEABLE(v)
> >-        ? new XPCTraceableVariant(nsXPConnect::GetRuntimeInstance(), v)
> >-        : new XPCVariant(v);
> >+    mThrownJSVal.Hold(cx);
> 
> That can (in theory, maybe not once we have infallible malloc?) fail, and we'd
> be holding a dangling object if it did. I think you should adjust the signature
> and check for failure accordingly.

Sure, and calling Release on failure also seems like a good idea.

Thanks for the good ideas, Ben.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 346053 [details] [diff] [review]
implementing bent's suggestions

>+  nsAutoJSValHolder()
>+    : mRt(NULL)
>+    , mVal(JSVAL_NULL)
>+    , mGCThing(JSVAL_NULL)

This works but it's more correct to set void* to NULL, not JSVAL_NULL. Let's fix that.

>   JSBool Hold(JSRuntime* aRt) {
>     if (!mHeld) {
>+      if (JS_AddNamedRootRT(aRt, &mGCThing, "nsAutoJSValHolder")) {
>         mRt = aRt;
>+        mHeld = JS_TRUE;
>+      } else Release(); // out of memory

This should be on two separate lines, with braces (since the if section needed braces).

>+  jsval Release() {
>     NS_ASSERTION(!mHeld || mRt, "Bad!");
>+
>+    jsval oldval = mVal;
>+
>+    if (mHeld)
>+      mHeld = !JS_RemoveRootRT(mRt, &mGCThing);

So I just chatted with mrbkap and JS_RemoveRootRT can't fail... Let's clean this code up in light of that.

>+    if (!mHeld) {
>+      mVal = JSVAL_NULL;
>+      mGCThing = JSVAL_NULL;

NULL.

>+    mGCThing = JSVAL_IS_GCTHING(aOther)
>+             ? JSVAL_TO_GCTHING(aOther)
>+             : JSVAL_NULL;

NULL.

>+    if(mThrownJSVal.IsHeld()) {
>+        *vp = mThrownJSVal.Release();
>         return PR_TRUE;
>     }
>     return PR_FALSE;

Hm, this check would make more sense as 'if(mThrownJSVal)' since you never assign without holding. That way you could always just set *vp = mThrownJSVal.Release() and return true.

Oh, and XPConnect style always has opening braces on the next line.

Comment 5

[Ben Newman (:bnewman) (:benjamn)]

Attachment: addressing further comments

(In reply to comment #4)
> >+    if(mThrownJSVal.IsHeld()) {
> >+        *vp = mThrownJSVal.Release();
> >         return PR_TRUE;
> >     }
> >     return PR_FALSE;
> 
> Hm, this check would make more sense as 'if(mThrownJSVal)' since you never
> assign without holding. That way you could always just set *vp =
> mThrownJSVal.Release() and return true.

Agree with everything except this.  Since not all jsvals are objects, mThrownJSVal might evaluate to a NULL JSObject* when we're actually holding a (primitive) jsval.

Otherwise, hope this all looks good.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 346086 [details] [diff] [review]
addressing further comments

Looks great!

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Pushed to moz-central.

Comment 8

[(no longer active)]

(In reply to comment #7)
> Pushed to moz-central.

For future reference: <http://hg.mozilla.org/mozilla-central/rev/6004a7f6e478>

Comment 9

[[e-mail]]

Comment on attachment 346086 [details] [diff] [review]
addressing further comments

>   /**
>    * Pretend to be a JSObject*.
>    */
>+  operator JSObject*() const {
>+    return JSVAL_IS_OBJECT(mVal)
>+         ? JSVAL_TO_OBJECT(mVal)
>+         : JSVAL_NULL;
>   }
> 
>   /**
>+   * Pretend to be a jsval.
>    */
>+  operator jsval() const { return mVal; }
This confuses VC7.1 because it doesn't know which operator to use for !

Comment 10

[Evan Yan]

Sun CC is not happy with it either.

"mozilla-central/dom/src/threads/nsDOMWorkerScriptLoader.cpp", line 200: Error: Overloading ambiguity between "nsAutoJSValHolder::operator JSObject*() const" and "nsAutoJSValHolder::operator long() const".
"mozilla-central/dom/src/threads/nsDOMWorkerScriptLoader.cpp", line 204: Error: Overloading ambiguity between "nsAutoJSValHolder::operator JSObject*() const" and "nsAutoJSValHolder::operator long() const".
"mozilla-central/dom/src/threads/nsDOMWorkerScriptLoader.cpp", line 694: Error: Overloading ambiguity between "nsAutoJSValHolder::operator JSObject*() const" and "nsAutoJSValHolder::operator long() const".
"mozilla-central/dom/src/threads/nsDOMWorkerScriptLoader.cpp", line 726: Error: Overloading ambiguity between "nsAutoJSValHolder::operator JSObject*() const" and "nsAutoJSValHolder::operator long() const".
4 Error(s) detected.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We should remove the JSObject* operator in favor of the jsval operator.

Comment 12

[Ben Newman (:bnewman) (:benjamn)]

Attachment: changed operator JSObject* to a method called ToJSObject

Probably a good policy to avoid having more than one conversion operator per class.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 347599 [details] [diff] [review]
changed operator JSObject* to a method called ToJSObject

r=me if you change the JSVAL_NULL to NULL :-)

Comment 14

[Ben Newman (:bnewman) (:benjamn)]

Attachment: NULL instead of JS_NULL

Neil and Evan, would you like to sign off on this version?

Comment 15

[[e-mail]]

Comment on attachment 347627 [details] [diff] [review]
NULL instead of JS_NULL

>   /**
>-   * Pretend to be a JSObject*.
>+   * Explicit JSObject* conversion.
>    */
>-  operator JSObject*() const {
>+  JSObject* ToJSObject() const {
>     return JSVAL_IS_OBJECT(mVal)
>          ? JSVAL_TO_OBJECT(mVal)
>-         : JSVAL_NULL;
>+         : NULL;
>   }
Compiles fine here on VC7.1 but shouldn't that be nsnull instead of NULL?

Comment 16

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #15)
> Compiles fine here on VC7.1 but shouldn't that be nsnull instead of NULL?

I'm inclined to agree.  My only reason for using NULL instead of nsnull was that the original nsAutoJSObjectHolder implementation used NULL.  Maybe bent (original author) has an opinion on this?

Comment 17

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #16)
> (In reply to comment #15)
> > Compiles fine here on VC7.1 but shouldn't that be nsnull instead of NULL?
> 
> I'm inclined to agree.  My only reason for using NULL instead of nsnull was
> that the original nsAutoJSObjectHolder implementation used NULL.  Maybe bent
> (original author) has an opinion on this?

bent says he did it because the JS engine uses NULL, so I guess it's a moot point.  they're all 0, after all.

Comment 18

[Evan Yan]

Comment on attachment 347627 [details] [diff] [review]
NULL instead of JS_NULL

I'm not a superreviewer, but I can confirm that it makes Sun CC happy. Thanks!

Comment 20

[Boying Lu]

Add checkin-needed for the last patch.

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 347627 [details] [diff] [review]
NULL instead of JS_NULL

This trivial patch fixes build bustage for b2 on solaris and windows with vc7.1.

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 347627 [details] [diff] [review]
NULL instead of JS_NULL

a1.9.1b2=beltzner

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed e67d4de99d98

Comment 24

[timeless]

this didn't actually fix threadsafety. you're still terribly broken. in addition to doing evil things - bug 467900