Closed Bug 651030 Opened 9 years ago Closed 9 years ago

"Assertion failure: JSVAL_IS_DOUBLE_IMPL(data)"

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
firefox5 --- unaffected
firefox6 + fixed
firefox7 + fixed
status2.0 --- unaffected
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: jruderman, Assigned: jorendorff)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [sg:critical?][fixed-in-tracemonkey][qa-])

Attachments

(3 files, 1 obsolete file)

Assertion failure: JSVAL_IS_DOUBLE_IMPL(data), at js/src/jsvalue.h:715

TM branch only. I'm guessing it's a TM branch regression.

(Tested using http://hg.mozilla.org/tracemonkey/rev/59325b2ca38b)
Attached file stack trace
Yeah, I see this.  In this case |data| is an object.  In particular, a Function object; the JSFunction looks to be non-interpreted and the native looks like Function(JSContext*, unsigned int, js::Value*).

It looks like in JSObject::allocSlot (where |this| is a Window) we end up in the |if (inDictionaryMode() && lastProp->hasTable()) {| case and |last| ends up 82.  The code then tries to do:

4398    #ifdef DEBUG
4399                JS_ASSERT(last < slot);
4400                uint32 next = getSlot(last).toPrivateUint32();
4401                JS_ASSERT_IF(next != SHAPE_INVALID_SLOT, next < slot);
4402    #endif

but getSlot(last) is storing Function, not an integer.

Is this another symptom of JS_ClearScope biting us?
Hitting this on mozilla-central now that TM has merged to m-c.
This doesn't repro for me on today's nightly or on TM tip (although I thought it did a few days ago). Can you recheck?
I can still reproduce with http://hg.mozilla.org/mozilla-central/rev/19555867d9eb
Jesse showed this to me in gdb. It is crashing calling js_GetMethod on an object that looks like it's been "fake-ClearScope'd", i.e., it has a lot of properties, most of which are undefined.

I can now repro this on 32-bit Win7 loading the test from a local file.
Bisected to:

changeset:   67781:e8a149ad8bcf
user:        Jason Orendorff <jorendorff@mozilla.com>
date:        Thu Apr 14 16:59:26 2011 -0700
summary:     Bug 614714 - Change JS_ClearScope to use the new enumeration code.
r=Waldo.
Depends on: 637099
Taking.
Assignee: general → jorendorff
In the shell:

a = 1;
b = 2;
delete a;
clear();
c = 3;
Correction, clear() without arguments always crashes. That test case is bogus.
Sigh, just wasn't trying hard enough. Real shell test case:

a = 1;
b = 1;
delete a;
c = 1;
delete b;
clear(this);
d = 1;

The bug is in jsscope.cpp, JSObject::removeProperty:

                /*
                 * Maintain slot freelist consistency. The only constraint we
                 * have is that slot numbers on the freelist are less than 
                 * lastProp->slotSpan. Thus, if the freelist is non-empty,
                 * then lastProp->slotSpan may not decrease.
                 */ 
                if (table->freelist != SHAPE_INVALID_SLOT) {
                    lastProp->slotSpan = shape->slotSpan;
                    
                    /* Add the slot to the freelist if it wasn't added in freeSlot. */
                    if (hadSlot && !addedToFreelist) {
                        getSlotRef(shape->slot).setPrivateUint32(table->freelist);
                        table->freelist = shape->slot;
                    }
                }

Actually there is another constraint, which is that reserved slots must not be put on the freelist. Bug 595230 fixed this in JSObject::freeSlot, but we missed the similar code in JSObject::removeProperty. :-\

I have a patch, but it simply copies the fix. I should try commoning up code instead. No time now.
Attached patch WIP 1 - patch that works (obsolete) — Splinter Review
(no test, either)
Without knowing anything "lots of properties that appear undefined" (comment 6) sounds potentially exploitable.
Blocks: 614714
Keywords: regression
Whiteboard: [sg:critical?]
Attached patch v2Splinter Review
It seems like to "common up" we could switch these two chunks of code in JSObject::removeProperty:

>     /* First, if shape is unshared and has a slot, free its slot number. */
>     bool addedToFreelist = false;
>     bool hadSlot = !shape->isAlias() && shape->hasSlot();
>     if (hadSlot) {
>         addedToFreelist = freeSlot(cx, shape->slot);
>         JS_ATOMIC_INCREMENT(&cx->runtime->propertyRemovals);
>     }
> 
>     /* If shape is not the last property added, switch to dictionary mode. */
>     if (shape != lastProp && !inDictionaryMode()) {
>         if (!toDictionaryMode(cx))
>             return false;
>         spp = nativeSearch(shape->id);
>         shape = SHAPE_FETCH(spp);
>     }

Then get rid of addedToFreelist entirely and the chunk of code that tests it.

It looks like it would work, but I don't want to take on another slightly risky change right now. So posting the smaller, more cowardly patch.
Attachment #529228 - Attachment is obsolete: true
Attachment #530051 - Flags: review?(brendan)
Seen this in automation on WinXP with:

http://cabanaomundao.blogspot.com/ | http://www.cabanaomundaogospel.net.br/
http://vuaphim.net/xem-phim (reproducible on xp)
Blocks: 532972
OS: Mac OS X → All
Hardware: x86_64 → All
Comment on attachment 530051 [details] [diff] [review]
v2

Shifting review, hoping to get this in.

dmandelin, please feel free to redirect this to whoever knows this code.
Attachment #530051 - Flags: review?(brendan) → review?(dmandelin)
seems likely related to bug 653561 based on the time-frame in these comments. Jesse, presumably you're tracking both?
robcee, bug 653561 comment 4 is all I've got.
Righto. We'll see if it goes away when Jason lands his fix.
Comment on attachment 530051 [details] [diff] [review]
v2

Stealing review with permission.
Attachment #530051 - Flags: review?(dmandelin) → review?(cdleary)
Comment on attachment 530051 [details] [diff] [review]
v2

Looks right to me. Without reading the comments I also saw the difference from JSObject::freeSlot and thought that the addedToFreeList stuff could go, but I understand your reluctance.
Attachment #530051 - Flags: review?(cdleary) → review+
http://hg.mozilla.org/tracemonkey/rev/4964885e6b9a
test omitted
Flags: in-testsuite-
Whiteboard: [sg:critical?] → [sg:critical?][fixed-in-tracemonkey]
This seems like it's fixed everywhere by now. Marking as such.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
qa-: fix verification unnecessary
Whiteboard: [sg:critical?][fixed-in-tracemonkey] → [sg:critical?][fixed-in-tracemonkey][qa-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.