Closed Bug 713944 Opened 13 years ago Closed 13 years ago

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


(Core :: JavaScript Engine, defect)

Not set





(Reporter: gkw, Assigned: Waldo)



(Keywords: assertion, regression, testcase, Whiteboard: js-triage-done)


(4 files)

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

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(),
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);
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.
I'll take a look.
Assignee: general → jwalden+bmo
fyi, the original assert "Assertion failure: !entry->vindex" is filed as bug 643847 in case that is related.
See Also: → 643847
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.
No, not that.  Still looking.
...wait, there was an assertion before here?  I missed that here.  :-\  Still looking a bit.
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.
OS: Mac OS X → All
Hardware: x86 → All
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.
Attachment #584726 - Flags: review?(jorendorff)
Trivial, nothing to see here, move along...
Attachment #584727 - Flags: review+
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...
Attachment #584728 - Flags: review?(jorendorff)
Whiteboard: js-triage-needed → js-triage-done
Attachment #584726 - Flags: review?(jorendorff) → review+
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 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.
Attachment #584728 - Flags: review?(jorendorff) → review+
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):
Automatically extracted testcase for this bug was committed:
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.