Closed Bug 1342642 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- disabled
firefox55 --- fixed

People

(Reporter: gkw, Assigned: bhackett)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, jsbugmon, testcase, Whiteboard: [jsbugmon:update])

Attachments

(4 files)

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.
Attached file Testcase
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?
Flags: needinfo?(bhackett1024)
// 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.
Blocks: 1337968
Summary: Assertion failure: CurrentThreadCanAccessRuntime(cx->runtime()), at js/src/threading/ProtectedData.cpp:97 → Assertion failure: CurrentThreadCanAccessRuntime(cx->runtime()), at threading/ProtectedData.cpp:97 or Assertion failure: rt->jitRuntime()->preventBackedgePatching() || rt->activeContext()->handlingJitInterrupt(), at jit/ExecutableAllocator.cpp:330
Attached patch patchSplinter Review
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.
Assignee: nobody → bhackett1024
Flags: needinfo?(bhackett1024)
Attachment #8843933 - Flags: review?(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()?
Flags: needinfo?(bhackett1024)
(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.
Flags: needinfo?(bhackett1024)
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?
(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.
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.
Attachment #8843933 - Flags: review?(luke) → review+
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de139bac9b5c
Don't modify jitcode when interrupting an idle cooperative thread, r=luke.
https://hg.mozilla.org/mozilla-central/rev/de139bac9b5c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.