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

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: gkw, Assigned: bhackett)

Tracking

(Blocks: 2 bugs, {assertion, jsbugmon, testcase})

Trunk
mozilla55
x86
Linux
assertion, jsbugmon, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 disabled, firefox55 fixed)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(4 attachments)

(Reporter)

Description

a year ago
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.
(Reporter)

Comment 1

a year ago
Created attachment 8841160 [details]
Detailed Crash Information
(Reporter)

Comment 2

a year ago
Created attachment 8841161 [details]
Testcase
(Reporter)

Comment 3

a year ago
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)
(Reporter)

Comment 4

a year ago
// 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
(Reporter)

Comment 5

a year ago
Created attachment 8842770 [details]
stack for Assertion failure: rt->jitRuntime()->preventBackedgePatching() || rt->activeContext()->handlingJitInterrupt()
(Assignee)

Comment 6

a year ago
Created attachment 8843933 [details] [diff] [review]
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.
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()?

Updated

a year ago
Flags: needinfo?(bhackett1024)
(Assignee)

Comment 8

a year ago
(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?
(Assignee)

Comment 10

a year ago
(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.

Updated

a year ago
Attachment #8843933 - Flags: review?(luke) → review+

Comment 12

a year ago
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.

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/de139bac9b5c
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox52: --- → unaffected
status-firefox53: --- → unaffected
status-firefox54: affected → disabled
status-firefox-esr52: --- → unaffected
You need to log in before you can comment on or make changes to this bug.