Closed Bug 843854 Opened 9 years ago Closed 9 years ago

BaselineCompiler: Assertion failure: !script->baseline->active(), at jscompartment.cpp:605 with OOM

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86_64
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: decoder, Assigned: jandem)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files)

Attached file Testcase for shell
The attached testcase asserts on baseline compiler branch revision de894e57ecb2 (run with --ion-eager).
I can't reproduce this on OS X 64-bit at the revision in comment 0. Do you have a stack trace? (Ideally also one at the point where the OOM happens..)
OK, I could reproduce on decoder's fuzz machine and also a similar OOM. Two problems:

(1) DoUseCountFallback does: 

JS_ASSERT(ion::IsEnabled(cx));

But a TI OOM will disable TI and Ion. Looks like we can just change the assert to an early return.

(2) InvalidateActivation sets the "active" flag on baseline scripts if invalidateAll is set. We assume this only happens when discarding JIT code, but this case also comes up when we disable TI due to an OOM and in this case the "active" flag is never reset and discardJItCode will assert.
Attached patch PatchSplinter Review
Fixes the two bugs described in comment 2. This adds a separate function to mark baseline scripts as active, initially I added this to Invalidate but that's a mistake I think: it really are two different operations and discardJitCode is not the only caller of InvalidateAll (in this case it's called when TI OOM's).
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Attachment #724087 - Flags: review?(kvijayan)
Comment on attachment 724087 [details] [diff] [review]
Patch

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

Nice, this is indeed cleaner than what we were doing.
Attachment #724087 - Flags: review?(kvijayan) → review+
Comment on attachment 724087 [details] [diff] [review]
Patch

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

::: js/src/jscompartment.cpp
@@ +614,1 @@
>          ion::InvalidateAll(fop, this);

Just noticed this when doing SPS related stuff, but |js::ReleaseAllJITCode| in js/src/jsgc.cpp also calls |ion::InvalidateAll|.  We probably want to mark active baseline scripts here as well.
That prompted me to check mxr, and TypeCompartment::nukeTypes in jsinfer.cpp also calls it: https://mxr.mozilla.org/mozilla-aurora/source/js/src/jsinfer.cpp#2780

Fix up here as well.
We only want to call MarkActiveBaselineScripts if we are going to call FinishDiscardBaselineScript. Currently this is only discardJitCode.

We should probably do the same in ReleaseAllJITCode, but not TypeCompartment::nukeTypes: marking active scripts there without calling FinishDiscardBaselineScript is what caused this bug in the first place.
https://hg.mozilla.org/projects/ionmonkey/rev/2db2d953efb6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.