Closed Bug 1843499 Opened 1 year ago Closed 11 months ago

Don't clobber exception stack when re-throwing non-ErrorObject values

Categories

(Core :: JavaScript Engine, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
124 Branch
Tracking Status
firefox117 --- wontfix
firefox124 --- fixed

People

(Reporter: anba, Assigned: anba)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

When a non-ErrorObject value is re-thrown in for-of loops or in try-finally statements, the original exception stack is clobbered. This can give a bad user experience.

I've encountered this in test262 when an assertion in https://github.com/tc39/test262/blob/main/test/staging/ArrayBuffer/resizable/subarray.js failed. The test contains a for-of and in the loop's body, multiple test assertions are used. One of those assertions failed for me and the exception stack unhelpfully pointed to the last line of the for-of loop. Only after rewriting the for-of loop into a normal C-style for loop I was able to find which assertion failed, because then the exception stack was correct. Test262 throws Test262Error when an assertion fails and Test262Error isn't an Error subclass, cf. https://github.com/tc39/test262/blob/main/harness/sta.js.

Add JSOp::ExceptionAndStack to retrieve the exception stack in addition to
the exception value. And add JSOp::ThrowWithStack to throw an exception with
an exception stack.

The next part will use both new opcodes.

The exception stack trace is discarded when using the opcodes JSOp::Exception
and JSOp::Throw. This is visible when throwing values which aren't instances
of ErrorObject.

This happens for example in test262, because Test262Error isn't an Error
subclass.

Example using test262 assertions:

for (let value of [0]) {
  assert.throws(TypeError, () => {});  // This is line 2.

  assert.sameValue(value, 0);  // This is line 4.
}

Before this patch:

/tmp/a.js:4:10 uncaught exception: Test262Error: Expected a TypeError to be thrown but no exception was thrown at all
Stack:
  @/tmp/a.js:4:10

With this patch:

/tmp/test262-assert.js:104:9 uncaught exception: Test262Error: Expected a TypeError to be thrown but no exception was thrown at all
Stack:
  assert.throws@/tmp/test262-assert.js:104:9
  @/tmp/a.js:2:10

Depends on D183562

Similar to the last part 2, re-throw the original stack trace in finally
blocks. This is implemented by changing finally blocks to push the exception
stack onto the value stack.

Using test262 assertions:

try {
  assert.throws(TypeError, () => {});  // This is line 2.
} finally {
  assert.sameValue(0, 0);  // This is line 4. (End of finally block)
}

Before:

/tmp/a.js:4:10 uncaught exception: Test262Error: Expected a TypeError to be thrown but no exception was thrown at all
Stack:
  @/tmp/a.js:4:10

Now:

/tmp/test262-assert.js:104:9 uncaught exception: Test262Error: Expected a TypeError to be thrown but no exception was thrown at all
Stack:
  assert.throws@/tmp/test262-assert.js:104:9
  @/tmp/a.js:2:10

Depends on D183563

Split rejection handling from JSOp::AsyncResolve into a new JSOp::AsyncReject
operation. Similar to the last parts, the new JSOp::AsyncReject opcode also
keeps the original exception stack trace.

Depends on D183564

JSOp::AsyncResolve is now always called with AsyncFunctionResolveKind::Fulfill,
so we can remove the AsyncFunctionResolveKind operand.

Depends on D183565

Pushed by andre.bargull@gmail.com: https://hg.mozilla.org/integration/autoland/rev/f070dd9807b3 Part 1: Add ThrowWithStack and ExceptionAndStack opcodes. r=iain https://hg.mozilla.org/integration/autoland/rev/9eed95f5cacc Part 2: Rethrow with exception stack in for-of loop. r=iain https://hg.mozilla.org/integration/autoland/rev/08e2589b2dcc Part 3: Add exception stack to finally. r=iain https://hg.mozilla.org/integration/autoland/rev/0250cdf11658 Part 4: Add JSOp::AsyncReject. r=iain https://hg.mozilla.org/integration/autoland/rev/b777b54accce Part 5: Remove AsyncFunctionResolveKind. r=iain https://hg.mozilla.org/integration/autoland/rev/e991da5475e8 apply code formatting via Lando
Backout by chorotan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8aafa58b6192 Backed out 6 changesets for causing xpcshell failures at test_bookmark_engine.js CLOSED TREE

I thought I could get away with not having to worry about wrappers, because all values are on the stack, but I forgot about generators storing the stack state in an Array.

Flags: needinfo?(andrebargull)
Blocks: sm-runtime
Severity: -- → N/A
Priority: -- → P2

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:anba, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Flags: needinfo?(iireland)
Flags: needinfo?(andrebargull)
Flags: needinfo?(iireland)
Flags: needinfo?(andrebargull)
Pushed by andre.bargull@gmail.com: https://hg.mozilla.org/integration/autoland/rev/f8b78b751f8e Part 1: Add ThrowWithStack and ExceptionAndStack opcodes. r=iain https://hg.mozilla.org/integration/autoland/rev/1a03aca53b24 Part 2: Rethrow with exception stack in for-of loop. r=iain https://hg.mozilla.org/integration/autoland/rev/b2c376db1d62 Part 3: Add exception stack to finally. r=iain https://hg.mozilla.org/integration/autoland/rev/27944ef8cc57 Part 4: Add JSOp::AsyncReject. r=iain https://hg.mozilla.org/integration/autoland/rev/1782607d6384 Part 5: Remove AsyncFunctionResolveKind. r=iain
Regressions: 1875894
Regressions: 1878261
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: