Closed
Bug 1155618
Opened 9 years ago
Closed 9 years ago
Ensure we always report allocation failures to the context
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
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.
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
Fix a whole bunch of places where we don't report allocation failure to the context.
Attachment #8598002 -
Flags: review?(terrence)
Assignee | ||
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Comment 5•9 years ago
|
||
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 6•9 years ago
|
||
Comment on attachment 8598002 [details] [diff] [review] oom-fixes-global Review of attachment 8598002 [details] [diff] [review]: ----------------------------------------------------------------- Wow!
Attachment #8598002 -
Flags: review?(terrence) → review+
Updated•9 years ago
|
Attachment #8598003 -
Flags: review?(terrence) → review+
Comment 7•9 years ago
|
||
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)
Assignee | ||
Comment 8•9 years ago
|
||
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)
Assignee | ||
Comment 9•9 years ago
|
||
Patch to fix iterating a possibly uninitialized hash table and remove unnecessary calls to finish().
Attachment #8599289 -
Flags: review?(benj)
Comment 10•9 years ago
|
||
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 11•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fae47e061312 https://hg.mozilla.org/integration/mozilla-inbound/rev/454541170ba2 https://hg.mozilla.org/integration/mozilla-inbound/rev/c87002625551
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fae47e061312 https://hg.mozilla.org/mozilla-central/rev/454541170ba2 https://hg.mozilla.org/mozilla-central/rev/c87002625551
Comment 13•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
Attachment #8598002 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #8598003 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #8599289 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #8599242 -
Flags: checkin+
Assignee | ||
Comment 16•9 years ago
|
||
A few minor fixes, mostly in baseline.
Attachment #8606346 -
Flags: review?(jdemooij)
Updated•9 years ago
|
Attachment #8606346 -
Flags: review?(jdemooij) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8606346 -
Flags: checkin+
Assignee | ||
Comment 19•9 years ago
|
||
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+
Assignee | ||
Comment 20•9 years ago
|
||
Attachment #8608820 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8608819 -
Flags: checkin+
Updated•9 years ago
|
Attachment #8611239 -
Flags: review?(terrence) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8611239 -
Flags: checkin+
Assignee | ||
Comment 25•9 years ago
|
||
Attachment #8612782 -
Flags: review?(terrence)
Updated•9 years ago
|
Attachment #8612782 -
Flags: review?(terrence) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8612782 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #8608820 -
Flags: checkin+
Comment 30•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/24d30440d9eb
Flags: in-testsuite+
Assignee | ||
Comment 31•9 years ago
|
||
Fix a bunch more places we don't correctly report allocation failure to the context.
Attachment #8627702 -
Flags: review?(terrence)
Assignee | ||
Comment 32•9 years ago
|
||
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)
Assignee | ||
Comment 33•9 years ago
|
||
Add a couple more OOM tests now that these pass.
Attachment #8627708 -
Flags: review?(terrence)
Updated•9 years ago
|
Attachment #8627702 -
Flags: review?(terrence) → review+
Comment 34•9 years ago
|
||
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 35•9 years ago
|
||
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+
Comment 36•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9f4af8fe60e https://hg.mozilla.org/integration/mozilla-inbound/rev/a9f12b317316 https://hg.mozilla.org/integration/mozilla-inbound/rev/3ca5b7671aaa
Comment 37•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c9f4af8fe60e https://hg.mozilla.org/mozilla-central/rev/a9f12b317316 https://hg.mozilla.org/mozilla-central/rev/3ca5b7671aaa
Assignee | ||
Updated•9 years ago
|
Attachment #8627702 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #8627704 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #8627708 -
Flags: checkin+
Assignee | ||
Comment 38•9 years ago
|
||
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 39•9 years ago
|
||
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+
Assignee | ||
Comment 40•9 years ago
|
||
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)
Assignee | ||
Comment 41•9 years ago
|
||
This still fails on Windows.
Comment 42•9 years ago
|
||
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+
Assignee | ||
Comment 43•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Attachment #8637996 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #8640539 -
Flags: checkin+
Assignee | ||
Comment 46•9 years ago
|
||
Fix more OOM handling bugs.
Attachment #8656665 -
Flags: review?(terrence)
Updated•9 years ago
|
Attachment #8656665 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 49•9 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•