Closed Bug 1201057 Opened 6 years ago Closed 6 years ago

Don't simulate OOM in places where we will crash with CrashAtUnhandlableOOM()


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox44 --- fixed


(Reporter: jonco, Assigned: jonco)



(3 files)

There are many spots in the engine where for one reason or another we crash if we fail to allocate memory by CrashAtUnhandlableOOM().

When running a test which simulates OOM, doing so in these places will cause the engine to exit straight away and abort the test, and we currently have to allow this.  This has the potential to hide errors though as later allocation points are not tested.

We already have AutoEnterOOMUnsafeRegion and we can use this to disable OOM simulation where necessary.
Attached patch move-oom-guardSplinter Review
Move the definition of AutoEnterOOMUnsafeRegion to Utility.h where all the rest of the OOM simulation infrastructure is.

Fix warnings when this is pulled into gecko that revealed bad implicit type conversions between 64 and 32 bits.
Attachment #8656039 - Flags: review?(terrence)
Use AutoEnterOOMUnsafeRegion to disable OOM simulation where we will just crash anyway.

I tried to keep the scope of the guard as small as reasonably possible but there are a couple of places where it made more sense to have it live for a longer time, notably JSObject::swap() and LoopUnroller::go().

This removes almost all the direct calls to CrashAtUnhandlableOOM().  There are a few remaining uses which could do with further investigation, but these were more complicated than a simple OOM:
 - jit::ExceptionHandlerBailout()
 - SnapshotIterator::maybeRead()
 - BOffImm::BOffImm()
Attachment #8656054 - Flags: review?(terrence)
Attached patch update-oom-testsSplinter Review
Update OOM tests to not specify --allow-unhandlable-oom now we don't expect to hit this.  Ditto for --no-ggc, which was there because GGC hits an unhandlable OOM as soon as we do a minor GC.  Add --no-threads because none of this infrastructure is thread safe yet and we want reliable test results.
Attachment #8656059 - Flags: review?(terrence)
Attachment #8656039 - Flags: review?(terrence) → review+
Comment on attachment 8656054 [details] [diff] [review]

Review of attachment 8656054 [details] [diff] [review]:

This is very nice! Sadly, I just checked in a couple more CrashAtUnhandlableOOM with the stable hash codes patch -- sorry about that. :-(
Attachment #8656054 - Flags: review?(terrence) → review+
Attachment #8656059 - Flags: review?(terrence) → review+
You need to log in before you can comment on or make changes to this bug.