Closed Bug 1155618 Opened 5 years ago Closed 4 years ago

Ensure we always report allocation failures to the context

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jonco, Assigned: jonco)

References

(Blocks 1 open bug)

Details

Attachments

(16 files, 2 obsolete files)

14.15 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
6.61 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
108.88 KB, patch
jandem
: review+
jonco
: checkin+
Details | Diff | Splinter Review
916 bytes, patch
bbouvier
: review+
jonco
: checkin+
Details | Diff | Splinter Review
6.84 KB, patch
jandem
: review+
jonco
: checkin+
Details | Diff | Splinter Review
8.77 KB, patch
jonco
: review+
jonco
: checkin+
Details | Diff | Splinter Review
470 bytes, patch
jonco
: review+
jonco
: checkin+
Details | Diff | Splinter Review
9.24 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
9.37 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
14.40 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
3.73 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
816 bytes, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
7.55 KB, patch
terrence
: review+
jonco
: checkin+
Details | Diff | Splinter Review
12.10 KB, patch
luke
: review+
jonco
: checkin+
Details | Diff | Splinter Review
722 bytes, patch
Details | Diff | Splinter Review
12.07 KB, patch
terrence
: review+
Details | Diff | Splinter Review
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.
Attached patch oom-shell-test (obsolete) — Splinter Review
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.
Assignee: nobody → jcoppeard
Attachment #8598001 - Flags: review?(terrence)
Attached patch oom-fixes-globalSplinter Review
Fix a whole bunch of places where we don't report allocation failure to the context.
Attachment #8598002 - Flags: review?(terrence)
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.
Attachment #8598003 - Flags: review?(terrence)
Attached patch oom-baseline-fixes (obsolete) — Splinter Review
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.
Attachment #8598004 - Flags: review?(jdemooij)
Keywords: leave-open
Comment on attachment 8598001 [details] [diff] [review]
oom-shell-test

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

Neat!
Attachment #8598001 - Flags: review?(terrence) → review+
Comment on attachment 8598002 [details] [diff] [review]
oom-fixes-global

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

Wow!
Attachment #8598002 - Flags: review?(terrence) → review+
Attachment #8598003 - Flags: review?(terrence) → review+
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.
Attachment #8598004 - Flags: review?(jdemooij)
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
Attachment #8598004 - Attachment is obsolete: true
Attachment #8599242 - Flags: review?(jdemooij)
Patch to fix iterating a possibly uninitialized hash table and remove unnecessary calls to finish().
Attachment #8599289 - Flags: review?(benj)
Comment on attachment 8599289 [details] [diff] [review]
oom-tracelogger-fix

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

Good catch!
Attachment #8599289 - Flags: review?(benj) → review+
Comment on attachment 8599242 [details] [diff] [review]
oom-baseline-fixes v2

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

Nice!
Attachment #8599242 - Flags: review?(jdemooij) → review+
Attachment #8598002 - Flags: checkin+
Attachment #8598003 - Flags: checkin+
Attachment #8599289 - Flags: checkin+
Depends on: 1161303
Depends on: 1161968
Attachment #8599242 - Flags: checkin+
A few minor fixes, mostly in baseline.
Attachment #8606346 - Flags: review?(jdemooij)
Attachment #8606346 - Flags: review?(jdemooij) → review+
Attachment #8606346 - Flags: checkin+
Attached patch oom-test-supportSplinter Review
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.
Attachment #8598001 - Attachment is obsolete: true
Attachment #8608819 - Flags: review+
Attachment #8608820 - Flags: review+
Attachment #8608819 - Flags: checkin+
Attached patch oom-fixes-6Splinter Review
Various fixes.
Attachment #8611239 - Flags: review?(terrence)
Attachment #8611239 - Flags: review?(terrence) → review+
Attachment #8611239 - Flags: checkin+
Attached patch oom-fixes-7Splinter Review
Attachment #8612782 - Flags: review?(terrence)
Attachment #8612782 - Flags: review?(terrence) → review+
Depends on: 1170665
Attachment #8612782 - Flags: checkin+
Attachment #8608820 - Flags: checkin+
Attached patch oom-fixes-8Splinter Review
Fix a bunch more places we don't correctly report allocation failure to the context.
Attachment #8627702 - Flags: review?(terrence)
I realised that retrying the allocation in onOutOfMemory() actually defeats our OOM testing.  This patch removes this behaviour for simulated OOM failures.
Attachment #8627704 - Flags: review?(terrence)
Attached patch oom-testsSplinter Review
Add a couple more OOM tests now that these pass.
Attachment #8627708 - Flags: review?(terrence)
Attachment #8627702 - Flags: review?(terrence) → review+
Comment on attachment 8627704 [details] [diff] [review]
stop-onOutOfMemory-defeating-oom-testing

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

Good find.
Attachment #8627704 - Flags: review?(terrence) → review+
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.
Attachment #8627708 - Flags: review?(terrence) → review+
Attachment #8627702 - Flags: checkin+
Attachment #8627704 - Flags: checkin+
Attachment #8627708 - Flags: checkin+
Attached patch oom-fixes-9Splinter Review
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).
Attachment #8637996 - Flags: review?(terrence)
Comment on attachment 8637996 [details] [diff] [review]
oom-fixes-9

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

\o/ Very neat!
Attachment #8637996 - Flags: review?(terrence) → review+
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.
Attachment #8640539 - Flags: review?(luke)
This still fails on Windows.
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?
Attachment #8640539 - Flags: review?(luke) → review+
(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.
Attachment #8637996 - Flags: checkin+
Attachment #8640539 - Flags: checkin+
Depends on: 1195297
Depends on: 1198090
No longer depends on: 1198090
Attached patch fixes-10Splinter Review
Fix more OOM handling bugs.
Attachment #8656665 - Flags: review?(terrence)
Attachment #8656665 - Flags: review?(terrence) → review+
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.
Blocks: 912928
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Depends on: 1214006
You need to log in before you can comment on or make changes to this bug.