583943 - "Assertion failure: JSVAL_IS_DOUBLE" with XBL, mangled __proto__

Status: RESOLVED FIXED

(Core :: XBL, defect, P1)

Description

[Jesse Ruderman]

Attachment: testcase

On Mac trunk, the testcase causes the following assertion.  I think I'm using a 32-bit build.

Assertion failure: JSVAL_IS_DOUBLE(v), at ../../../dist/include/jsapi.h:283

 0  libmozjs.dylib!JS_Assert [jsutil.cpp:1e989961cfed : 81 + 0x5]
 1  XUL!JSVAL_TO_PRIVATE [jsapi.h : 283 + 0x2e]
 2  XUL!nsXBLBinding::ChangeDocument [nsXBLBinding.cpp:1e989961cfed : 1145 + 0xa]
 3  XUL!nsBindingManager::RemovedFromDocumentInternal [nsBindingManager.cpp:1e989961cfed : 652 + 0x23]
 4  XUL!nsBindingManager::RemovedFromDocument [nsBindingManager.h : 98 + 0x18]
...

Comment 1

[Jesse Ruderman]

Oops, *closing* the testcase triggers the assertion failure.

Comment 2

[Luke Wagner [:luke]]

I got this under a debugger.  It looks like the value being returned by JS_GetReservedSlot (http://mxr.mozilla.org/mozilla-central/source/content/xbl/src/nsXBLBinding.cpp#1139) is 'undefined'.  Now, pre-fatvals, JSVAL_TO_PRIVATE didn't assert that the given value was indeed a private (the lsb was set), it just masked the bit and returned the pointer.  For the code pointed to by the mxr link, JSVAL_TO_PRIVATE(JSVAL_VOID) would != mPrototypeBinding and thus everything would sorta work.

I'm not familiar enough with this code to know the right fix, but it seems to me its either:
 1) change the condition in ChangeDocument to

      if (JSVAL_IS_VOID(protoBinding) || JSVAL_TO_PRIVATE(protoBinding) != mPrototypeBinding)

 2) make sure JS_SetReservedSlot is always called before JS_GetReservedSlot.

Comment 3

[Boris Zbarsky [:bzbarsky]]

(2) should already be happening, last I checked.  All sorts of code depends on being able to get that reserved slot...

When you hit this, what does the proto chain for the object in |v| look like?  What's the actual JSClass in |clazz|?

Comment 4

[Luke Wagner [:luke]]

At nsXBLBinding.cpp:1145:

(gdb) call js_DumpObject(proto)
object 0xa89023f0
class 0xae6a1b64 XULElement
properties:
proto <XPC_WN_ModsAllowed_NoCall_Proto_JSClass object at 0xa8902330>
parent <HTMLDocument object at 0xae5f1360>
private 0xa8a80500
slots:
   1 (reserved) = undefined
   2 = undefined

To wit, proto == JS_GetPrototype(cx, scriptObject), and:

(gdb) call js_DumpObject(scriptObject)
object 0xa8902420
class 0xae6a1b64 XULElement
properties:
proto <XULElement object at 0xa89023f0>
parent <HTMLDocument object at 0xae5f1360>
private 0xa8ac0e40
slots:
   1 (reserved) = undefined

Comment 5

[Boris Zbarsky [:bzbarsky]]

Aha.  OK, I see what's going on here.

So the issue is that XBL munges the proto chain when a binding is attached: it inserts an object between the node and its normal proto.  When a binding is detached it tries to find and remove this object.

But this testcase _also_ munges the proto chain, so that we get a proto chain like:

  b -> a -> xbl proto for a -> normal proto for a

Then when we go to try to remove the xbl proto for b, we run into |a| on the proto chain, and |a| in fact has a reserved slot but that reserved slot has nothing to do with xbl whatsoever.

So in fact, the slot could contain any jsval.  Luke, would testing for JSVAL_IS_PRIVATE before doing JSVAL_TO_PRIVATE do the right thing for us here?  And even then... it could be some random non-pointer private that has nothing to do with us.  So if there's a better way to locate "our" proto that would be nice...

Comment 6

[Luke Wagner [:luke]]

There is no JSVAL_IS_PRIVATE: a private is not a separate type of jsval, but a void* that has been crammed into some non-GC'd jsval type (currently, a double).  So, it is possible that you could have a real double that happens to look like PRIVATE_TO_JSVAL(mPrototypeBinding).  Is there some other property of proto you can look at to know that its reserved slot is a privatized jsval?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Is there some other property of proto you can look at to know that its reserved
> slot is a privatized jsval?

Hmm....  As a matter of fact, maybe there is.  Specifically, we could examine the resolve or finalize hooks on its JSClass to see whether those match the hooks that our proto's JSClass should have.  That should be safe enough, I'd think.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Hmm.   The attached testcase no longer works since we disabled remote XBL/XUL.  Jonas, is there a sane way I can test this in our test suites?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 472659 [details] [diff] [review]
Fix

I can't say that I'm very good with how these flags work, but this does make sense to me.

Comment 11

[Boris Zbarsky [:bzbarsky]]

We should get this fixed on branches too...

Comment 12

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/6e0a42c4af82

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 472659 [details] [diff] [review]
Fix

Approved for 1.9.2.11 and 1.9.1.14, a=dveditz for release-drivers

Comment 16

[Boris Zbarsky [:bzbarsky]]

Pushed:
  http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4dd3e3a46239
  http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0d6dd74ef6fe

Comment 17

[Al Billings [:abillings - ex-MoCo]]

(In reply to comment #0)
> Created attachment 462288 [details]
> testcase
> 
> On Mac trunk, the testcase causes the following assertion.  I think I'm using a
> 32-bit build.
> 
> Assertion failure: JSVAL_IS_DOUBLE(v), at ../../../dist/include/jsapi.h:283
> 
>  0  libmozjs.dylib!JS_Assert [jsutil.cpp:1e989961cfed : 81 + 0x5]
>  1  XUL!JSVAL_TO_PRIVATE [jsapi.h : 283 + 0x2e]
>  2  XUL!nsXBLBinding::ChangeDocument [nsXBLBinding.cpp:1e989961cfed : 1145 +
> 0xa]
>  3  XUL!nsBindingManager::RemovedFromDocumentInternal
> [nsBindingManager.cpp:1e989961cfed : 652 + 0x23]
>  4  XUL!nsBindingManager::RemovedFromDocument [nsBindingManager.h : 98 + 0x18]
> ...

I'm not seeing this in a pre-fix 1.9.2 OS X build that I'm running. Are we sure that the assert appeared on branch builds?

Comment 18

[Boris Zbarsky [:bzbarsky]]

That exact assert might not have, depending on the jseng guts.  We would have gotten incorrect behavior, however... (e.g. leaks, if you construct the right testcase).

Comment 19

[Al Billings [:abillings - ex-MoCo]]

I have the one testcase that is given here. 

The only thing I see is:

WARNING: NS_ENSURE_TRUE(mMutable) failed: file /Users/albill/mozilla-192/netwerk/base/src/nsSimpleURI.cpp, line 224

Comment 20

[Boris Zbarsky [:bzbarsky]]

Look, there have been a LOT of js engine changes since 1.9.2.  The API misuse is present on 1.9.2.  I wouldn't be surprised if the js engine doesn't assert on that API misuse on 1.9.2.

Comment 21

[Al Billings [:abillings - ex-MoCo]]

I'm well aware of that, Boris, which is why I asked my initial question about whether or not we knew if the assert ever appeared on branch. I'm going to mark this as not needing further QA on branch.