Closed Bug 1730426 Opened 3 years ago Closed 2 years ago

Strict SpiderMonkey Exception Lifetimes (partial)

Categories

(Core :: JavaScript Engine, task, P3)

task

Tracking

()

RESOLVED FIXED

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

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
Regressions: 1756592

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.

Flags: needinfo?(tcampbell)

Closing up old bugs with landed patches

Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(tcampbell)
Resolution: --- → FIXED
Summary: Strict SpiderMonkey Exception Lifetimes → Strict SpiderMonkey Exception Lifetimes (partial)
Attachment #9241035 - Attachment is obsolete: true
Attachment #9240844 - Attachment is obsolete: true
Attachment #9240845 - Attachment is obsolete: true
Attachment #9240846 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: