"Assertion failure: JSVAL_IS_DOUBLE" with XBL, mangled __proto__

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
XBL
P1
critical
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Jesse Ruderman, Assigned: bz)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Trunk
mozilla2.0b7
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking2.0 final+, status1.9.2 .11-fixed, status1.9.1 .14-fixed)

Details

(Whiteboard: [qa-ntd-192] [qa-ntd-191])

Attachments

(2 attachments)

(Reporter)

Description

7 years ago
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]
...
(Reporter)

Comment 1

7 years ago
Oops, *closing* the testcase triggers the assertion failure.

Comment 2

7 years ago
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.
(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

7 years ago
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
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

7 years ago
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?
> 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.
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?
Assignee: nobody → bzbarsky
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Whiteboard: [need review]
Created attachment 472659 [details] [diff] [review]
Fix
Attachment #472659 - Flags: review?(jonas)
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.
Attachment #472659 - Flags: review?(jonas) → review+
We should get this fixed on branches too...
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
Whiteboard: [need review] → [need approval]
Attachment #472659 - Flags: approval2.0?

Updated

7 years ago
Attachment #472659 - Flags: approval2.0? → approval2.0+
Whiteboard: [need approval] → [need landing]
Pushed http://hg.mozilla.org/mozilla-central/rev/6e0a42c4af82
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b7
Attachment #472659 - Flags: approval1.9.2.11?
Attachment #472659 - Flags: approval1.9.1.14?
Whiteboard: [need landing]
blocking2.0: ? → final+
blocking1.9.1: ? → ---
blocking1.9.2: ? → ---
status1.9.1: --- → wanted
status1.9.2: --- → wanted
Comment on attachment 472659 [details] [diff] [review]
Fix

Approved for 1.9.2.11 and 1.9.1.14, a=dveditz for release-drivers
Attachment #472659 - Flags: approval1.9.2.11?
Attachment #472659 - Flags: approval1.9.2.11+
Attachment #472659 - Flags: approval1.9.1.14?
Attachment #472659 - Flags: approval1.9.1.14+
Pushed:
  http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4dd3e3a46239
  http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0d6dd74ef6fe
status1.9.1: wanted → .14-fixed
status1.9.2: wanted → .11-fixed
(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?
Whiteboard: [qa-examined-192]
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).
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
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.
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.
Whiteboard: [qa-examined-192] → [qa-ntd-192] [qa-ntd-191]
You need to log in before you can comment on or make changes to this bug.