950658 - FinishCompilation/CodeGenerator::link can leave a pending exception

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

decoder found a testcase where we throw an OOM exception, return false from FinishCompilation and then return true from CodeGenerator::link. Later on we assert !cx->isExceptionPending().

Comment 1

[Christian Holler (:decoder)]

Attachment: js-finishcomp-oom.patch

This patch fixes it as jandem suggested and also adds an assertion he suggests to add. However, the assertion might be problematic (such an assertion recently lead to intermittent orange), so I'm not sure if we should take it here. Brian, any thoughts? I'm also making a try run for this on Linux right now.

Comment 2

[Brian Hackett [Laid off!]]

Comment on attachment 8348026 [details] [diff] [review]
js-finishcomp-oom.patch

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

::: js/src/jit/Ion.cpp
@@ +1923,5 @@
>          return Method_Error;
>      }
>  
>      // Compilation succeeded or we invalidated right away or an inlining/alloc abort
> +    JS_ASSERT(!cx->isExceptionPending());

This assert will have the same problem as the assert which bug 923614 removed --- we can do Ion compilation with an exception already on the context.

::: js/src/jsinfer.cpp
@@ +997,5 @@
>  
>      if (!succeeded || types.compilerOutputs->back().pendingInvalidation()) {
>          types.compilerOutputs->back().invalidate();
>          script->resetUseCount();
> +        cx->clearPendingException();

This clearPendingException should happen immediately after the call in this function which generated the exception.  Here it could clear an exception already on the cx at the start, or there could be a premature return where the exception was not cleared before returning.

Comment 3

[Christian Holler (:decoder)]

Attachment: js-ti-instantiate-oom.patch

As discussed on IRC :)

Comment 4

[Christian Holler (:decoder)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a70f5add1982

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/97822855e969 for breaking a test on Windows like this: https://tbpl.mozilla.org/php/getParsedLog.php?id=32057355&tree=Mozilla-Inbound

Comment 6

[Christian Holler (:decoder)]

Relanded after the throwing fix has made it to inbound in bug 950725. jit-tests show green now :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/6523e469b1cb

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6523e469b1cb