Closed
Bug 898617
Opened 11 years ago
Closed 11 years ago
Assert that the OOM reporter callback does not set an exception
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: terrence, Assigned: terrence)
References
Details
Attachments
(1 file)
(deleted),
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
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+
Assignee | ||
Comment 2•11 years ago
|
||
Comment 3•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in
before you can comment on or make changes to this bug.
Description
•