Closed Bug 1297858 Opened 8 years ago Closed 8 years ago

OOM in getPendingException is silenced by GetAndClearException in interpreter

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: mismith, Assigned: mismith, Mentored)

References

Details

Attachments

(1 file)

GetAndClearException in the interpreter calls cx->getPendingException to attempt to convert a pending exception into an object, then clears exceptions, then checks whether getPendingException OOM'd. If an OOM occurs in getPendingException, the OOM message is cleared by clearPendingException and the engine is left unsure of the source of the failure.

It's unclear to me why GetAndClearException would need to call clearPendingException before checking the return value of getPendingException, unless this is an artifact of a literal translation from the interpreter code that this function was factored out of: https://hg.mozilla.org/mozilla-central/rev/1ab05052b3b5
Patch attached.

I'm having trouble coming up with an isolated test case for this issue. It causes a test to fail with a patch I'm finishing up, but only if run with a non-debug build of SpiderMonkey. oomTest is unfortunately only available in debug builds.

Requesting a review from :jandem as the original author of this code.
Attachment #8784593 - Flags: review?(jdemooij)
Assignee: nobody → mismith
Status: NEW → ASSIGNED
Comment on attachment 8784593 [details] [diff] [review]
Bug 1297858: Check getPendingException status before clearing exception

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

Two reasons for the current code I think:

* getPendingException used to be infallible (it was when I added GetAndClearException).
* OOM used to be propagated by returning false *without setting any exception on the context*, the current code made more sense in that world.

Thanks for fixing, good find!
Attachment #8784593 - Flags: review?(jdemooij) → review+
Pushed by michael@spinda.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12d8a0ac1520
Check getPendingException status before clearing exception r=jandem
https://hg.mozilla.org/mozilla-central/rev/12d8a0ac1520
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: