Last Comment Bug 1155618 - Ensure we always report allocation failures to the context
: Ensure we always report allocation failures to the context
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: Jon Coppeard (:jonco)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 1161303 1161968 1170665 1195297 1214006
Blocks: 912928
  Show dependency treegraph
 
Reported: 2015-04-17 04:02 PDT by Jon Coppeard (:jonco)
Modified: 2015-10-12 14:55 PDT (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
oom-shell-test (8.75 KB, patch)
2015-04-27 07:11 PDT, Jon Coppeard (:jonco)
terrence.d.cole: review+
Details | Diff | Splinter Review
oom-fixes-global (14.15 KB, patch)
2015-04-27 07:11 PDT, Jon Coppeard (:jonco)
terrence.d.cole: review+
jcoppeard: checkin+
Details | Diff | Splinter Review
oom-dependent-add-ptr (6.61 KB, patch)
2015-04-27 07:13 PDT, Jon Coppeard (:jonco)
terrence.d.cole: review+
jcoppeard: checkin+
Details | Diff | Splinter Review
oom-baseline-fixes (72.76 KB, patch)
2015-04-27 07:17 PDT, Jon Coppeard (:jonco)
no flags Details | Diff | Splinter Review
oom-baseline-fixes v2 (108.88 KB, patch)
2015-04-29 03:40 PDT, Jon Coppeard (:jonco)
jdemooij: review+
jcoppeard: checkin+
Details | Diff | Splinter Review
oom-tracelogger-fix (916 bytes, patch)
2015-04-29 06:14 PDT, Jon Coppeard (:jonco)
bbouvier: review+
jcoppeard: checkin+
Details | Diff | Splinter Review
oom-more-baseline-fixes (6.84 KB, patch)
2015-05-15 09:50 PDT, Jon Coppeard (:jonco)
jdemooij: review+
jcoppeard: checkin+
Details | Diff | Splinter Review
oom-test-support (8.77 KB, patch)
2015-05-21 09:41 PDT, Jon Coppeard (:jonco)
jcoppeard: review+
jcoppeard: checkin+
Details | Diff | Splinter Review
oom-add-new-global-test (470 bytes, patch)
2015-05-21 09:42 PDT, Jon Coppeard (:jonco)
jcoppeard: review+
jcoppeard: checkin+
Details | Diff | Splinter Review
oom-fixes-6 (9.24 KB, patch)
2015-05-27 08:25 PDT, Jon Coppeard (:jonco)
terrence.d.cole: review+
jcoppeard: checkin+
Details | Diff | Splinter Review
oom-fixes-7 (9.37 KB, patch)
2015-05-29 02:59 PDT, Jon Coppeard (:jonco)
terrence.d.cole: review+
jcoppeard: checkin+
Details | Diff | Splinter Review
oom-fixes-8 (14.40 KB, patch)
2015-06-30 08:13 PDT, Jon Coppeard (:jonco)
terrence.d.cole: review+
jcoppeard: checkin+
Details | Diff | Splinter Review
stop-onOutOfMemory-defeating-oom-testing (3.73 KB, patch)
2015-06-30 08:14 PDT, Jon Coppeard (:jonco)
terrence.d.cole: review+
jcoppeard: checkin+
Details | Diff | Splinter Review
oom-tests (816 bytes, patch)
2015-06-30 08:21 PDT, Jon Coppeard (:jonco)
terrence.d.cole: review+
jcoppeard: checkin+
Details | Diff | Splinter Review
oom-fixes-9 (7.55 KB, patch)
2015-07-23 09:38 PDT, Jon Coppeard (:jonco)
terrence.d.cole: review+
jcoppeard: checkin+
Details | Diff | Splinter Review
oom-fixes-9-part-2 (12.10 KB, patch)
2015-07-29 08:56 PDT, Jon Coppeard (:jonco)
luke: review+
jcoppeard: checkin+
Details | Diff | Splinter Review
oom-test-parse-asm (722 bytes, patch)
2015-07-29 08:56 PDT, Jon Coppeard (:jonco)
no flags Details | Diff | Splinter Review
fixes-10 (12.07 KB, patch)
2015-09-03 09:58 PDT, Jon Coppeard (:jonco)
terrence.d.cole: review+
Details | Diff | Splinter Review

Description User image Jon Coppeard (:jonco) 2015-04-17 04:02:10 PDT
As I understand it we are supposed to always report an allocation failure to the context (if available) with js::ReportOutOfMemory().  This throws an exception to let the caller know what happened.

There are a bunch of places where we don't do this however, which results in a failed evaluation without an exception being thrown.
Comment 1 User image Jon Coppeard (:jonco) 2015-04-27 07:11:09 PDT
Created attachment 8598001 [details] [diff] [review]
oom-shell-test

Add shell functions oomAtAllocation() and resetOOMFailure() to cause an OOM at one specific memory allocation.  Add a test case to attempt to create a new global and fail at every successive allocation until this succeeds.
Comment 2 User image Jon Coppeard (:jonco) 2015-04-27 07:11:59 PDT
Created attachment 8598002 [details] [diff] [review]
oom-fixes-global

Fix a whole bunch of places where we don't report allocation failure to the context.
Comment 3 User image Jon Coppeard (:jonco) 2015-04-27 07:13:43 PDT
Created attachment 8598003 [details] [diff] [review]
oom-dependent-add-ptr

Change DependentAddPtr::add() to report allocation failure to the context and update callers that did this previously.  The aim is that everything that takes a context will report allocation failure to it if necessary.
Comment 4 User image Jon Coppeard (:jonco) 2015-04-27 07:17:05 PDT
Created attachment 8598004 [details] [diff] [review]
oom-baseline-fixes

Add an overload of ICStub::New() that takes a context and reports allocations failures and update callers to use this where they have a context.
Comment 5 User image Terrence Cole [:terrence] 2015-04-27 09:35:50 PDT
Comment on attachment 8598001 [details] [diff] [review]
oom-shell-test

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

Neat!
Comment 6 User image Terrence Cole [:terrence] 2015-04-27 09:39:37 PDT
Comment on attachment 8598002 [details] [diff] [review]
oom-fixes-global

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

Wow!
Comment 7 User image Jan de Mooij [:jandem] 2015-04-28 10:48:02 PDT
Comment on attachment 8598004 [details] [diff] [review]
oom-baseline-fixes

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

Clearing review for now per our IRC discussion earlier today.
Comment 8 User image Jon Coppeard (:jonco) 2015-04-29 03:40:15 PDT
Created attachment 8599242 [details] [diff] [review]
oom-baseline-fixes v2

Here's an updated patch as disucussed that:
 - removes the overload of ICStub::New() that doesn't take a context
 - adds ICStubCompiler::newStub<T>() that forwards to ICStub::New()
 - passes a context to IC Clone() methods
Comment 9 User image Jon Coppeard (:jonco) 2015-04-29 06:14:33 PDT
Created attachment 8599289 [details] [diff] [review]
oom-tracelogger-fix

Patch to fix iterating a possibly uninitialized hash table and remove unnecessary calls to finish().
Comment 10 User image Benjamin Bouvier [:bbouvier] 2015-04-29 09:22:47 PDT
Comment on attachment 8599289 [details] [diff] [review]
oom-tracelogger-fix

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

Good catch!
Comment 13 User image Jan de Mooij [:jandem] 2015-05-04 07:42:14 PDT
Comment on attachment 8599242 [details] [diff] [review]
oom-baseline-fixes v2

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

Nice!
Comment 15 User image Wes Kocher (:KWierso) 2015-05-07 15:16:25 PDT
https://hg.mozilla.org/mozilla-central/rev/29f691ba3222
Comment 16 User image Jon Coppeard (:jonco) 2015-05-15 09:50:46 PDT
Created attachment 8606346 [details] [diff] [review]
oom-more-baseline-fixes

A few minor fixes, mostly in baseline.
Comment 18 User image Wes Kocher (:KWierso) 2015-05-20 18:20:43 PDT
https://hg.mozilla.org/mozilla-central/rev/62412fa0716f
Comment 19 User image Jon Coppeard (:jonco) 2015-05-21 09:41:59 PDT
Created attachment 8608819 [details] [diff] [review]
oom-test-support

I'm splitting the first patch into two parts: shell functions to support OOM testing (this patch) and adding a test for global creation (next patch).  I'm going to land the first patch now as it's going to be useful for writing a test for bug 1165966.
Comment 20 User image Jon Coppeard (:jonco) 2015-05-21 09:42:31 PDT
Created attachment 8608820 [details] [diff] [review]
oom-add-new-global-test
Comment 22 User image Phil Ringnalda (:philor) 2015-05-23 13:34:25 PDT
https://hg.mozilla.org/mozilla-central/rev/6ffa14c65354
Comment 23 User image Jon Coppeard (:jonco) 2015-05-27 08:25:29 PDT
Created attachment 8611239 [details] [diff] [review]
oom-fixes-6

Various fixes.
Comment 25 User image Jon Coppeard (:jonco) 2015-05-29 02:59:57 PDT
Created attachment 8612782 [details] [diff] [review]
oom-fixes-7
Comment 26 User image Ryan VanderMeulen [:RyanVM] 2015-05-29 06:44:59 PDT
https://hg.mozilla.org/mozilla-central/rev/1519a2b83f3f
Comment 28 User image Carsten Book [:Tomcat] 2015-06-02 04:16:39 PDT
https://hg.mozilla.org/mozilla-central/rev/7a5a8ce170a0
Comment 30 User image Ryan VanderMeulen [:RyanVM] 2015-06-03 13:07:28 PDT
https://hg.mozilla.org/mozilla-central/rev/24d30440d9eb
Comment 31 User image Jon Coppeard (:jonco) 2015-06-30 08:13:07 PDT
Created attachment 8627702 [details] [diff] [review]
oom-fixes-8

Fix a bunch more places we don't correctly report allocation failure to the context.
Comment 32 User image Jon Coppeard (:jonco) 2015-06-30 08:14:37 PDT
Created attachment 8627704 [details] [diff] [review]
stop-onOutOfMemory-defeating-oom-testing

I realised that retrying the allocation in onOutOfMemory() actually defeats our OOM testing.  This patch removes this behaviour for simulated OOM failures.
Comment 33 User image Jon Coppeard (:jonco) 2015-06-30 08:21:10 PDT
Created attachment 8627708 [details] [diff] [review]
oom-tests

Add a couple more OOM tests now that these pass.
Comment 34 User image Terrence Cole [:terrence] 2015-07-01 07:59:37 PDT
Comment on attachment 8627704 [details] [diff] [review]
stop-onOutOfMemory-defeating-oom-testing

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

Good find.
Comment 35 User image Terrence Cole [:terrence] 2015-07-01 08:00:16 PDT
Comment on attachment 8627708 [details] [diff] [review]
oom-tests

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

No review needed for adding tests, but r=me, fwiw.
Comment 38 User image Jon Coppeard (:jonco) 2015-07-23 09:38:07 PDT
Created attachment 8637996 [details] [diff] [review]
oom-fixes-9

This patch fixes several assertions that happen in asm.js compilation when we hit OOM:

 - check for allocation failure in FunctionCompiler::prepareEmitMIR()
 - move assertions out of ~FunctionCompiler() and only run them if the compilation succeeds.  We aren't running JS during asm.js compilation so ReportOutOfMemory() doesn't set a pending exception on the context.
 - don't simulate allocation failure in LifoAlloc::allocInfallible()
 - fix off-by-one in check for simulated OOM failure in ~Label() (this is a bit gross but I can't think of a better way to skip the check on error here).
Comment 39 User image Terrence Cole [:terrence] 2015-07-23 13:10:10 PDT
Comment on attachment 8637996 [details] [diff] [review]
oom-fixes-9

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

\o/ Very neat!
Comment 40 User image Jon Coppeard (:jonco) 2015-07-29 08:56:04 PDT
Created attachment 8640539 [details] [diff] [review]
oom-fixes-9-part-2

The test in the previous patch is still failing on Windows.  This patch fixes more OOM issues related to parsing AsmJS, but it's still not enough to make the test pass.  I'm planning to land the fixes anyway and leave the test out until I work out what's going wrong.

The strategy for handling OOM while compiling seems to be to note the fact that it has happened and report it later.  A lot of the changes in this patch are related to fixing assertions that didn't pass if OOM had happened.  In some cases (the Generate* functions in AsmJSValidate.cpp) it's easiest to just return failure sooner.
Comment 41 User image Jon Coppeard (:jonco) 2015-07-29 08:56:47 PDT
Created attachment 8640540 [details] [diff] [review]
oom-test-parse-asm

This still fails on Windows.
Comment 42 User image Luke Wagner [:luke] 2015-07-29 10:35:52 PDT
Comment on attachment 8640539 [details] [diff] [review]
oom-fixes-9-part-2

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

::: js/src/jit/x64/MacroAssembler-x64.cpp
@@ +40,3 @@
>          enoughMemory_ &= doubleMap_.add(p, d, doubleIndex);
>          if (!enoughMemory_)
>              return;

Why do we need the ones in this file?  Is it just good form or is there an assert that can be hit with the new testing function and, if so, how?
Comment 43 User image Jon Coppeard (:jonco) 2015-07-30 02:13:51 PDT
(In reply to Luke Wagner (PTO) [:luke] from comment #42)

> Why do we need the ones in this file?  Is it just good form or is there an
> assert that can be hit with the new testing function and, if so, how?

The vector append can fail and the following hash map insertion can succeed.  Since we don't abort compilation immediately, a later call to this method can find the entry in map and try to access a location past the end of the vector.
Comment 45 User image Ryan VanderMeulen [:RyanVM] 2015-07-30 13:12:52 PDT
https://hg.mozilla.org/mozilla-central/rev/a69943dee0b2
Comment 46 User image Jon Coppeard (:jonco) 2015-09-03 09:58:32 PDT
Created attachment 8656665 [details] [diff] [review]
fixes-10

Fix more OOM handling bugs.
Comment 48 User image Carsten Book [:Tomcat] 2015-09-08 06:40:45 PDT
https://hg.mozilla.org/mozilla-central/rev/7c18fbc2d151
Comment 49 User image Jon Coppeard (:jonco) 2015-10-09 01:26:23 PDT
There are still instances of this problem, but it's all part of the wider OOM fixing effort in bug 912928, so closing this bug.

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