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

RESOLVED FIXED in Firefox 47

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gkw, Assigned: jimb)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
mozilla47
x86_64
Mac OS X
assertion, regression, testcase
Points:
---

Firefox Tracking Flags

(firefox46 wontfix, firefox47 fixed)

Details

(Whiteboard: [fuzzblocker][jsbugmon:update])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
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.
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(jimb)
Whiteboard: [fuzzblocker][jsbugmon:update,bisect] → [fuzzblocker][jsbugmon:update]
(Assignee)

Comment 2

2 years ago
I can reproduce.
Flags: needinfo?(jimb)
(Assignee)

Updated

2 years ago
Assignee: nobody → jimb
Thanks, Jim!
Flags: needinfo?(nfitzgerald)
(Assignee)

Comment 4

2 years ago
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.
(Assignee)

Comment 5

2 years ago
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.
(Assignee)

Comment 6

2 years ago
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?
Flags: needinfo?(shu)

Comment 7

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

Thanks to Jim's analysis the fix was easy.
Attachment #8711919 - Flags: review?(jimb)

Updated

2 years ago
Flags: needinfo?(shu)
(Assignee)

Comment 8

2 years ago
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.
Attachment #8711919 - Flags: review?(jimb) → review+

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/23f22124b285
https://hg.mozilla.org/mozilla-central/rev/834c46411298
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Too late for assertion fixes in 46.
status-firefox46: affected → wontfix
You need to log in before you can comment on or make changes to this bug.