597945 - "Assertion failure: getSlot(slot).isUndefined(),"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

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

function b(foo) {
    delete foo.d
    delete foo.w
    foo.d = true
    foo.w = Object
    delete Object.defineProperty(foo, "d", ({
        set: Math.w
    })); {}
}
for each(e in [arguments, arguments]) {
    try {
        b(e)('')
    } catch (e) {}
}

asserts js debug shell on TM changeset 3559d9fded5f without -m nor -j at Assertion failure: getSlot(slot).isUndefined(),

Comment 1

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

autoBisect shows this is probably related to the following changeset:

The 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 2

[Jason Orendorff [:jorendorff]]

To start with, this fails:

  var a = {d:true};
  Object.defineProperty(a, "d", {set: undefined});
  delete a.d;
  assertEq(a.hasOwnProperty("d"), true);

Waldo might want to take a look and see why his megatest didn't catch this.

Comment 3

[Jason Orendorff [:jorendorff]]

And here's the crashing version of that, minimized from comment 0.

var a = {d: true, w: true};
Object.defineProperty(a, "d", {set: undefined});
delete a.d;
delete a.w;
a.d = true;

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #2)
> To start with, this fails:
> 
>   var a = {d:true};
>   Object.defineProperty(a, "d", {set: undefined});
>   delete a.d;
>   assertEq(a.hasOwnProperty("d"), true);
> 
> Waldo might want to take a look and see why his megatest didn't catch this.

Whoa:

js>   Object.defineProperty(a, "d", {set: undefined});
({})
js> Object.getOwnPropertyDescriptor(a, "d")            
({get:(void 0), set:(void 0), enumerable:true, configurable:true})

Why are enumerable and configurable defaulting to true?

Debugging a bit, the |var a = {d:true}| makes an object with enumerable, writable, configurable 'd'. The Object.defineProperty(a, "d", {set:undefined}) call then sets up a PropDesc with attrs SHARED | SETTER | PERMANENT | READONLY, which check out.

But then DefinePropertyOnObject computes a changed mask thus:

2222	        uintN changed = 0;
(gdb) 
2223	        if (desc.hasConfigurable)
(gdb) 
2225	        if (desc.hasEnumerable)
(gdb) 
2227	        if (desc.hasGet)
(gdb) 
2229	        if (desc.hasSet)
(gdb) 
2230	            changed |= JSPROP_SETTER | JSPROP_SHARED;
(gdb) p/x desc.attrs
$7 = 0x66
(gdb) p/x changed
$9 = 0x60
(gdb) n
2232	        attrs = (desc.attrs & changed) | (shape->attributes() & ~changed);
(gdb) n
2233	        if (desc.hasGet) {
(gdb) p/x attrs
$10 = 0x61
We have lost PERMANENT and READONLY, aka not-configurable and not-writable, even though these are the defaults, and the PropDesc's attrs had them right.
Waldo around? This needs a bug, pronto.
/be

Comment 5

[Brendan Eich [:brendan]]

Filed bug 598164 on the Object.defineProperty problem.

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: proposed fix

This is in the way of my fix for bug 595805.

Separating slot allocation and putting it a layer up from shape change looks better all the time.

/be

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #5)
> Filed bug 598164 on the Object.defineProperty problem.

Now RESOLVED INVALID. I was burned by the issue Jeff brought up (too late but it's a good point) on es-discuss: Object.defineProperty is really two APIs in one: createProperty and changeProperty.

/be

Comment 8

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/d7d3c0af2877

/be

Comment 9

[Robert Sayre]

I backed this out because it was failing a 597945-1.js on all platforms. probably something simple, but other stuff needs merging.

REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tracemonkey_fedora-debug_test-jsreftest/build/jsreftest/tests/jsreftest.html?test=js1_8_5/regress/regress-597945-1.js | Unknown file:///home/cltbld/talos-slave/tracemonkey_fedora-debug_test-jsreftest/build/jsreftest/tests/js1_8_5/regress/regress-597945-1.js:16: arguments is not defined item 1

Comment 10

[Brendan Eich [:brendan]]

Sorry about that -- fuzzer generated global arguments usage and I missed it. Just needed a shell-only incantation in jstests.list:

http://hg.mozilla.org/tracemonkey/rev/28428a30999f

/be

Comment 11

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/8deef18e795a

Comment 12

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-597945-1.js.

Comment 13

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

Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.