Strict SpiderMonkey Exception Lifetimes (partial)
Categories
(Core :: JavaScript Engine, task, P3)
Tracking
()
People
(Reporter: tcampbell, Assigned: tcampbell)
References
(Blocks 1 open bug)
Details
Attachments
(10 files, 4 obsolete files)
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
We should experiment with using strict exception lifetimes in order to make
things like JIT Bailouts and the Debugger easier to reason about. As a general
principal we should avoid silently clobbering exception statuses and instead
make it explicit whether we are preserving the older or new exception. Exception
handling is already hard enough as it is to get test coverage, so reducing the
number of edge cases we let sneak in should de-risk it further.
While we can fix the violations in SpiderMonkey, it may make sense for this
checks to be optional to avoid causing too much grief to embedders. Double
throwing has previously worked fine and it is an easy mistake to make (as
evidenced by dozens of defects in SpiderMonkey itself).
-
Invariant 1: Interrupt (aka "uncatchable") exceptions should be noted on the
JSContext before propagating return-false/nullptr. This lets us distinguish an
Interrupt from simply forgetting to set an exception. It also makes it easier to
detect the exception status being clobbered. -
Invariant 2: Ok / ForcedReturn / Throwing / Interrupt are mutually exclusive
statuses. This should be represented with an enum on the JSContext instead of
current collection of bools. This already holds in nearly all cases, but using
an enum lets us avoid mistakes and documents intent. -
Invariant 3: An exception status should not be implicitly clobbered. For
example, thesetPendingException
function must only be called if status is Ok.
In the few cases we want to replace an existing exception, the caller should
explicitly save or drop it first. This makes the code clearly show how
exceptions are prioritized (which depends on the operation).
Additionally, the AutoSaveExceptionState machinery probably should be cleaned up
to better document the different uses. If possible, the last-return-value should
be passed to it to better synchronize uncatchable exception cases.
Assignee | ||
Comment 1•3 years ago
|
||
The last use of this extra argument was removed in Bug 1303079.
Assignee | ||
Comment 2•3 years ago
|
||
Combine the PropagatingForcedReturn, Throwing, and OverRecursed flags into a
single enumeration. Throwing and PropagatingForcedReturn are already mutually
exclusive status. OverRecursed state implies Throwing, so add a helper for
querying if the status represents throwing.
The clearPendingException
call will now also clear forced-return status. No
code hits this case, but I think this new definition is more consistent.
It is also no longer possible to generate exceptions that are both OOM and
OverRecursed at the same time (yikes!).
Depends on D125353
Assignee | ||
Comment 3•3 years ago
|
||
Similar to OverRecursed, directly track OutOfMemory on ExceptionStatus. The
exception still continues to turn into the "out of memory" string, but while
propagating up the call stack it has a special status. This doesn't really
change too much, but both OutOfMemory and OverRecursed are internal exceptions
that are often checked for.
Depends on D125354
Assignee | ||
Comment 4•3 years ago
|
||
This test was missing from moz.build so it has not been running for long enough
that the JavaScript in it is no longer even legal JS.
Depends on D125355
Assignee | ||
Comment 5•3 years ago
|
||
This oomTest covers snapshot recovery during a bailout, including inlined
frames. Previous test coverage was spotty and was platform dependent.
Depends on D125356
Assignee | ||
Comment 6•3 years ago
|
||
Depends on D125357
Assignee | ||
Comment 7•3 years ago
|
||
Explicitly call ReportOutOfMemory from callers of getDebuggerFrames instead of
implicitly by the Vector. Also clear pending exception in slowPathOnLeaveFrame
before raising the OOM if getDebuggerFrames fails.
Depends on D125358
Assignee | ||
Comment 8•3 years ago
|
||
Save the exception state of the ExceptionHandlerBailout before calling the
normal bailout code. If the the bailout fails, we drop the error that triggered
the bailout and instead use the error that caused bailout to fail (which is
probably OOM). This is implicitly what the previous code did, but this avoids
implicitly clobbering exceptions.
Depends on D125359
Assignee | ||
Comment 9•3 years ago
|
||
Avoid throwing custom errors in cases when an appropriate error is already being
thrown.
Depends on D125360
Assignee | ||
Comment 10•3 years ago
|
||
Remove ReportOutOfMemory calls that are redundant. Also add a few missing calls
that were previously covered by a redundant call. Later patches will add asserts
to check for these redundancies. This patch should not change behaviour since
the out-of-memory exception continues to be thrown (just now only once).
Depends on D125361
Assignee | ||
Comment 11•3 years ago
|
||
Add Interrupt status to JS::ExceptionStatus and set it when intentionally
throwing an uncatchable exception. This is only a guideline for now, so avoid
excessive asserts for now.
Depends on D125362
Assignee | ||
Comment 12•3 years ago
|
||
Check that a new exception is not thrown while one is already pending. Instead
the existing one should be saved and then either the new or old one dropped.
Check that uncatchable are explicitly noted on the JSContext. Add various checks
that the return value is consistent with the JSContext exception status. This is
a breaking change, but earlier patches fix SpiderMonkey and Gecko use cases.
Depends on D125363
Assignee | ||
Comment 13•3 years ago
|
||
Stop allowing a saved exception to be implicitly dropped. Add explicit calls to
drop() when a saved exception is being discarded in favour of a new exception.
Previously, the AutoSaveExceptionState destructor would unintentionally clobber
an uncatchable exception with a catchable one. This only occured with edge cases
of the Debugger API, but fixing this makes things easier to reason about.
Depends on D125364
Assignee | ||
Comment 14•3 years ago
|
||
I've moved the asserts to the end (in the WIP) patches since I'm not sure if we want to make this a hard requirement yet. The rest of the stack is fixing a variety of minor defects as well as adding JS::ExceptionStatus to represent current exception status on the JSContext.
Updated•3 years ago
|
Comment 15•3 years ago
|
||
Comment 16•3 years ago
|
||
Assignee | ||
Comment 17•3 years ago
|
||
Subsequent patches will try to track that JS interrupts are explictly raised and
explicitly cleared. This patch ensures that AudioWorklets clear from the
JSContext. This prevents faults when multiple tracks are cancelled at once.
Depends on D125362
Comment 18•3 years ago
|
||
Comment 19•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/700f0094b4fa
https://hg.mozilla.org/mozilla-central/rev/e0c797249e6f
https://hg.mozilla.org/mozilla-central/rev/9ef7808e2e87
https://hg.mozilla.org/mozilla-central/rev/ee20919479fc
https://hg.mozilla.org/mozilla-central/rev/04c04b79e89c
https://hg.mozilla.org/mozilla-central/rev/6fc5fa8a7ae8
https://hg.mozilla.org/mozilla-central/rev/2b60185b24f9
https://hg.mozilla.org/mozilla-central/rev/d48caedacbb1
https://hg.mozilla.org/mozilla-central/rev/0c581cb43a7c
https://hg.mozilla.org/mozilla-central/rev/ed6ca0884441
Comment 21•2 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:tcampbell, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.
Assignee | ||
Comment 22•2 years ago
|
||
Closing up old bugs with landed patches
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Description
•