Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: gkw, Assigned: Waldo)

Tracking

({assertion, regression, testcase})

Trunk
mozilla12
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: js-triage-done)

Attachments

(4 attachments)

(Reporter)

Description

6 years ago
Created attachment 584634 [details]
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(),
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.
I'll take a look.
Assignee: general → jwalden+bmo
(Reporter)

Comment 3

6 years ago
fyi, the original assert "Assertion failure: !entry->vindex" is filed as bug 643847 in case that is related.
See Also: → bug 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
Created attachment 584726 [details] [diff] [review]
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.
Attachment #584726 - Flags: review?(jorendorff)
Created attachment 584727 [details] [diff] [review]
Move some declarations closer to first initializations in PropertyCache::fill

Trivial, nothing to see here, move along...
Attachment #584727 - Flags: review+
Created attachment 584728 [details] [diff] [review]
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...
Attachment #584728 - Flags: review?(jorendorff)
Status: NEW → ASSIGNED
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+
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
Target Milestone: --- → mozilla12
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
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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.