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] ...
Oops, *closing* the testcase triggers the assertion failure.
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|?
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...
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?
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.
We should get this fixed on branches too...
Comment on attachment 472659 [details] [diff] [review] Fix Approved for 184.108.40.206 and 220.127.116.11, a=dveditz for release-drivers
Pushed: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4dd3e3a46239 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0d6dd74ef6fe
(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?
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.