Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 713944 - "Assertion failure: (shape->writable()),"
: "Assertion failure: (shape->writable()),"
: assertion, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla12
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 630996 711288
  Show dependency treegraph
Reported: 2011-12-28 13:50 PST by Gary Kwong [:gkw] [:nth10sd]
Modified: 2013-01-19 14:03 PST (History)
7 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

stack (2.57 KB, text/plain)
2011-12-28 13:50 PST, Gary Kwong [:gkw] [:nth10sd]
no flags Details
Convert PropertyCacheEntry::vindex into two private fields, with some nicely named testing methods (9.10 KB, patch)
2011-12-29 03:42 PST, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Splinter Review
Move some declarations closer to first initializations in PropertyCache::fill (1.60 KB, patch)
2011-12-29 03:44 PST, Jeff Walden [:Waldo] (remove +bmo to email)
jwalden+bmo: review+
Details | Diff | Splinter Review
Don't assert about the cache entry's shape if the cache entry is a miss (2.77 KB, patch)
2011-12-29 03:54 PST, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Splinter Review

Description Gary Kwong [:gkw] [:nth10sd] 2011-12-28 13:50:07 PST
Created attachment 584634 [details]

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(),
Comment 1 Bobby Holley (:bholley) (busy with Stylo) 2011-12-28 17:03:22 PST
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.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-28 17:27:42 PST
I'll take a look.
Comment 3 Gary Kwong [:gkw] [:nth10sd] 2011-12-28 17:30:57 PST
fyi, the original assert "Assertion failure: !entry->vindex" is filed as bug 643847 in case that is related.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-28 17:42:54 PST
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] (remove +bmo to email) 2011-12-28 18:24:23 PST
No, not that.  Still looking.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-28 20:40:26 PST
...wait, there was an assertion before here?  I missed that here.  :-\  Still looking a bit.
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-29 00:26:10 PST
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] (remove +bmo to email) 2011-12-29 03:42:56 PST
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.
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-29 03:44:54 PST
Created attachment 584727 [details] [diff] [review]
Move some declarations closer to first initializations in PropertyCache::fill

Trivial, nothing to see here, move along...
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-29 03:54:57 PST
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...
Comment 11 Jason Orendorff [:jorendorff] 2011-12-30 08:15:36 PST
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] 2011-12-30 08:58:05 PST
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 14 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-30 19:45:24 PST
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):
Comment 16 Christian Holler (:decoder) 2013-01-19 14:03:24 PST
Automatically extracted testcase for this bug was committed:

Note You need to log in before you can comment on or make changes to this bug.