1269711 - Crash [@ is<js::StaticBlockScope>] or Assertion failure: !done(), at vm/ScopeObject.cpp:1523

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 77cead2cd203 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug, run with --fuzzing-safe):

setInterruptCallback(function() frame)
setJitCompilerOption("ion.warmup.trigger", 50)
function entry_menu() {
    dialog = 0;
    with(dialog) try {
        return;
    } finally {
        interruptIf(5);
    }
}
entry_menu();



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
is<js::StaticBlockScope> (this=0x0) at js/src/vm/ScopeObject.h:1390
#0  is<js::StaticBlockScope> (this=0x0) at js/src/vm/ScopeObject.h:1390
#1  type (this=this@entry=0x7fffffffd350) at js/src/vm/ScopeObject-inl.h:152
#2  js::ScopeIter::type (this=this@entry=0x7fffffffd350) at js/src/vm/ScopeObject.cpp:1525
#3  0x0000000000880fee in PopScope (si=..., cx=0x7ffff6907800) at js/src/vm/Interpreter.cpp:977
#4  js::UnwindScope (cx=0x7ffff6907800, si=..., pc=<optimized out>) at js/src/vm/Interpreter.cpp:1005
#5  0x0000000000881163 in SettleOnTryNote (cx=<optimized out>, tn=0x7ffff31fc438, si=..., regs=...) at js/src/vm/Interpreter.cpp:1063
#6  0x0000000000892601 in ProcessTryNotes (regs=..., si=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:1126
#7  HandleError (regs=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:1214
#8  Interpret (cx=0x7ffff6907800, state=...) at js/src/vm/Interpreter.cpp:3996
[...]
#18 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:7483
rax	0x0	0
rbx	0x7fffffffd350	140737488343888
rcx	0x18690a0	25596064
rdx	0x7ffff7e690a0	140737352470688
rsi	0x0	0
rdi	0x7fffffffd350	140737488343888
rbp	0x7ffff6907800	140737330051072
rsp	0x7fffffffc9b8	140737488341432
r8	0x1	1
r9	0x0	0
r10	0x1	1
r11	0x0	0
r12	0x7fffffffffff	140737488355327
r13	0xfff9800000000000	-1829587348619264
r14	0x2	2
r15	0x7ffff31fc438	140737272333368
rip	0x8d68cb <js::ScopeIter::type() const+43>
=> 0x8d68cb <js::ScopeIter::type() const+43>:	mov    (%rax),%rax
   0x8d68ce <js::ScopeIter::type() const+46>:	mov    (%rax),%rdx

Comment 1

[Liz Henry (:lizzard) (relman/hg->git project)]

Noting that we don't need to track this. We should probably remove fuzzing/assertion issues from the release health dashboard.

Comment 2

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

This seems to go back before m-c rev dc4b163f7db7, early Nov 2014. Setting needinfo? from Jan as a fallback.

Comment 3

[Jan de Mooij [:jandem]]

We have the following with-statement:

    with (dialog) {
	try {
            return;
	} finally {
            interruptIf(5);
	}
    }

For the return inside the try block, we emit this:

  00030:  undefined
  00031:  setrval
  00032:  gosub 52 (+20)
  00037:  jumptarget
  00038:  leavewith
  00039:  retrval

The RETRVAL at offset 39 is where we trigger the interrupt callback and it throws an exception. So far so good.

Then in SettleOnTryNote, we do:

  UnwindScope(cx, si, UnwindScopeToTryPc(regs.fp()->script(), tn));

UnwindScopeToTryPc returns the pc at offset 29 (JSOP_TRY). That makes us go off the rails, because our static scope array contains this:

  scope 0: index 0, start 29, length 50, parent UINT32_MAX
  scope 1: index NoBlockScopeIndex, start 39, length 1, parent 0

So at offset 39 we are already *past* the LEAVEWITH and don't have a With scope object on the scope chain, but because we pretend our scope chain is the same as at offset 29 (JSOP_TRY), we do expect a With object on the scope chain and we crash.

Shu, any thoughts on this?

Marking this s-s just to be sure.

Comment 4

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> We have the following with-statement:
> 
>     with (dialog) {
> 	try {
>             return;
> 	} finally {
>             interruptIf(5);
> 	}
>     }
> 
> For the return inside the try block, we emit this:
> 
>   00030:  undefined
>   00031:  setrval
>   00032:  gosub 52 (+20)
>   00037:  jumptarget
>   00038:  leavewith
>   00039:  retrval
> 
> The RETRVAL at offset 39 is where we trigger the interrupt callback and it
> throws an exception. So far so good.
> 
> Then in SettleOnTryNote, we do:
> 
>   UnwindScope(cx, si, UnwindScopeToTryPc(regs.fp()->script(), tn));
> 
> UnwindScopeToTryPc returns the pc at offset 29 (JSOP_TRY). That makes us go
> off the rails, because our static scope array contains this:
> 
>   scope 0: index 0, start 29, length 50, parent UINT32_MAX
>   scope 1: index NoBlockScopeIndex, start 39, length 1, parent 0
> 
> So at offset 39 we are already *past* the LEAVEWITH and don't have a With
> scope object on the scope chain, but because we pretend our scope chain is
> the same as at offset 29 (JSOP_TRY), we do expect a With object on the scope
> chain and we crash.
> 
> Shu, any thoughts on this?
> 
> Marking this s-s just to be sure.

This is pretty annoying and I'm not sure what the right fix here. Scope unwinding for try will always fail when JSOP_RETRVAL itself throws, because that'll always be the last op in the bytecode.

I will ruminate.

Comment 5

[Shu-yu Guo [:shu]]

Actually, it's not just that scope unwinding will fail, if there was a catch block the scope chain itself will actually be wrong!

Because RETRVAL is the last op, LEAVEWITH popped the actual scope chain so if there were a catch block, we wouldn't execute it inside the with.

Comment 6

[Shu-yu Guo [:shu]]

My current line of thinking is JSOP_RETRVAL should not call CHECK_BRANCH() in the interpreter. Return should *not* be able to throw.

Comment 7

[Shu-yu Guo [:shu]]

(In reply to Shu-yu Guo [:shu] from comment #6)
> My current line of thinking is JSOP_RETRVAL should not call CHECK_BRANCH()
> in the interpreter. Return should *not* be able to throw.

Ah so actually |return| usually *doesn't* throw, because a false return from the interrupt callback usually means termination (uncatchable error).

It's throwing in the test because the ShellInterruptCallback as implemented by the js shell basically runs re-entrant JS code on the same cx, causing this super wonky behavior.

There is an AutoSaveExceptionState in ShellInterruptCallback but it is not used to restore the state. Any exceptions thrown by the JS interrupt func should be independently reported (like unhandled exceptions in Debugger) and cx's original exception state should be restored on exit.

Comment 8

[Shu-yu Guo [:shu]]

Attachment: bug1269711.patch

Not sure if it's worth to have a test case for this, but will be happy to attach the fuzz test.

Comment 9

[Shu-yu Guo [:shu]]

Opening the bug since this issue is isolated to the JS shell and with testing functions only.

Comment 10

[Jim Blandy :jimb]

Comment on attachment 8758493 [details] [diff] [review]
bug1269711.patch

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

::: js/src/shell/js.cpp
@@ +484,5 @@
> +                RootedValue exn(cx);
> +                if (cx->getPendingException(&exn)) {
> +                    cx->clearPendingException();
> +                    ReportExceptionClosure rec(exn);
> +                    PrepareScriptEnvironmentAndInvoke(cx, cx->global(), rec);

How is all this different from just calling JS_ReportPendingException? The shell never registers a ScriptEnvironmentPreparer, does it?

Comment 11

[Jim Blandy :jimb]

On IRC, Shu said JS_ReportPendingException seemed like an adequate replacement.

Comment 12

[Jim Blandy :jimb]

Comment on attachment 8758493 [details] [diff] [review]
bug1269711.patch

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

Clearing review, since it seems like that gets rid of most of the patch.

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Jim Blandy :jimb from comment #11)
> On IRC, Shu said JS_ReportPendingException seemed like an adequate
> replacement.

Please don't use JS_ReportPendingException. It will probably assert in the error reporter now (it should only be used for warnings) and we want to remove JS_ReportPendingException soon.

The shell has its own ScriptEnvironmentPreparer.

Comment 14

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #13)
> (In reply to Jim Blandy :jimb from comment #11)
> > On IRC, Shu said JS_ReportPendingException seemed like an adequate
> > replacement.
> 
> Please don't use JS_ReportPendingException. It will probably assert in the
> error reporter now (it should only be used for warnings) and we want to
> remove JS_ReportPendingException soon.
> 
> The shell has its own ScriptEnvironmentPreparer.

I see, I didn't know that. Given Jim's request to shorten the patch given the small amount of work that it's actually doing, what's the simplest API I can use to report the pending exception?

Comment 15

[Jim Blandy :jimb]

(In reply to Jan de Mooij [:jandem] from comment #13)
> (In reply to Jim Blandy :jimb from comment #11)
> > On IRC, Shu said JS_ReportPendingException seemed like an adequate
> > replacement.
> 
> Please don't use JS_ReportPendingException. It will probably assert in the
> error reporter now (it should only be used for warnings) and we want to
> remove JS_ReportPendingException soon.
> 
> The shell has its own ScriptEnvironmentPreparer.

As of last Saturday! (I did search for uses of SetScriptEnvironmentPreparer...)

Okay, thanks very much for watching out, Jan. Sorry about the bum steer, Shu. I'm also interested to see the proper way to handle this in new code.

Comment 16

[Shu-yu Guo [:shu]]

Attachment: bug1269711.patch

After talking with Jason, using AutoReportException here works fine.

Comment 17

[Jan de Mooij [:jandem]]

Sorry for not suggesting AutoReportException right away.

(In reply to Jim Blandy :jimb from comment #15)
> As of last Saturday! (I did search for uses of
> SetScriptEnvironmentPreparer...)

DXR maybe? It hasn't been updated since last week or so :/

Comment 18

[Jim Blandy :jimb]

(In reply to Jan de Mooij [:jandem] from comment #17)
> DXR maybe? It hasn't been updated since last week or so :/

No, I just hadn't pulled since Thursday.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e489044de50
Disallow JS shell interrupt callback function to affect exception state of interrupted JS. (r=jimb)

Comment 20

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=29475959&repo=mozilla-inbound

Comment 21

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2395a1485c76
Backed out changeset 6e489044de50 for spidermonkey test failures

Comment 22

[Shu-yu Guo [:shu]]

(In reply to Carsten Book [:Tomcat] from comment #20)
> sorry had to back this out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=29475959&repo=mozilla-
> inbound

shell/futex.js has an explicit test for propagating errors out of the JS interrupt handler. This is the problematic behavior this patch removes.

Lars, what should be done here?

Comment 23

[Lars T Hansen [:lth]]

Start at https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/AtomicsObject.cpp#972.

There's a larger comment block further down in that function explaining some of the complications that give rise to this situation.

IMO, since the wait() is disallowed in this corner case and we signal that by an exception being thrown it is not fabulous if that exception is being dropped on the floor.  That is, I believe the test case happens to be reasonably valid.

That said, this is outside the spec; we could invent other behavior (return a new error code).  But an exception is by far the best solution; generally programs don't inspect the return value from wait() unless a timeout was requested.

(If that doesn't muddy the waters I don't know what will.)

Comment 24

[Shu-yu Guo [:shu]]

(In reply to Lars T Hansen [:lth] from comment #23)
> Start at
> https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/AtomicsObject.
> cpp#972.
> 
> There's a larger comment block further down in that function explaining some
> of the complications that give rise to this situation.
> 
> IMO, since the wait() is disallowed in this corner case and we signal that
> by an exception being thrown it is not fabulous if that exception is being
> dropped on the floor.  That is, I believe the test case happens to be
> reasonably valid.
> 

I agree throwing is preferred here. I'm guessing this code wants to test the situation where another thread is attempting to wait() *while* the main thread is servicing an interrupt. 

What it's actually testing is a re-entrant (but not concurrent!) JS interrupt handler calling wait(). Exceptions that arise in the re-entrant but not concurrent case is what the patch in this bug drops on the floor, because it leads to bizarre behavior like RETRVAL being capable of throwing.

Is there a way to test the not re-entrant but concurrent version?

Comment 25

[Lars T Hansen [:lth]]

The code is testing what it's supposed to test: that wait() is disallowed when being called from the interrupt handler.  No other threads are in the picture.

Comment 26

[Shu-yu Guo [:shu]]

Attachment: futex-test-fix.patch

Is this acceptable?

Comment 27

[Lars T Hansen [:lth]]

Comment on attachment 8760925 [details] [diff] [review]
futex-test-fix.patch

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

I suppose so.  It doesn't seem completely unreasonable for the interrupt handler to eat uncaught exceptions.

Comment 28

[Jim Blandy :jimb]

Wait, what do you mean by "eat"? If you mean "silently ignore", I will hunt you down until the ends of the earth, so that the Furies themselves will be put to shame. Silently ignored errors waste hours of my time on a regular basis. At least print it, and make the shell exit with a non-zero status!

Comment 29

[Lars T Hansen [:lth]]

(In reply to Jim Blandy :jimb from comment #28)
> Wait, what do you mean by "eat"? If you mean "silently ignore", I will hunt
> you down until the ends of the earth, so that the Furies themselves will be
> put to shame.

I hope that was directed at Shu and not at me :)

<snitch>
To the best of my knowledge he will be in London next week, which should simplify your task.
</snitch>

> Silently ignored errors waste hours of my time on a regular
> basis. At least print it, and make the shell exit with a non-zero status!

I think I'll let the two of you figure it out.

Atomics.wait throws an error if called from an interrupt handler that interrupted a previous wait and I don't want to change that.  Previously this error was propagated out from the outer wait (which was aborted by that exception).  Now it is stopped by (notionally) whatever invokes the interrupt handler.  IMO this is not unreasonable but it is suboptimal.

This is possibly not a shell-only issue, though?

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a945349f5f3d
Disallow JS interrupt callback function to affect exception state of interrupted JS. (r=jimb)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5937b04cdeb
Update futex shell test. (r=lth)

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a945349f5f3d
https://hg.mozilla.org/mozilla-central/rev/e5937b04cdeb

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

Is this worth uplifting to 49? I noticed it did not quite make the cut-off before merge day.