Open Bug 1730426 Opened 3 months ago Updated 4 days ago

Strict SpiderMonkey Exception Lifetimes

Categories

(Core :: JavaScript Engine, task, P3)

task

Tracking

()

People

(Reporter: tcampbell, Assigned: tcampbell)

References

(Blocks 1 open bug)

Details

(Keywords: leave-open)

Attachments

(14 files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

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, the setPendingException 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.

The last use of this extra argument was removed in Bug 1303079.

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

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

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

This oomTest covers snapshot recovery during a bailout, including inlined
frames. Previous test coverage was spotty and was platform dependent.

Depends on D125356

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

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

Avoid throwing custom errors in cases when an appropriate error is already being
thrown.

Depends on D125360

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

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

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

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

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.

See Also: → 341962, 552007
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9e42f1860492
Remove extra ReportOverRecursed argument. r=jandem
https://hg.mozilla.org/integration/autoland/rev/d89e8415905e
Add JS::ExceptionStatus enum to track JSContext exception state. r=jandem
https://hg.mozilla.org/integration/autoland/rev/8363cf2dbda3
Add OutOfMemory to JS::ExceptionStatus. r=jandem
https://hg.mozilla.org/integration/autoland/rev/a22343e12ce4
Fix testSlowScript JSAPI test. r=jandem
https://hg.mozilla.org/integration/autoland/rev/b28009dee3ff
Add testcase for OOM during Bailout. r=jandem
https://hg.mozilla.org/integration/autoland/rev/4d27b63a0250
Stop leaking exceptions in TypeArray jsapi-test. r=sfink
https://hg.mozilla.org/integration/autoland/rev/bf394cc4b7f3
Use SystemAllocPolicy for DebuggerFrameVector. r=jandem
https://hg.mozilla.org/integration/autoland/rev/76ff766c5cdf
Save exception before calling jit::BailoutIonToBaseline. r=jandem
https://hg.mozilla.org/integration/autoland/rev/d2dadbfb2cca
Do not call JS_ReportError* when exception is pending. r=sfink,rhunt
https://hg.mozilla.org/integration/autoland/rev/f1c824d4d39c
Remove redundant ReportOutOfMemory calls. r=jandem
https://hg.mozilla.org/integration/autoland/rev/ef37ce0219ea
Start explicitly throwing uncatchable exceptions. r=jandem
Backout by smolnar@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2d5fb224e0e0
Backed out 11 changesets for causing assertion failures in src/vm/JSContext.

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

Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/700f0094b4fa
Remove extra ReportOverRecursed argument. r=jandem
https://hg.mozilla.org/integration/autoland/rev/e0c797249e6f
Add JS::ExceptionStatus enum to track JSContext exception state. r=jandem
https://hg.mozilla.org/integration/autoland/rev/9ef7808e2e87
Add OutOfMemory to JS::ExceptionStatus. r=jandem
https://hg.mozilla.org/integration/autoland/rev/ee20919479fc
Fix testSlowScript JSAPI test. r=jandem
https://hg.mozilla.org/integration/autoland/rev/04c04b79e89c
Add testcase for OOM during Bailout. r=jandem
https://hg.mozilla.org/integration/autoland/rev/6fc5fa8a7ae8
Stop leaking exceptions in TypeArray jsapi-test. r=sfink
https://hg.mozilla.org/integration/autoland/rev/2b60185b24f9
Use SystemAllocPolicy for DebuggerFrameVector. r=jandem
https://hg.mozilla.org/integration/autoland/rev/d48caedacbb1
Save exception before calling jit::BailoutIonToBaseline. r=jandem
https://hg.mozilla.org/integration/autoland/rev/0c581cb43a7c
Do not call JS_ReportError* when exception is pending. r=sfink,rhunt
https://hg.mozilla.org/integration/autoland/rev/ed6ca0884441
Remove redundant ReportOutOfMemory calls. r=jandem
Regressions: 1733899
You need to log in before you can comment on or make changes to this bug.