651030 - "Assertion failure: JSVAL_IS_DOUBLE_IMPL(data)"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

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)

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Boris Zbarsky [:bzbarsky]]

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?

Comment 3

[Jesse Ruderman]

Hitting this on mozilla-central now that TM has merged to m-c.

Comment 4

[David Mandelin [:dmandelin]]

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?

Comment 5

[Jesse Ruderman]

I can still reproduce with http://hg.mozilla.org/mozilla-central/rev/19555867d9eb

Comment 6

[David Mandelin [:dmandelin]]

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.

Comment 7

[David Mandelin [:dmandelin]]

Bisected to:

changeset:   67781:e8a149ad8bcf
user:        Jason Orendorff <[e-mail]>
date:        Thu Apr 14 16:59:26 2011 -0700
summary:     Bug 614714 - Change JS_ClearScope to use the new enumeration code.
r=Waldo.

Comment 8

[Jason Orendorff [:jorendorff]]

Taking.

Comment 9

[Jason Orendorff [:jorendorff]]

In the shell:

a = 1;
b = 2;
delete a;
clear();
c = 3;

Comment 10

[Jason Orendorff [:jorendorff]]

Correction, clear() without arguments always crashes. That test case is bogus.

Comment 11

[Jason Orendorff [:jorendorff]]

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.

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: WIP 1 - patch that works

(no test, either)

Comment 13

[Daniel Veditz [:dveditz]]

Without knowing anything "lots of properties that appear undefined" (comment 6) sounds potentially exploitable.

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: v2

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.

Comment 15

[Bob Clary [:bc] (inactive)]

Seen this in automation on WinXP with:

http://cabanaomundao.blogspot.com/ | http://www.cabanaomundaogospel.net.br/
http://vuaphim.net/xem-phim (reproducible on xp)

Comment 16

[Jason Orendorff [:jorendorff]]

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.

Comment 17

[Rob Campbell [:rc] (:robcee)]

seems likely related to bug 653561 based on the time-frame in these comments. Jesse, presumably you're tracking both?

Comment 18

[Jesse Ruderman]

robcee, bug 653561 comment 4 is all I've got.

Comment 19

[Rob Campbell [:rc] (:robcee)]

Righto. We'll see if it goes away when Jason lands his fix.

Comment 20

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 530051 [details] [diff] [review]
v2

Stealing review with permission.

Comment 21

[Chris Leary [:cdleary] (not checking bugmail)]

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.

Comment 22

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/4964885e6b9a
test omitted

Comment 23

[Johnny Stenback (:jst)]

This seems like it's fixed everywhere by now. Marking as such.

Comment 24

[u279076]

qa-: fix verification unnecessary