Last Comment Bug 950658 - FinishCompilation/CodeGenerator::link can leave a pending exception
: FinishCompilation/CodeGenerator::link can leave a pending exception
Status: RESOLVED FIXED
: assertion
Product: Core
Classification: Components
Component: JavaScript Engine: JIT (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla29
Assigned To: Christian Holler (:decoder)
:
: Hannes Verschore [:h4writer]
Mentors:
Depends on:
Blocks: langfuzz 912928
  Show dependency treegraph
 
Reported: 2013-12-16 05:42 PST by Jan de Mooij [:jandem]
Modified: 2013-12-18 20:51 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
js-finishcomp-oom.patch (1.50 KB, patch)
2013-12-16 06:14 PST, Christian Holler (:decoder)
no flags Details | Diff | Splinter Review
js-ti-instantiate-oom.patch (1006 bytes, patch)
2013-12-16 09:45 PST, Christian Holler (:decoder)
bhackett1024: review+
Details | Diff | Splinter Review

Description User image Jan de Mooij [:jandem] 2013-12-16 05:42:35 PST
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 User image Christian Holler (:decoder) 2013-12-16 06:14:17 PST
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.
Comment 2 User image Brian Hackett (:bhackett) 2013-12-16 08:51:34 PST
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 User image Christian Holler (:decoder) 2013-12-16 09:45:50 PST
Created attachment 8348152 [details] [diff] [review]
js-ti-instantiate-oom.patch

As discussed on IRC :)
Comment 4 User image Christian Holler (:decoder) 2013-12-16 14:57:41 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/a70f5add1982
Comment 5 User image Wes Kocher (:KWierso) 2013-12-16 17:47:52 PST
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 User image Christian Holler (:decoder) 2013-12-18 16:17:42 PST
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 User image Wes Kocher (:KWierso) 2013-12-18 20:51:58 PST
https://hg.mozilla.org/mozilla-central/rev/6523e469b1cb

Note You need to log in before you can comment on or make changes to this bug.