1242111 - Assertion failure: !script->baselineScript()->active(), at js/src/gc/Zone.cpp:227

Status: RESOLVED FIXED

Description

[Gary Kwong [:gkw] [:nth10sd]]

The following testcase crashes on mozilla-central revision 7104d650a97d (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager):

// Adapted from randomly chosen test: js/src/jit-test/tests/debug/bug1240546.js
var g = newGlobal();
g.debuggeeGlobal = [];
g.eval("(" + function() {
    oomAfterAllocations(57);
    Debugger(debuggeeGlobal).onEnterFrame = function() {}
} + ")()");

Backtrace:

0   js-dbg-64-dm-clang-darwin-7104d650a97d	0x000000010095f2a1 JS::Zone::discardJitCode(js::FreeOp*) + 433 (Zone.cpp:227)
1   js-dbg-64-dm-clang-darwin-7104d650a97d	0x00000001005a56eb js::gc::GCRuntime::beginSweepingZoneGroup() + 2763 (FindSCCs.h:33)
2   js-dbg-64-dm-clang-darwin-7104d650a97d	0x00000001005a6d38 js::gc::GCRuntime::beginSweepPhase(bool) + 1432 (jsgc.cpp:5380)
3   js-dbg-64-dm-clang-darwin-7104d650a97d	0x00000001005aab97 js::gc::GCRuntime::incrementalCollectSlice(js::SliceBudget&, JS::gcreason::Reason) + 951 (SliceBudget.h:77)
4   js-dbg-64-dm-clang-darwin-7104d650a97d	0x00000001005ab77c js::gc::GCRuntime::gcCycle(bool, js::SliceBudget&, JS::gcreason::Reason) + 460 (jsgc.cpp:6355)
5   js-dbg-64-dm-clang-darwin-7104d650a97d	0x00000001005ac135 js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason) + 757 (jsgc.cpp:6455)
6   js-dbg-64-dm-clang-darwin-7104d650a97d	0x000000010059be76 js::gc::GCRuntime::gc(JSGCInvocationKind, JS::gcreason::Reason) + 86 (jsgc.cpp:6514)
7   js-dbg-64-dm-clang-darwin-7104d650a97d	0x000000010053eb9e js::DestroyContext(JSContext*, js::DestroyContextMode) + 462 (Utility.h:384)
8   js-dbg-64-dm-clang-darwin-7104d650a97d	0x0000000100004abc main + 12284 (js.cpp:308)
9   js-dbg-64-dm-clang-darwin-7104d650a97d	0x0000000100000ea4 start + 52

This is happening often just enough to warrant a [fuzzblocker] status.

Comment 1

[Gary Kwong [:gkw] [:nth10sd]]

Seems to go back further than m-c rev 54be5416ae5d. I guess the reason why this just popped up now is because the test for bug 1240546 only just landed.

Setting needinfo? from our Debugger gurus, Jim and Nick.

Comment 2

[Jim Blandy :jimb]

I can reproduce.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Thanks, Jim!

Comment 4

[Jim Blandy :jimb]

The test injects an OOM when a HashTable tries to allocate a smaller table because the current table is underloaded. The error is ignored, because this is supposed to be a benign failure. This all occurs during GC, which is not supposed to OOM.

Comment 5

[Jim Blandy :jimb]

Cancel that; this test runs with OOM_failAlways set, so there are twenty OOMs injected into the program before it crashes; the failure I was looking at in comment 4 is probably innocent.

Comment 6

[Jim Blandy :jimb]

Okay, this is looking more plausible.

Note that the --no-threads and --ion-eager flags are required to see the crash; and since it depends on a specific allocation failing, you probably want to reproduce with the exact changeset suggested.

The test case injects OOMs starting with the 57th allocation onwards. If I let the 57th allocation pass, but begin failing with the 58th, the test case doesn't crash.

The 57th allocation is an attempt to grow a vector, as requested by the call to AppendAndInvalidateScript in UpdateExecutionObservabilityOfScriptsInZone. The crash is a failure of the assertion:

            MOZ_ASSERT_IF(script->hasBaselineScript(), !script->baselineScript()->active());

Since there is a call to FinishDiscardBaselineScript just after the call to AppendAndInvalidateScript, and that specific allocation must fail to trigger the bug, I suspect that we are not backing out of UpdateExecutionObservabilityOfScriptsInZone correctly.

Shu, would you be willing to take a look at this?

Comment 7

[Shu-yu Guo [:shu]]

Created attachment 8711919 [details] [diff] [review]
Handle OOM in UpdateExecutionObservabilityOfScriptsInZone.

Thanks to Jim's analysis the fix was easy.

Comment 8

[Jim Blandy :jimb]

Comment on attachment 8711919 [details] [diff] [review]
Handle OOM in UpdateExecutionObservabilityOfScriptsInZone.

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

Looks good to me - thanks for the fix!

I worry that the regression test will stop being useful almost immediately, as the magic number '57' will become wrong. If it's easy to make it sweep a range of allocation counts (as with 'oomTest' or something like that) and still catch the crash, that might give us more robust coverage. But that's not a condition for r=me.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23f22124b285

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/834c46411298

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/23f22124b285
https://hg.mozilla.org/mozilla-central/rev/834c46411298

Comment 12

[Liz Henry (:lizzard) (needinfo? me)]

Too late for assertion fixes in 46.