Assert that the OOM reporter callback does not set an exception

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Use of an AutoCompartment inside of a NoGC block is generally unsafe. If the other-compartment code throws an exception, then ~AutoCompartment will wrap the exception. This may trigger GC in a location that is unsafe.

However, if the other-compartment code may only throw OOM, then this is safe. OOM only sets an exception to be an Atom, or calls the reportOOM callback. Atoms do not need to be wrapped, so no further allocation or other fallible code is required. The reportOOM callback should never set an exception, so again no wrapping is required in ~AutoCompartment. This bug is adding an assertion that reportOOM really does not ever set an exception.

Try, at least, is green with this assertion:
https://tbpl.mozilla.org/?tree=Try&rev=4156acf94736
Attachment #781915 - Flags: review?(wmccloskey)
Comment on attachment 781915 [details] [diff] [review]
assert_no_exception_when_reporting_oom-v0.diff

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

Just to be clear, the callback in question is the normal error reporter. There's no such thing as reportOOM.

::: js/src/jscntxt.cpp
@@ +403,5 @@
>          AutoSuppressGC suppressGC(cx);
>          onError(cx, msg, &report);
>      }
> +
> +    JS_ASSERT(!cx->isExceptionPending());

Please put a comment above here saying something like:

"We would like to enforce the invariant that any exception reported during an OOM situation does not require wrapping. Besides avoiding allocation when memory is low, this reduces the number of places where we might need to GC.

When JS code is running, we set the pending exception to an atom, which does not need wrapping. If no JS code is running, no exception should be set at all."
Attachment #781915 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/95cdd796f481
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.