Last Comment Bug 583943 - "Assertion failure: JSVAL_IS_DOUBLE" with XBL, mangled __proto__
: "Assertion failure: JSVAL_IS_DOUBLE" with XBL, mangled __proto__
Status: RESOLVED FIXED
[qa-ntd-192] [qa-ntd-191]
: assertion, testcase
Product: Core
Classification: Components
Component: XBL (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: mozilla2.0b7
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: 326633 344486
  Show dependency treegraph
 
Reported: 2010-08-02 18:27 PDT by Jesse Ruderman
Modified: 2010-09-21 14:45 PDT (History)
5 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
.11-fixed
.14-fixed


Attachments
testcase (408 bytes, text/html)
2010-08-02 18:27 PDT, Jesse Ruderman
no flags Details
Fix (2.04 KB, patch)
2010-09-07 09:57 PDT, Boris Zbarsky [:bz]
jonas: review+
jst: approval2.0+
dveditz: approval1.9.2.11+
dveditz: approval1.9.1.14+
Details | Diff | Splinter Review

Description Jesse Ruderman 2010-08-02 18:27:51 PDT
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]
...
Comment 1 Jesse Ruderman 2010-08-02 18:32:08 PDT
Oops, *closing* the testcase triggers the assertion failure.
Comment 2 Luke Wagner [:luke] 2010-08-03 10:49:01 PDT
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 [:bz] 2010-08-03 12:32:42 PDT
(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] 2010-08-03 13:12:03 PDT
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 [:bz] 2010-08-03 13:36:09 PDT
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] 2010-08-03 13:57:35 PDT
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 [:bz] 2010-08-03 22:10:03 PDT
> 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 [:bz] 2010-09-07 08:26:38 PDT
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 [:bz] 2010-09-07 09:57:50 PDT
Created attachment 472659 [details] [diff] [review]
Fix
Comment 10 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-09-13 14:14:12 PDT
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 [:bz] 2010-09-14 10:55:37 PDT
We should get this fixed on branches too...
Comment 12 Boris Zbarsky [:bz] 2010-09-15 14:52:51 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/6e0a42c4af82
Comment 15 Daniel Veditz [:dveditz] 2010-09-20 10:31:04 PDT
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 17 Al Billings [:abillings] 2010-09-21 13:46:15 PDT
(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 [:bz] 2010-09-21 13:47:42 PDT
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] 2010-09-21 13:48:52 PDT
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 [:bz] 2010-09-21 13:54:58 PDT
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] 2010-09-21 14:45:54 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.