FinishCompilation/CodeGenerator::link can leave a pending exception

RESOLVED FIXED in mozilla29

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jandem, Assigned: decoder)

Tracking

(Blocks: 2 bugs, {assertion})

unspecified
mozilla29
assertion
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

3 years ago
Created attachment 8348026 [details] [diff] [review]
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.
Assignee: nobody → choller
Attachment #8348026 - Flags: review?(bhackett1024)
(Assignee)

Updated

3 years ago
Blocks: 912928
Keywords: assertion
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.
Attachment #8348026 - Flags: review?(bhackett1024)
(Assignee)

Comment 3

3 years ago
Created attachment 8348152 [details] [diff] [review]
js-ti-instantiate-oom.patch

As discussed on IRC :)
Attachment #8348026 - Attachment is obsolete: true
Attachment #8348152 - Flags: review?(bhackett1024)
Attachment #8348152 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 4

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a70f5add1982
Status: NEW → ASSIGNED
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
(Assignee)

Comment 6

3 years ago
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
https://hg.mozilla.org/mozilla-central/rev/6523e469b1cb
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.