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

RESOLVED FIXED

Status

()

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

People

(Reporter: decoder, Unassigned)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

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

Firefox Tracking Flags

(blocking2.0 .x+)

Details

(Whiteboard: js-triage-done)

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
Created attachment 515511 [details]
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).
(autoBisect does not yet support -f <file> I think..)

Not the smallest regression window:

asserts in 8323a963fd6c
does not assert in 8c52a9486c8f
Blocks: 630996
Keywords: regression, regressionwindow-wanted
Hardware: x86_64 → All
Created attachment 515636 [details]
Reduced a bit

Reduced to a single file to make it bisectable...
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"
Blocks: 558451
blocking2.0: --- → ?
Keywords: regressionwindow-wanted
blocking2.0: ? → .x+
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
Created attachment 516075 [details] [diff] [review]
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
Attachment #516075 - Flags: feedback?(jorendorff)
My kind of bug. I'll minimize first, look at the patch second.
Assignee: general → 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();
(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.
(Reporter)

Comment 9

6 years ago
(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 =)
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).
OS: Linux → All
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.
Attachment #516075 - Flags: feedback?(jorendorff) → feedback+
(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
This needs an owner, but it's not me.
Assignee: jorendorff → general
(Reporter)

Updated

6 years ago
Blocks: 676763
(Reporter)

Comment 14

6 years ago
Confirmed that the test in comment 2 still reproduces on m-c tip (tested revision 7ed661aa832d).
Whiteboard: js-triage-done
See Also: → bug 643847
(Reporter)

Comment 15

5 years ago
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?
(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?
Flags: in-testsuite?
It's possible that cset fixed this bug, it made a lot of property cache modifications and removed a lot of complexity.
(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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.