Last Comment Bug 945754 - Assertion failure: !cx->isExceptionPending(), at ../vm/Interpreter.cpp:3468 due to OOM
: Assertion failure: !cx->isExceptionPending(), at ../vm/Interpreter.cpp:3468 d...
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
-- critical (vote)
: mozilla28
Assigned To: Christian Holler (:decoder)
: general
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: langfuzz 912928
  Show dependency treegraph
 
Reported: 2013-12-03 07:45 PST by Christian Holler (:decoder)
Modified: 2013-12-03 13:50 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
js-ti-oom.patch (2.45 KB, patch)
2013-12-03 07:45 PST, Christian Holler (:decoder)
no flags Details | Diff | Splinter Review
js-ti-oom.patch (877 bytes, patch)
2013-12-03 08:43 PST, Christian Holler (:decoder)
bhackett1024: review+
Details | Diff | Splinter Review

Description User image Christian Holler (:decoder) 2013-12-03 07:45:29 PST
Created attachment 8341744 [details] [diff] [review]
js-ti-oom.patch

I'm hitting the mentioned assertion during fuzzing and tracked it down to the function EnsureTrackPropertyTypes together with jandem. He suggested that this function should propagate the OOM condition, so I made a small patch to do that. It passes all jit-tests but I'm not exactly sure about the first return. Is it right to return true in this function if the inference is not running at all?
Comment 1 User image Brian Hackett (:bhackett) 2013-12-03 08:24:23 PST
Comment on attachment 8341744 [details] [diff] [review]
js-ti-oom.patch

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

::: js/src/jsinferinlines.h
@@ +429,5 @@
>  
>      if (obj->hasSingletonType()) {
>          AutoEnterAnalysis enter(cx);
>          if (obj->hasLazyType() && !obj->getType(cx)) {
>              cx->compartment()->types.setPendingNukeTypes(cx);

Generally, TI stuff isn't meant to be fallible because it can just setPendingNukeTypes and let execution proceed as normal.  This is no longer strictly necessary but is still nice for clean APIs and so forth, e.g. this patch is missing edits to about 10 calls to EnsureTrackPropertyTypes that now do not check their return value.

Instead of changing the interface to EnsureTrackPropertyTypes, can you just add a cx->clearPendingException() call here?
Comment 2 User image Christian Holler (:decoder) 2013-12-03 08:43:50 PST
Created attachment 8341773 [details] [diff] [review]
js-ti-oom.patch

Like this? Also fixes the assert for me.
Comment 3 User image Christian Holler (:decoder) 2013-12-03 09:26:11 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a7ed8362caa
Comment 4 User image Ryan VanderMeulen [:RyanVM] 2013-12-03 13:50:33 PST
https://hg.mozilla.org/mozilla-central/rev/8a7ed8362caa

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