1730426 - Strict SpiderMonkey Exception Lifetimes (partial)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Ted Campbell [:tcampbell]]

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.

Comment 1

[Ted Campbell [:tcampbell]]

Attachment: Bug 1730426 - Remove extra ReportOverRecursed argument. r?jandem!

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

Comment 2

[Ted Campbell [:tcampbell]]

Attachment: Bug 1730426 - Add JS::ExceptionStatus enum to track JSContext exception state. r?jandem!

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

Comment 3

[Ted Campbell [:tcampbell]]

Attachment: Bug 1730426 - Add OutOfMemory to JS::ExceptionStatus. r?jandem!

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

Comment 4

[Ted Campbell [:tcampbell]]

Attachment: Bug 1730426 - Fix testSlowScript JSAPI test. r?jandem!

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

Comment 5

[Ted Campbell [:tcampbell]]

Attachment: Bug 1730426 - Add testcase for OOM during Bailout. r?jandem!

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

Depends on D125356

Comment 6

[Ted Campbell [:tcampbell]]

Attachment: Bug 1730426 - Stop leaking exceptions in TypeArray jsapi-test. r?sfink!

Depends on D125357

Comment 7

[Ted Campbell [:tcampbell]]

Attachment: Bug 1730426 - Use SystemAllocPolicy for DebuggerFrameVector. r?jandem!

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

Comment 8

[Ted Campbell [:tcampbell]]

Attachment: Bug 1730426 - Save exception before calling jit::BailoutIonToBaseline. r?jandem!

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

Comment 9

[Ted Campbell [:tcampbell]]

Attachment: Bug 1730426 - Do not call JS_ReportError* when exception is pending. r?sfink!,rhunt!

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

Depends on D125360

Comment 10

[Ted Campbell [:tcampbell]]

Attachment: Bug 1730426 - Remove redundant ReportOutOfMemory calls. r?jandem!

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

Comment 11

[Ted Campbell [:tcampbell]]

Attachment: Bug 1730426 - Start explicitly throwing uncatchable exceptions. r?jandem!

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

Comment 12

[Ted Campbell [:tcampbell]]

Attachment: WIP: Bug 1730426 - Enforce strict spidermonkey exception handling

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

Comment 13

[Ted Campbell [:tcampbell]]

Attachment: WIP: Bug 1730426 - Require explicit AutoSaveExceptionState::drop calls

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

Comment 14

[Ted Campbell [:tcampbell]]

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.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d5fb224e0e0
Backed out 11 changesets for causing assertion failures in src/vm/JSContext.

Comment 17

[Ted Campbell [:tcampbell]]

Attachment: Bug 1730426 - AudioWorklets should explicitly drop interrupts. r?padenot!

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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Sandor Molnar[:smolnar]]

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

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 22

[Ted Campbell [:tcampbell]]

Closing up old bugs with landed patches