If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Improve coverage of simulated OOM testing

RESOLVED FIXED in Firefox 44

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(5 attachments, 4 obsolete attachments)

(Assignee)

Description

2 years ago
We have infrastructure to allow us to test handling of out of memory conditions by simulating OOM in debug builds.  This works by arranging to return an error condition from a specific memory allocation (or all allocations after a certain point).

We attempt to test all failure paths by running test code in a loop and failing at successive allocation points.  However this only tests failure on paths where was allocated in this execution, not all paths where memory might be allocated in a real execution.

For example, a vector allocates memory for many elements at the same time.  Currently we will only simulate OOM on those vector operations that cause it to allocate.  This means that there are many paths where vector operations may fail which are not exercised by our testing.

We can improve this situation by allowing our containers to simulate OOM on operations that might have but did not allocate memory.
(Assignee)

Comment 1

2 years ago
Created attachment 8655470 [details] [diff] [review]
1 - oom-alloc-policy

Add maybeSimulateOOM() method to all implementations of AllocPolicy.  This does nothing for the ones outside js/src.
(Assignee)

Comment 2

2 years ago
Created attachment 8655479 [details] [diff] [review]
2 - oom-vector

Simuate OOM in vector operations that may allocate memory.  Take care not to simulate OOM when growing to less than the inline size N as this is assumed not to fail.
(Assignee)

Updated

2 years ago
Attachment #8655479 - Attachment description: oom-vector → 2 - oom-vector
(Assignee)

Comment 3

2 years ago
Created attachment 8655484 [details] [diff] [review]
3 - oom-hashtable

Simulate OOM in hashtable operations that may allocate memory.  Add putNewInfallible() for populating sets and maps allocated with an initial capacity without the possibility of failure.
(Assignee)

Comment 4

2 years ago
Created attachment 8655486 [details] [diff] [review]
4 - fix-ooms

Fix OOM handling issues revealed by previous patches.
(Assignee)

Updated

2 years ago
Attachment #8655486 - Attachment description: 5 - fix-ooms → 4 - fix-ooms
(Assignee)

Updated

2 years ago
Attachment #8655470 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

2 years ago
Attachment #8655479 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

2 years ago
Attachment #8655484 - Flags: review?(terrence)
(Assignee)

Updated

2 years ago
Attachment #8655486 - Flags: review?(terrence)
Attachment #8655484 - Flags: review?(terrence) → review+
Attachment #8655486 - Flags: review?(terrence) → review+
Comment on attachment 8655470 [details] [diff] [review]
1 - oom-alloc-policy

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

Fine enough, but documentation.

::: mfbt/AllocPolicy.h
@@ +83,5 @@
>    void reportAllocOverflow() const
>    {
>    }
> +
> +  bool maybeSimulateOOM() const

This addition requires a corresponding update to the comment near the start of the file.  This patch, at least, nowhere clarifies what this method should do.  And future readers won't even be seeing this much of a patch.
Attachment #8655470 - Flags: review?(jwalden+bmo) → review-
Comment on attachment 8655479 [details] [diff] [review]
2 - oom-vector

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

I'm gonna hold off on reviewing this til I have clear documentation in hand for the new method, to measure the patch against that.
(Assignee)

Comment 7

2 years ago
Created attachment 8657071 [details] [diff] [review]
oom-alloc-policy v2

Good point, comments added.
Attachment #8655470 - Attachment is obsolete: true
Attachment #8657071 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

2 years ago
Keywords: leave-open

Comment 8

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

Updated

2 years ago
Attachment #8655486 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/c96fdda79727
Comment on attachment 8657071 [details] [diff] [review]
oom-alloc-policy v2

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

::: mfbt/AllocPolicy.h
@@ +47,5 @@
> + *      memory (failure simulation in cases where memory *is* allocated is handled
> + *      in the allocation methods of this class). This allows all possible
> + *      failure paths to be tested, not just those that actually caused an
> + *      allocation.
> + *      The default behaviour should be to always return false.

Fallible operations that fail when they return truthy are highly atypical.  This should be consistent with everything else, inverted.

Also, don't mention SpiderMonkey here -- mfbt is general-purpose and not tied to SpiderMonkey, so features, algorithms, data structures, etc. should be described in generic terms.

Putting that together, how about something like:

 * - bool checkSimulatedOOM() const
 *     Some clients generally allocate memory yet in some circumstances won't
 *     need to do so.  For example, appending to a vector with a small amount
 *     of inline storage generally allocates memory, but no allocation occurs
 *     unless appending exceeds inline storage.  But for testing purposes, it
 *     can be useful to treat *every* operation as allocating.  Clients (such
 *     as this hypothetical append method implementation) should call this
 *     method in situations that don't allocate, but could generally, to
 *     support this.  Default behavior should return true; more complicated
 *     behavior might be to return false only after a certain number of
 *     allocations-or-check-simulated-OOMs (coordinating with the other
 *     AllocPolicy methods) have occurred.

And then at a use site you might have

   if (!checkSimulatedOOM())
       return false;
Attachment #8657071 - Flags: review?(jwalden+bmo) → review-
(Assignee)

Comment 11

2 years ago
Created attachment 8659223 [details] [diff] [review]
1 - oom-alloc-policy v3
Attachment #8657071 - Attachment is obsolete: true
Attachment #8659223 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 12

2 years ago
Created attachment 8659224 [details] [diff] [review]
2 - oom-vector v2

Updated with review comments.
Attachment #8655479 - Attachment is obsolete: true
Attachment #8655479 - Flags: review?(jwalden+bmo)
Attachment #8659224 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 13

2 years ago
Created attachment 8659225 [details] [diff] [review]
3 - oom-hashtable v2

Updated with review comments.
Attachment #8655484 - Attachment is obsolete: true
Attachment #8659225 - Flags: review+
Comment on attachment 8659223 [details] [diff] [review]
1 - oom-alloc-policy v3

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

::: js/src/jsalloc.h
@@ +101,5 @@
>      JS_FRIEND_API(void) reportAllocOverflow() const;
> +
> +    bool checkSimulatedOOM() const {
> +        if (!js::oom::ShouldFailWithOOM())
> +            return true;

This negation is confusing, I'd probably do

  if (should fail with oom) {
     report oom
     return false;
  }

  return true

It feel to me like maybe ShouldFailWithOOM should also invert, somehow, but I can't think of quite the right name if it happened, that wouldn't be confusing the opposite way.  Meh, dilatory.
Attachment #8659223 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8659224 [details] [diff] [review]
2 - oom-vector v2

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

This is totally gonna screw up all the outstanding OOM bug oom-after numbers (probably required them to be bumped significantly).  So it goes.
Attachment #8659224 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 16

2 years ago
Created attachment 8665430 [details] [diff] [review]
5 - oom-additional-fixes

Fix a few more issues need to make all tests to pass with these patches:

- GCRuntime::startTask() needs to release the lock in the fallback path that runs the task on the main thread if it fails to start the task on a helper thread.

- Make IonCompile() return an error result if compilation threw and exception

- Check for OOM in Linker constructor in baseline compilation

- Remove assertion in ~AutoEnterOOMUnsafeRegion() that fails due to OOM testing not currently being threadsafe.  I'm working on this separately.
Attachment #8665430 - Flags: review?(terrence)
Comment on attachment 8665430 [details] [diff] [review]
5 - oom-additional-fixes

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

Nice!
Attachment #8665430 - Flags: review?(terrence) → review+
(Assignee)

Updated

2 years ago
Keywords: leave-open

Comment 18

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b75024c49a12
https://hg.mozilla.org/integration/mozilla-inbound/rev/c508580bb56d
https://hg.mozilla.org/integration/mozilla-inbound/rev/61535c12904e
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a2de2c3581a
https://hg.mozilla.org/mozilla-central/rev/b75024c49a12
https://hg.mozilla.org/mozilla-central/rev/c508580bb56d
https://hg.mozilla.org/mozilla-central/rev/61535c12904e
https://hg.mozilla.org/mozilla-central/rev/3a2de2c3581a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1209945
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1209943
Depends on: 1211100
You need to log in before you can comment on or make changes to this bug.