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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: Testcase for shell

The attached testcase asserts on baseline compiler branch revision de894e57ecb2 (run with --ion-eager).

Comment 1

[Jan de Mooij [:jandem]]

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..)

Comment 2

[Jan de Mooij [:jandem]]

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.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch

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).

Comment 4

[Kannan Vijayan [:djvj]]

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.

Comment 5

[Kannan Vijayan [:djvj]]

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.

Comment 6

[Kannan Vijayan [:djvj]]

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.

Comment 7

[Jan de Mooij [:jandem]]

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.

Comment 8

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/projects/ionmonkey/rev/2db2d953efb6