596805 - Crash [@ JSObject::allocSlot] or "Assertion failure: t->freelist < slotSpan" or "Assertion failure: JSVAL_IS_DOUBLE_IMPL(data),"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

var f = function(){};
for (var y in f);
f.j = 0;
Object.defineProperty(f, "k", ({configurable: true}));
delete f.j;
Object.defineProperty(f, "k", ({get: function() {}}));

Assertion failure: t->freelist < slotSpan, at jsscope.h:364The first bad revision is:
changeset:   53414:b1facf8ba54e
user:        Brendan Eich
date:        Thu Sep 02 14:50:44 2010 -0700
summary:     Eliminate JSObject::freeslot via monotonic lastProp->freeslot (592556, r=jorendorff,dvander).

Comment 1

[Brendan Eich [:brendan]]

js> var f = function(){};

f has empty shape, slotSpan 3

js> for (var y in f);

f has {prototype, length, arity, name, arguments, caller}, slotSpan 9

js> f.j = 0;

f has {..., j}, slotSpan 10, j has slot 9

js> Object.defineProperty(f, "k", ({configurable: true}));

f has {..., j, k}, slotSpan 11, k has slot 10

js> delete f.j;

f has {..., k}, slotSpan 11, lastProp->table->freelist is 9

NB: lastProp->parent->id is 'caller' and lastProp->parent->slotSpan is 9

js> Object.defineProperty(f, "k", ({get: function() {}}));

f has {..., k}, slotSpan 9, k has no slot, assert setting table

The redefinition of k replaces lastProp, temporarily exposing 'caller' with slotSpan 9. The new shape for k has no slot, so its slotSpan is same as its parent's, 9. But we forgot about freelist, and worse, JSObject::putProperty sets slot 9 to undefined in DEBUG builds, since slot 9 is < numSlots().

This is a putProperty bug. I will fix today, should go into b7.

/be

Comment 2

[Brendan Eich [:brendan]]

Worse, and not covered by bug 594899, this shows a slot leak: k-the-first has a slot, k-the-second does not. Fixing the slot leak should help focus the fix for the freelist invariant violation.

/be

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

for each(let c in [0, 0, 0, 0, 0]) {
  try { (function() {
      this.c = this;
      this.e = arguments;
      Object.defineProperty(this, "d", {
        get: Math.pow,
        configurable: true
      });
      delete this.e;
      delete this.c;
      Object.defineProperty(this, "d", {
        writable: true
      });
      if (Math.tan( - 1)) {
        Object.defineProperty(this, {});
      }
    } (c));
  } catch(e) {}
}

crashes js opt shell on TM changeset dfcf5611ce02 at JSObject::allocSlot when passed in as a CLI argument, and asserts at Assertion failure: JSVAL_IS_DOUBLE_IMPL(data), (this assertion was duped here in comment #3)

Turning bug s-s because this seems to be accessing a scary memory address.

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x098139f0
0x000939b8 in JSObject::allocSlot ()
(gdb) bt
#0  0x000939b8 in JSObject::allocSlot ()
#1  0x000f9aa3 in JSObject::getChildProperty ()
Previous frame inner to this frame (gdb could not unwind past this frame)
(gdb) x/i $eip
0x939b8 <_ZN8JSObject9allocSlotEP9JSContextPj+72>:      mov    (%ebx),%eax
(gdb) x/b $ebx
0x98139f0:      Cannot access memory at address 0x98139f0

Comment 5

[Brendan Eich [:brendan]]

Attachment: proposed fix

The jsscope.cpp code can be simplified quite a bit based on ideas in bug 593129, but I need to spot-fix this bug.

/be

Comment 6

[Jason Orendorff [:jorendorff]]

>-        if (lastProp->slotSpan < numSlots())
>+        if (lastProp->slotSpan < numSlots() && !(table && table->freelist == lastProp->slotSpan))
>             getSlotRef(lastProp->slotSpan).setUndefined();

I think a comment like

  /*
   * At this point, we may have just added a slot to table->freelist which
   * is not covered by lastProp->slotSpan. This temporary breaking of the
   * slotSpan invariant is fixed below.
   */

is warranted.

>+            if (shape->slotSpan == table->freelist)
>+                ++shape->slotSpan;

OK, but we could alternatively remove the slot from the freelist, right?

Note that this is not done if getChildProperty() fails. I think that is a bug in the unlikely case that we run out of memory here and survive.

Long-term, can we move the responsibility for slot allocation into these methods instead of having the caller do it and pass a slot parameter?

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 476708 [details] [diff] [review]
proposed fix

What I mean by that comment about adding a comment is that what's happening here is, that an invariant is temporarily relaxed within this function, and must be fixed up before we return. The comment in the patch

+            /*
+             * If shape, the new lastProp in this dictionary-mode object, has a
+             * slotSpan equal to the table freelist, bump shape->slotSpan so it
+             * covers the free slot.
+             */

tells what, but left me wondering why.

r=me with comments addressed.

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #6)
> >-        if (lastProp->slotSpan < numSlots())
> >+        if (lastProp->slotSpan < numSlots() && !(table && table->freelist == lastProp->slotSpan))
> >             getSlotRef(lastProp->slotSpan).setUndefined();
> 
> I think a comment like
> 
>   /*
>    * At this point, we may have just added a slot to table->freelist which
>    * is not covered by lastProp->slotSpan. This temporary breaking of the
>    * slotSpan invariant is fixed below.
>    */
> 
> is warranted.

For sure.

> >+            if (shape->slotSpan == table->freelist)
> >+                ++shape->slotSpan;
> 
> OK, but we could alternatively remove the slot from the freelist, right?

Yeah but strictly more work and more Object/Shape concern mixing AKA layering violation -- see below.

> Note that this is not done if getChildProperty() fails. I think that is a bug
> in the unlikely case that we run out of memory here and survive.

It is a bug, again see below.

> Long-term, can we move the responsibility for slot allocation into these
> methods instead of having the caller do it and pass a slot parameter?

Not into these methods -- slot allocation should move back to jsobj.cpp and JSObject::* methods, these jsscope.cpp (jsshape.cpp, Shape.cpp) methods should be Shape::* methods, which do not allocate slots from objects, or free slots back to objects.

Then we can have Object-centric code do something like:

    if (Shape *shape = parent->getChild(...)) {
        if (shape->hasSlot())
            allocSlot(shape->slot);
    }

(sketching, ignoring errors), and:

    freeSlot(shape->slot);
    removeShape(shape);

(latter only in js_DeleteProperty, which should be JSObject::deleteProperty with a bit of work). See bug 593129.

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: proposed fix

Jason, the assertion we discussed on IRC,

    JS_ASSERT_IF(inDictionaryMode(), !lastProp->frozen());

at the top of putProperty smoked out the (in hindsight obvious) offenders who can extend Call objects, what with their shared frozen dictionary shapes (created for fun->u.i.names if non-empty-pattern destructuring params or overlong parameter lists are used). The same assertion in changeProperty has yet to botch and should not or something is up.

So now putProperty compensates using newDictionaryList and addPropertyInternal (note new name and comment in jsobj.h) asserts.

The three-way structure of putProperty is much better. Mutable dictionary shapes FTW!

Only thing left is bug 593129 cleanup (a shallow Shape class hierarchy, moving slot alloc/free upstairs to jsobj.cpp, Shape not JSObject methods, etc.).

/be

Comment 10

[Jason Orendorff [:jorendorff]]

Preliminary comments. I have an appointment this morning, so I can't finish the
review right now.

I like the new structure of putProperty a lot better.

In jsscope.cpp, JSObject::putProperty:
>+            setOwnShape(shape->shape);

Doesn't this conflict with what it says in checkShapeConsistency?

    if (hasOwnShape())
        JS_ASSERT(objShape != lastProp->shape);
    else
        JS_ASSERT(objShape == lastProp->shape);

>+    /*
>+     * Second case: reordering shapes by overwriting a non-last shape. Let's
>+     * use removeShape and addPropertyInternal to keep things simple and avoid
>+     * temporarily violating invariants.
>+     */
>+    if (shape != lastProp) {
>+        if (!removeShape(cx, hadSlot, shape, spp))
>+            return NULL;
>[...]
>+        spp = nativeSearch(id, true);
>+        return addPropertyInternal(cx, id, getter, setter, slot, attrs, flags, shortid, spp);
>+    }

On OOM, this would have the effect of removing a property, even a
non-configurable one (being changed from writeable to non-writeable, which
Object.defineProperty at least can do).

The same could happen in the final case.

> bool
>-JSObject::removeProperty(JSContext *cx, jsid id)
>+JSObject::removeShape(JSContext *cx, bool hadSlot, Shape *shape, Shape **spp)

hadSlot is only used for an assertion, and it could be recalculated there.

In JSObject::removeProperty:
>+    /* First, if shape is unshared and not cleared, free its slot number. */

The word "First" in this comment was meant to go with the word "Next" in the
following comment, but now they have been factored into separate methods.

What does "cleared" mean here?

>+    bool hadSlot = !shape->isAlias() && containsSlot(shape->slot);
>+    if (hadSlot) {

How is containsSlot(shape->slot) different from shape->hasSlot() here?

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> In jsscope.cpp, JSObject::putProperty:
> >+            setOwnShape(shape->shape);
> 
> Doesn't this conflict with what it says in checkShapeConsistency?
> 
>     if (hasOwnShape())
>         JS_ASSERT(objShape != lastProp->shape);
>     else
>         JS_ASSERT(objShape == lastProp->shape);

Yes, indeed -- no need for OWN_SHAPE here. Fixed.

> On OOM, this would have the effect of removing a property, ...
> 
> The same could happen in the final case.

The old jsscope.cpp code was careful to restore on failed overwrite, at most reordering for-in enumeration order. Fixed, and no more for-in enumeration order change on OOM.

> >+JSObject::removeShape(JSContext *cx, bool hadSlot, Shape *shape, Shape **spp)
> 
> hadSlot is only used for an assertion, and it could be recalculated there.

removeShape is gone in the next patch I'll attach.

> In JSObject::removeProperty:
> >+    /* First, if shape is unshared and not cleared, free its slot number. */
. . .
> What does "cleared" mean here?

This dates from when JS_ClearScope could zap slots still mapped by properties in use (js_NativeGet calling a getter, e.g., or a setter including a watchpoint setter-wrapper). That should not be possible any longer thanks to the runtime's propertyRemovals generation number. So I changed all containsSlot(shape->slot) calls to shape->hasSlot().

Thanks for the comments, very helpful. On a plane, will test and attach a v2 patch after test victory.

/be

Comment 12

[Brendan Eich [:brendan]]

Attachment: proposed fix, v2

Comment 13

[Brendan Eich [:brendan]]

Attachment: proposed fix, v2 (comment fixes and tweaks)

Plane lacks power for everyone, I ran my batt down without noticing, so pushed to try and attached here to optimize. Back later (or sooner if power returns).

/be

Comment 14

[Jason Orendorff [:jorendorff]]

Comment on attachment 477530 [details] [diff] [review]
proposed fix, v2 (comment fixes and tweaks)

I can't find anything wrong with this one.

Comment 15

[Brendan Eich [:brendan]]

Attachment: consolidate error reporting, provide generating-expression blame, shorter names

Comment 16

[Brendan Eich [:brendan]]

Also, "cannot" had 4 hits in js.msg to many more for "can't", so I normalized. Also^2, no messages save two new ones put single quotes around decompiled property expressions, which just adds noise and potential confusion, so I got rid of those.

/be

Comment 17

[Brendan Eich [:brendan]]

Attachment: consolidate and improve error reporting, with test mods to match

Comment 18

[Jason Orendorff [:jorendorff]]

Comment on attachment 477990 [details] [diff] [review]
consolidate and improve error reporting, with test mods to match

The CANT_APPEND message starts with a capital letter, unlike all the others. Fix that while you're in js.msg?

Please keep the quotes in JSMSG_CANT_REDEFINE_PROP. At least in some cases (maybe all), the argument isn't a property expression but just an id:

  js> var obj = Object.create(null, {name: {value: 'k'}});
  js> Object.defineProperty(obj, "name", {value: 'j'});
  typein:4: TypeError: can't redefine non-configurable property 'name'

Without quotes this would be really confusing.

However removing the quotes in JSMSG_CANT_DELETE is the right thing.

Other than that, this looks good. "Cannot" is computerese. Good riddance.

Comment 19

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/7ef107ab081e

/be

Comment 20

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/7ef107ab081e