898617 - Assert that the OOM reporter callback does not set an exception

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: assert_no_exception_when_reporting_oom-v0.diff

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

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 2

[Terrence Cole [:terrence]]

Thanks, Bill!

https://hg.mozilla.org/integration/mozilla-inbound/rev/95cdd796f481

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/95cdd796f481