Last Comment Bug 950658 - FinishCompilation/CodeGenerator::link can leave a pending exception
: FinishCompilation/CodeGenerator::link can leave a pending exception
: 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]
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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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]

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]

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]

As discussed on IRC :)
Comment 4 User image Christian Holler (:decoder) 2013-12-16 14:57:41 PST
Comment 5 User image Wes Kocher (:KWierso) 2013-12-16 17:47:52 PST
Backed out in for breaking a test on Windows like this:
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 :)
Comment 7 User image Wes Kocher (:KWierso) 2013-12-18 20:51:58 PST

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