1115844 - Assertion failure: !cur_->is<ScopeObject>() || (cur_->is<DynamicWithObject>() && !cur_->as<DynamicWithObject>().isSyntactic()), at vm/ScopeObject.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

function f() {
    let(x)
    yield x = Proxy.createFunction(
        (function(){}), (function() {})
    );
}
for (d in f()) {}

asserts js debug shell on m-c changeset 54e902f5e85d with --fuzzing-safe --no-threads --ion-eager at Assertion failure: !cur_->is<ScopeObject>() || (cur_->is<DynamicWithObject>() && !cur_->as<DynamicWithObject>().isSyntactic()), at vm/ScopeObject.cpp.

Debug configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/2e24211fa51c
user:        Tom Schuster
date:        Wed Dec 17 00:28:38 2014 +0100
summary:     Bug 783829 - Change from Proxy iterate to enumerate. r=efaust,bholley

Tom, is bug 783829 a likely regressor?

Comment 1

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

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0x153b12, 0x00000001006d3af3 js-dbg-opt-64-dm-nsprBuild-darwin-54e902f5e85d`js::ScopeIter::settle(this=<unavailable>) + 3171 at ScopeObject.cpp:1215, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001006d3af3 js-dbg-opt-64-dm-nsprBuild-darwin-54e902f5e85d`js::ScopeIter::settle(this=<unavailable>) + 3171 at ScopeObject.cpp:1215
    frame #1: 0x00000001006d2dfc js-dbg-opt-64-dm-nsprBuild-darwin-54e902f5e85d`js::ScopeIter::ScopeIter(this=0x00007fff5fbfd160, frame=(ptr_ = 140734799796722), pc=<unavailable>, cx=<unavailable>, _notifier=0x00007fff5fbfd150) + 428 at ScopeObject.cpp:1100
    frame #2: 0x000000010042c390 js-dbg-opt-64-dm-nsprBuild-darwin-54e902f5e85d`js::jit::DebugEpilogue(cx=0x0000000101f00e70, frame=0x00007fff5fbfd9f0, pc=0x0000000101f02e5a, ok=true) + 128 at VMFunctions.cpp:787
    frame #3: 0x000000010034bbde js-dbg-opt-64-dm-nsprBuild-darwin-54e902f5e85d`js::jit::HandleClosingGeneratorReturn(JSContext*, js::jit::JitFrameIterator const&, unsigned char*, unsigned char*, js::jit::ResumeFromException*, bool*) [inlined] js::jit::ForcedReturn(cx=0x0000000101f00e70, pc=0x0000000101f02e5a, rfe=<unavailable>) + 50 at JitFrames.cpp:515
    frame #4: 0x000000010034bbac js-dbg-opt-64-dm-nsprBuild-darwin-54e902f5e85d`js::jit::HandleClosingGeneratorReturn(cx=0x0000000101f00e70, frame=0x00007fff5fbfd6c0, pc=<unavailable>, unwoundScopeToPc=<unavailable>, rfe=0x00007fff5fbfd970, calledDebugEpilogue=0x00007fff5fbfd31e) + 316 at JitFrames.cpp:552
(lldb)

Comment 2

[Tom S]

I can't actually trigger this bug in a normal shell, we just throw an exception. I think the same thing happens here. We try to throw an exception, because of the new code that I introduced, we run hasProperty("done") on the result of the yield. This throws, because the Proxy is missing the right trap. So we begin unwinding and trying to close the generator object. This seems more of a general, generator closing issue.

Comment 3

[Jan de Mooij [:jandem]]

Tom is right, here's a simpler testcase:

function f() {
    let(x) yield 3;
}
var g = f();
g.next();
g.close();

Comment 4

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/4dffd706f975
user:        Shu-yu Guo
date:        Wed Nov 26 13:35:57 2014 -0800
summary:     Bug 1100337 - Fix baseline PC mapping for bytecodes that generate no native code. (r=djvj)

This is the new bisection result with the testcase in comment 3.

Shu-yu, is bug 1100337 a likely regressor?

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Patch

For "let(x) yield 3;" we emit:

00031:  yield 1
00035:  debugafteryield
00036:  debugleaveblock
00037:  popblockscope

When we close() the generator, we end up in HandleException. There we try to get the current pc but instead of JSOP_DEBUGAFTERYIELD we get JSOP_POPBLOCKSCOPE, because no code is emitted for the JSOP_DEBUG* ops.

JSScript::getStaticScope then returns nullptr, because the static scope ends right before the JSOP_POPBLOCKSCOPE, and this confuses ScopeIter::settle of course.

This patch emits a nop for JSOP_DEBUGAFTERYIELD in the non-debug case and adds an assert. Shu, let me know if you have a better idea.

We should probably also think about ways to improve Baseline's PCMappingEntry / ICEntry stuff. I think initially only ICEntry was used to recover the bytecode pc, later this was changed to also consider PCMappingEntry; it seems like we should be able to simplify this.

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8542145 [details] [diff] [review]
Patch

Review of attachment 8542145 [details] [diff] [review]:
-----------------------------------------------------------------

I believe this is the right short-term fix.

Comment 7

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #5)

> We should probably also think about ways to improve Baseline's
> PCMappingEntry / ICEntry stuff. I think initially only ICEntry was used to
> recover the bytecode pc, later this was changed to also consider
> PCMappingEntry; it seems like we should be able to simplify this.

The native code <-> pc mappings have been the thorns in my side for some months. A great many debug mode OSR bugs have been due to:

  1. The native code -> pc mapping isn't even a function! The same native code address could be both the return-from-IC address for some pc and the start-of-code address for pc+1. Which pc you want depends entirely on context: have we yet conceptually started executing the next pc yet?
  2. Opcodes we generate no JIT code for, like this bug.
  3. ICs and VM calls generated in the prologue and epilogue are indistinguishable from code generated for pc offset 0 or the last offset.

I think the problem of determining context is the ultimate root of a lot of these bugs. 2 conceptually shouldn't even be an issue, because why do we care about distinguishing native code address ranges for opcodes that can't even re-enter the VM?

If we're asking for the pc inside HandleException, we have not yet conceptually started executing the next opcode, and so the native address should be mapped as the returning-to address, even though it's the same physical address as the starting address of the next pc.

Maybe can codify these 2 contexts in code better, to force the callers to think through what they want?

Comment 8

[Shu-yu Guo [:shu]]

Typo: an extra 'yet' in 1.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Shu-yu Guo [:shu] from comment #7)
> I think the problem of determining context is the ultimate root of a lot of
> these bugs. 2 conceptually shouldn't even be an issue, because why do we
> care about distinguishing native code address ranges for opcodes that can't
> even re-enter the VM?

Yeah, agreed. Initially the bytecode <-> native map was only used for bailouts (to get the native code address for a given pc) or to toggle debug mode breakpoints. We didn't do the reverse (native -> pc). I think it'd be really nice if we could get back to that... Do you want to give it a shot? Else I can experiment a bit with this to see what the constraints are exactly...

Comment 10

[Kannan Vijayan [:djvj]]

(In reply to Shu-yu Guo [:shu] from comment #7)
> Maybe can codify these 2 contexts in code better, to force the callers to
> think through what they want?

That comment about context is a very astute point about the issue with native=>bytecode mapping.  I think it'd be extremely helpful to have the two different lookups have different names and force callers to think about what address they are trying to map and why.

Comment 11

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4a53398ac44

Again, we can back this out when bug 1118826 lands.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d4a53398ac44

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8542145 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1100337.
[User impact if declined]: Crashes.
[Describe test coverage new/current, TBPL]: Fixes the test.
[Risks and why]: Low risk, very simple and small patch.
[String/UUID change made/needed]: None.

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

Marking 36 as affected given that bug 1100337 was fixed in 36. Do we need this on beta?

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8542145 [details] [diff] [review]
Patch

Aurora+

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 8542145 [details] [diff] [review]
Patch

Yes we should land this on beta too.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8542145 [details] [diff] [review]
Patch

This patch landed in initially in 37, no need to uplift to aurora (but we need it for beta, 36)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/54a53a093110