OOM in getPendingException is silenced by GetAndClearException in interpreter

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mismith, Assigned: mismith, Mentored)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

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
Created attachment 8784593 [details] [diff] [review]
Bug 1297858: Check getPendingException status before clearing exception

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+

Comment 4

2 years ago
Pushed by michael@spinda.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12d8a0ac1520
Check getPendingException status before clearing exception r=jandem

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/12d8a0ac1520
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.