Improve coverage of simulated OOM testing

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
4 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

4 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

4 years ago
Posted patch 1 - oom-alloc-policy (obsolete) — Splinter Review
Add maybeSimulateOOM() method to all implementations of AllocPolicy.  This does nothing for the ones outside js/src.
(Assignee)

Comment 2

4 years ago
Posted patch 2 - oom-vector (obsolete) — Splinter Review
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

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

Comment 3

4 years ago
Posted patch 3 - oom-hashtable (obsolete) — Splinter Review
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

4 years ago
Posted patch 4 - fix-oomsSplinter Review
Fix OOM handling issues revealed by previous patches.
(Assignee)

Updated

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

Updated

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

Updated

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

Updated

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

Updated

4 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

4 years ago
Posted patch oom-alloc-policy v2 (obsolete) — Splinter Review
Good point, comments added.
Attachment #8655470 - Attachment is obsolete: true
Attachment #8657071 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

4 years ago
Keywords: leave-open
(Assignee)

Updated

4 years ago
Attachment #8655486 - Flags: checkin+
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

4 years ago
Attachment #8657071 - Attachment is obsolete: true
Attachment #8659223 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 12

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

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

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

4 years ago
Keywords: leave-open
(Assignee)

Updated

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

Updated

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