713944 - "Assertion failure: (shape->writable()),"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

function f() {
    constructor = {}
}
Object.defineProperty(function() {
    return {}.__proto__
}(), "constructor", {
    set: function() {}
})
f()
Object.defineProperty({}.__proto__, "constructor", {
    value: 3
})
f()

asserts js debug shell on m-c changeset 7e28cce342a6 without any CLI arguments at Assertion failure: (shape->writable()),

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   82832:4b701b24223f
user:        Bobby Holley
date:        Fri Dec 16 14:32:39 2011 -0800
summary:     Bug 711288 - Unconditionally use the new value of JSPROP_READONLY in accessor -> data transformations. r=Waldo

(This revision changed the assertion that was it, I'm not entirely sure if it is the actual regressor)

 ~/Desktop/autobisect-cache/js-dbg-32-a5c58e9199a3-darwin w1701-reduced.js 
Assertion failure: !entry->vindex

$ ~/Desktop/autobisect-cache/js-dbg-32-4b701b24223f-darwin w1701-reduced.js
Assertion failure: shape->writable(),

Comment 1

[Bobby Holley (:bholley)]

It's hitting the assertion I added. I reduced the testcase a bit:

var accDesc = {set: function() {}};
var dataDesc = {value: 3};

function f() {constructor = {}}
function g() {constructor = {}}
Object.defineProperty({}.__proto__, "constructor", accDesc);
f();
Object.defineProperty({}.__proto__, "constructor", dataDesc);
f(); // g() instead here means that we don't assert!

So it looks like another case of not switching the bits over correctly for accessor -> data property changes. A key thing to note is that it only happens when the self-same function is used in both cases.

My assumption is that this has something to do with the property cache, but I know nothing about it. Does anyone have any ideas? I could dive in, but it might not be the most efficient for me to do so.

Comment 2

[Jeff Walden [:Waldo]]

I'll take a look.

Comment 3

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

fyi, the original assert "Assertion failure: !entry->vindex" is filed as bug 643847 in case that is related.

Comment 4

[Jeff Walden [:Waldo]]

I haven't fully debugged, but I bet it comes down to this, in JSObject::changeProperty:

    if (shape->attrs == attrs && shape->getter() == getter && shape->setter() == setter)
        return shape;

It is *so wrong* that we store attributes as an untyped integer manually used as a bitfield.  Continuing to investigate.

Comment 5

[Jeff Walden [:Waldo]]

No, not that.  Still looking.

Comment 6

[Jeff Walden [:Waldo]]

...wait, there was an assertion before here?  I missed that here.  :-\  Still looking a bit.

Comment 7

[Jeff Walden [:Waldo]]

I still intend to look at this, but I don't understand the property cache as much as I could to do well here.  I'll try writing a series of micro-patches to the property cache to help me work through some of the meaning of things here first, then see where I stand at that point.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Convert PropertyCacheEntry::vindex into two private fields, with some nicely named testing methods

vindex doesn't really tell me anything, and isDirectHit() is sparsely used and not quite fully-clearly named.  Make vindex into two fields instead of one with some manual bit-shifting, change isDirectHit to isOwnPropertyHit, and add isPrototypePropertyHit for the other sense in which vindex was queried.

I'd like to see two different fill methods, one for resolving properties (which omits scopeIndex) and one for resolving identifiers (which would have it).  But that can happen later, and I figured out what was up without having to do that.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Move some declarations closer to first initializations in PropertyCache::fill

Trivial, nothing to see here, move along...

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Don't assert about the cache entry's shape if the cache entry is a miss

So in the end, this and bug 643847 are both (the key parts of both are substantively identical) just misplaced assertions.  What happens is the |constructor = {}| in |f| gets associated with the shape of the object where the lookup starts.  That's the global object.  Then we change Object.prototype.constructor to a data property, and further, one that's not writable.  Via scope chain purging, this changes the lastProperty() of Object.prototype, but it doesn't touch the global object's lastProperty().  The next time around, we pull out the entry using the pc (same) and global lastProperty() (also the same), and decide the entry's suitable for testForSet fast-path purposes.  And it is.  But it's not an own property hit (it's on the global object's prototype), and it's not a prototype property hit (the prototype's lastProperty() changed), so the entry's a miss.  Therefore the shape in it isn't meaningful, and we shouldn't assert stuff about it.

Given that I diverted myself into super-late-night hacking on this because I kept feeling like I almost understood things, I'd say don't expect to see me until the afternoon.  :-)  Zzz...

Comment 11

[Jason Orendorff [:jorendorff]]

Shorter test, not involving Object.prototype, the global object, or __proto__:

var a = {p1: 1, p2: 2}, b = Object.create(a);
Object.defineProperty(a, "p1", {set: function () {}});
for (var i = 0; i < 2; i++) {
    b.p1 = {};
    Object.defineProperty(a, "p1", {value: 3});
}

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 584728 [details] [diff] [review]
Don't assert about the cache entry's shape if the cache entry is a miss

(after lots of gdb-ing)

OK, I see now. :)

If we modify a Shape in place, we invalidate all property cache entries that might point to it. That makes it seem to property cache consumers as though the Shapes in the property cache are immutable. But that illusion is entirely predicated on those consumers only looking at the Shape when there's a valid cache hit. A miss might see an invalidated entry whose Shape has been modified since it was inserted--which is what's happening in this fuzzer test case.

Nice work.

Comment 13

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35124605e1a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/656af9b2d481
https://hg.mozilla.org/integration/mozilla-inbound/rev/a258a0b2d9e4

Comment 14

[Jeff Walden [:Waldo]]

And two more to disable the test and reenable it with some browser-specific fixing (the browser's Window.prototype.constructor shadows Object.prototype.constructor, which breaks it):

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e94dbbde863
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dc6795b4479

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/35124605e1a3
https://hg.mozilla.org/mozilla-central/rev/656af9b2d481
https://hg.mozilla.org/mozilla-central/rev/a258a0b2d9e4
https://hg.mozilla.org/mozilla-central/rev/7e94dbbde863
https://hg.mozilla.org/mozilla-central/rev/6dc6795b4479

Comment 16

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929