1342642 - Assertion failure: CurrentThreadCanAccessRuntime(cx->runtime()), at threading/ProtectedData.cpp:97 or Assertion failure: rt->jitRuntime()->preventBackedgePatching() || rt->activeContext()->handlingJitInterrupt(), at jit/ExecutableAllocator.cpp:330

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

The following testcase crashes on mozilla-central revision 69d2cf007cdc (build with --32 --enable-debug, run with --fuzzing-safe --gc-zeal=16 --ion-offthread-compile=off --no-baseline --no-ion):

See attachment.

Backtrace:

#0  0x08699a76 in js::CheckZoneGroup<(js::AllowedHelperThread)0>::check (this=0xf701b14c) at js/src/threading/ProtectedData.cpp:97
#1  0x089ac12b in js::ProtectedData<js::CheckZoneGroup<(js::AllowedHelperThread)0>, js::jit::JitZoneGroup*>::ref (this=0xf701b148) at js/src/threading/ProtectedData.h:109
#2  js::ProtectedData<js::CheckZoneGroup<(js::AllowedHelperThread)0>, js::jit::JitZoneGroup*>::operator js::jit::JitZoneGroup* const& (this=0xf701b148) at js/src/threading/ProtectedData.h:81
#3  RedirectIonBackedgesToInterruptCheck (cx=cx@entry=0xf704f800) at js/src/wasm/WasmSignalHandlers.cpp:1225
#4  0x089ae1bd in RedirectJitCodeToInterruptCheck (context=0xffbb314c, cx=0xf704f800) at js/src/wasm/WasmSignalHandlers.cpp:1236
/snip

For detailed crash information, see attachment.

Comment 1

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

Attachment: Detailed Crash Information

Comment 2

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

Attachment: Testcase

Comment 3

[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/fad2e60d7843
user:        Brian Hackett
date:        Fri Feb 17 05:13:11 2017 -0700
summary:     Bug 1337968 - Add API and shell harness for cooperative multithreading, r=jandem.

Brian, is bug 1337968 a likely regressor?

Comment 4

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

// jsfunfuzz-generated
for (var i = 0; i < 499; i++) {
    // Adapted from randomly chosen test: js/src/jit-test/tests/basic/testBug692274-2.js
    function f() {
        f();
        for (var x = this;;) {}
    }
    try {
        f();
    } catch (e) {}
    // jsfunfuzz-generated
    timeout(1800);
    // Adapted from randomly chosen test: js/src/jit-test/tests/basic/cooperative-threading-interrupt.js
    evaluate("evalInCooperativeThread('}');", {
        compileAndGo: true
    })
}

compile with --enable-debug, run with --fuzzing-safe --ion-offthread-compile=off --ion-eager --gc-zeal=6, tested on m-c rev e150eaff1f83

This asserts on Windows 10 at:

Assertion failure: rt->jitRuntime()->preventBackedgePatching() || rt->activeContext()->handlingJitInterrupt(), at js/src/jit/ExecutableAllocator.cpp:330

but the same testcase asserts at this CurrentThreadCanAccessRuntime one.

Comment 5

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

Attachment: stack for Assertion failure: rt->jitRuntime()->preventBackedgePatching() || rt->activeContext()->handlingJitInterrupt()

Comment 6

[Brian Hackett [Laid off!]]

Attachment: patch

Redirecting jitcode after an interrupt may only be done on the runtime's active thread.  If the embedder tries to interrupt a thread which is *not* the active thread it should be fine to just skip jitcode redirection --- the thread is idle and can't be interrupted right now anyways, and if control returns to it and it starts ilooping in jitcode or something then another interrupt request will come in shortly.

Comment 7

[Luke Wagner [:luke]]

To help me understand the invariants here: if a given thread has exclusive access to a given ZoneGroup that it's executing, what's the additional requirement that JSContext be the rt->activeContext()?

Comment 8

[Brian Hackett [Laid off!]]

(In reply to Luke Wagner [:luke] from comment #7)
> To help me understand the invariants here: if a given thread has exclusive
> access to a given ZoneGroup that it's executing, what's the additional
> requirement that JSContext be the rt->activeContext()?

Currently we only support cooperative multithreading: a lot of data which is on the runtime or associated structures is not threadsafe, and can only be accessed by the runtime's active context (these fields have an ActiveThreadData protected data type).  This includes the executable allocators and other state which are accessed when interrupting running Ion code.

Comment 9

[Luke Wagner [:luke]]

Ok, that makes sense.  But if we've halted/suspended the one cooperatively-shared thread, wouldn't it be ok to access that JSRuntime's shared data?

Comment 10

[Brian Hackett [Laid off!]]

(In reply to Luke Wagner [:luke] from comment #9)
> Ok, that makes sense.  But if we've halted/suspended the one
> cooperatively-shared thread, wouldn't it be ok to access that JSRuntime's
> shared data?

The problem is that there's no guarantee here that the active cooperatively scheduled thread has halted/suspended.  If the embedder calls JS_RequestInterruptCallback on a cx for an inactive thread, then we don't want to start accessing/modifying runtime data that could be concurrently accessed by the active thread itself.

Comment 11

[Luke Wagner [:luke]]

Ohhh, so it's cooperative, but there is a thread-per-JSContext (and only one is active at a time)?  (I sorta remember this from the plan a while back.)

Ok, and so even if we've halted a given cx's thread, that cx may not be the active one and so a different thread could be active and touching the JSRuntime.  Got it.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de139bac9b5c
Don't modify jitcode when interrupting an idle cooperative thread, r=luke.

Comment 13

[Carsten Book [:Tomcat]]

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