The default bug view has changed. See this FAQ.

Ensure we always report allocation failures to the context

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

(Blocks: 1 bug)

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(16 attachments, 2 obsolete attachments)

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
(Assignee)

Description

2 years ago
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

2 years ago
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.
Assignee: nobody → jcoppeard
Attachment #8598001 - Flags: review?(terrence)
(Assignee)

Comment 2

2 years ago
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.
Attachment #8598002 - Flags: review?(terrence)
(Assignee)

Comment 3

2 years ago
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.
Attachment #8598003 - Flags: review?(terrence)
(Assignee)

Comment 4

2 years ago
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.
Attachment #8598004 - Flags: review?(jdemooij)
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 8

2 years ago
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
Attachment #8598004 - Attachment is obsolete: true
Attachment #8599242 - Flags: review?(jdemooij)
(Assignee)

Comment 9

2 years ago
Created attachment 8599289 [details] [diff] [review]
oom-tracelogger-fix

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 11

2 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
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 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

2 years ago
Attachment #8598002 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8598003 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8599289 - Flags: checkin+
(Assignee)

Updated

2 years ago
Depends on: 1161303
(Assignee)

Updated

2 years ago
Depends on: 1161968

Comment 14

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/29f691ba3222
https://hg.mozilla.org/mozilla-central/rev/29f691ba3222
(Assignee)

Updated

2 years ago
Attachment #8599242 - Flags: checkin+
(Assignee)

Comment 16

2 years ago
Created attachment 8606346 [details] [diff] [review]
oom-more-baseline-fixes

A few minor fixes, mostly in baseline.
Attachment #8606346 - Flags: review?(jdemooij)

Updated

2 years ago
Attachment #8606346 - Flags: review?(jdemooij) → review+

Comment 17

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/62412fa0716f
(Assignee)

Updated

2 years ago
Attachment #8606346 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/62412fa0716f
(Assignee)

Comment 19

2 years ago
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.
Attachment #8598001 - Attachment is obsolete: true
Attachment #8608819 - Flags: review+
(Assignee)

Comment 20

2 years ago
Created attachment 8608820 [details] [diff] [review]
oom-add-new-global-test
Attachment #8608820 - Flags: review+

Comment 21

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ffa14c65354
https://hg.mozilla.org/mozilla-central/rev/6ffa14c65354
(Assignee)

Updated

2 years ago
Attachment #8608819 - Flags: checkin+
(Assignee)

Comment 23

2 years ago
Created attachment 8611239 [details] [diff] [review]
oom-fixes-6

Various fixes.
Attachment #8611239 - Flags: review?(terrence)
Attachment #8611239 - Flags: review?(terrence) → review+

Comment 24

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1519a2b83f3f
(Assignee)

Updated

2 years ago
Attachment #8611239 - Flags: checkin+
(Assignee)

Comment 25

2 years ago
Created attachment 8612782 [details] [diff] [review]
oom-fixes-7
Attachment #8612782 - Flags: review?(terrence)
https://hg.mozilla.org/mozilla-central/rev/1519a2b83f3f
Attachment #8612782 - Flags: review?(terrence) → review+

Comment 27

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a5a8ce170a0
https://hg.mozilla.org/mozilla-central/rev/7a5a8ce170a0
Depends on: 1170665

Comment 29

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/24d30440d9eb
(Assignee)

Updated

2 years ago
Attachment #8612782 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8608820 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/24d30440d9eb
Flags: in-testsuite+
(Assignee)

Comment 31

2 years ago
Created attachment 8627702 [details] [diff] [review]
oom-fixes-8

Fix a bunch more places we don't correctly report allocation failure to the context.
Attachment #8627702 - Flags: review?(terrence)
(Assignee)

Comment 32

2 years ago
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.
Attachment #8627704 - Flags: review?(terrence)
(Assignee)

Comment 33

2 years ago
Created attachment 8627708 [details] [diff] [review]
oom-tests

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+

Comment 36

2 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
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

2 years ago
Attachment #8627702 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8627704 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8627708 - Flags: checkin+
(Assignee)

Comment 38

2 years ago
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).
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+
(Assignee)

Comment 40

2 years ago
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.
Attachment #8640539 - Flags: review?(luke)
(Assignee)

Comment 41

2 years ago
Created attachment 8640540 [details] [diff] [review]
oom-test-parse-asm

This still fails on Windows.

Comment 42

2 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

2 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.

Comment 44

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a69943dee0b2
https://hg.mozilla.org/mozilla-central/rev/a69943dee0b2
(Assignee)

Updated

2 years ago
Attachment #8637996 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8640539 - Flags: checkin+
(Assignee)

Updated

2 years ago
Depends on: 1195297
Depends on: 1198090
No longer depends on: 1198090
(Assignee)

Comment 46

2 years ago
Created attachment 8656665 [details] [diff] [review]
fixes-10

Fix more OOM handling bugs.
Attachment #8656665 - Flags: review?(terrence)
Attachment #8656665 - Flags: review?(terrence) → review+

Comment 47

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c18fbc2d151
https://hg.mozilla.org/mozilla-central/rev/7c18fbc2d151
(Assignee)

Comment 49

2 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.
Blocks: 912928
Status: NEW → RESOLVED
Last Resolved: 2 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.