637202 - Assertion failure: entry->vcapTag() == 0, at jsinterp.cpp:4381

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: Shell testcase, unpack and run js -j -f part1.js -f part2.js -f part3.js

The attached testcase (shell testcase, unpack and run js -j -f part1.js -f part2.js -f part3.js) causes the assertion on TM tip. Tested also with opt build and stepping through assertion, but no sign of crash or memory corruption.

Found through combined fuzzing (jandem's method fuzzer + LangFuzz).

Comment 1

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

(autoBisect does not yet support -f <file> I think..)

Not the smallest regression window:

asserts in 8323a963fd6c
does not assert in 8c52a9486c8f

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Reduced a bit

Reduced to a single file to make it bisectable...

Comment 3

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

Thanks, jandem, that helps. It's probably bug 558451 that caused this..

Nominating for .x+.

autoBisect shows that due to skipped revisions, the first bad revision could be any of:

changeset:   52503:e5958cd4a135
user:        Brendan Eich
date:        Sun Aug 29 11:57:08 2010 -0700
summary:     Merge JSScope into JSObject and JSScopeProperty (now js::Shape; bug 558451, r=jorendorff).

changeset:   52504:0d5d2ceb9436
user:        Brendan Eich
date:        Sun Aug 29 12:41:24 2010 -0700
summary:     Back out patch for 477999 due to assertbotch on Mac OS X trace-test shutdown, all tests.

changeset:   52505:e6496cd735a6
user:        Brendan Eich
date:        Sun Aug 29 13:07:12 2010 -0700
summary:     Followup fix to my hand-merge botch backing out patch for bug 477999.

changeset:   52506:f3e58c264932
user:        Igor Bukanov
date:        Sun Aug 29 23:24:23 2010 +0200
summary:     bug 558451 - followup to fix GCC warnings and link error. "CLOSED TREE"

Comment 4

[Brendan Eich [:brendan]]

Updating to the rev before the patch series for bug 558451 first landed, 5685f8de41fa, and running the reduced test shows no assertbotch, but debugging shows that's because of a latent bug: on the 9th call to f10b, passing o3 (aka Number.prototype), with p1 a shared setter shape on Object.prototype, the code in f10b to set p1:

    with(o) {
        p1 = o4;
    }

consisting of bindname<p1>; name<o4>; setname<p1>, wrongly hits the property cache on the global object (bindname skips testing the property cache for the global object and before it, the With and Call objects on the scope chain, pushing the global).

We should never hit the property cache inside 'with' statement.

A while ago we actually disabled the cache inside with's body, but the patch for bug 487039 removed this in favor of whitelisting the cacheable non-global scope classes. That's not enough, as this bug's test shows.

The hit means the setter for p1 is not run, instead a global data property is created.

So the fix for bug 558451 simply fixed a latent bug that allowed this test to pass up until that fix landed.

More in a bit.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: brute-force fix for feedback

Jason, you should feel free to take this bug. I was interested in it because we aren't supposed to cache under 'with'. A few mysteries remain:

* Why is -j required?
* What is the real regressor (may be a very old bug)?
* What's the minimal testcase?

Igor, your feedback is welcome too.

/be

Comment 6

[Jason Orendorff [:jorendorff]]

My kind of bug. I'll minimize first, look at the patch second.

Comment 7

[Jason Orendorff [:jorendorff]]

This test case asserts the same way, but with or without -j.

I still don't know why the original test case requires -j.

var x = Object.create(null);
function f() {
    with (x)
        b = 0;
}
var op = Object.prototype;
delete op.toLocaleString;  // force dictionary mode
Object.defineProperty(op, 'b', {configurable: true, set: function (v) {}});
f();
Object.defineProperty(op, 'b', {configurable: true, writable: true, value: 0});
f();

Comment 8

[Jason Orendorff [:jorendorff]]

(In reply to comment #7)
> I still don't know why the original test case requires -j.

Oh, it doesn't. The original test case asserts the same with or without -j.

Comment 9

[Christian Holler (:decoder)]

(In reply to comment #8)

> Oh, it doesn't. The original test case asserts the same with or without -j.

My fault, I forgot to verify that the -j here is really required, which I usually do =)

Comment 10

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

autoBisect seems to point at another regressor, bug 596805, using the testcase in comment 7:

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   54300:7ef107ab081e
user:        Brendan Eich
date:        Thu Sep 16 11:56:54 2010 -0700
summary:     Fix shape vs. slot management under putProperty, plus related layering and error reporting fixes (596805, r=jorendorff).

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 516075 [details] [diff] [review]
brute-force fix for feedback

This seems reasonable.

However, back when this worked correctly, we just incremented a context-wide "disablePropertyCache" counter when we entered a with-scope, right? That is, the disabling was dynamically scoped rather than statically scoped. If the implementation's going to be a runtime check either way, dynamic scoping would be faster when there is no with-block.

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #11)
> However, back when this worked correctly, we just incremented a context-wide
> "disablePropertyCache" counter when we entered a with-scope, right? That is,
> the disabling was dynamically scoped rather than statically scoped. If the
> implementation's going to be a runtime check either way, dynamic scoping would
> be faster when there is no with-block.

Right. Igor, any reason not to revive the cache disabling within 'with' bodies? Did we have counter imbalance bugs?

/be

Comment 13

[Jason Orendorff [:jorendorff]]

This needs an owner, but it's not me.

Comment 14

[Christian Holler (:decoder)]

Confirmed that the test in comment 2 still reproduces on m-c tip (tested revision 7ed661aa832d).

Comment 15

[Christian Holler (:decoder)]

I cannot reproduce with the test in comment 7 anymore. Bisect says

The first good revision is:
changeset:   81234:ff51ddfdf5d1
parent:      77569:44ef245b8706
user:        Brian Hackett
date:        Wed Sep 28 15:04:55 2011 -0700
summary:     Remove shape numbers and Shape::slotSpan, factor Shape getter/setter into BaseShape, bug 684505.

Is this related or did it just break the test?

Comment 16

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

(In reply to Christian Holler (:decoder) from comment #15)
> I cannot reproduce with the test in comment 7 anymore. Bisect says
> 
> The first good revision is:
> changeset:   81234:ff51ddfdf5d1
> parent:      77569:44ef245b8706
> user:        Brian Hackett
> date:        Wed Sep 28 15:04:55 2011 -0700
> summary:     Remove shape numbers and Shape::slotSpan, factor Shape
> getter/setter into BaseShape, bug 684505.
> 
> Is this related or did it just break the test?

Brian, thoughts?

Comment 17

[Brian Hackett [Laid off!]]

It's possible that cset fixed this bug, it made a lot of property cache modifications and removed a lot of complexity.

Comment 18

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

(In reply to Brian Hackett (:bhackett) from comment #17)
> It's possible that cset fixed this bug, it made a lot of property cache
> modifications and removed a lot of complexity.

-> FIXED by bug 684505