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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[André Bargull [:anba]]

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.

Comment 1

[André Bargull [:anba]]

Attachment: Bug 1843499 - Part 1: Add ThrowWithStack and ExceptionAndStack opcodes. r=iain!

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.

Comment 2

[André Bargull [:anba]]

Attachment: Bug 1843499 - Part 2: Rethrow with exception stack in for-of loop. r=iain!

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

Comment 3

[André Bargull [:anba]]

Attachment: Bug 1843499 - Part 3: Add exception stack to finally. r=iain!

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

Comment 4

[André Bargull [:anba]]

Attachment: Bug 1843499 - Part 4: Add JSOp::AsyncReject. r=iain!

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

Comment 5

[André Bargull [:anba]]

Attachment: Bug 1843499 - Part 5: Remove AsyncFunctionResolveKind. r=iain!

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

Depends on D183565

Comment 6

[Pulsebot]

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

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8aafa58b6192
Backed out 6 changesets for causing xpcshell failures at test_bookmark_engine.js CLOSED TREE

Comment 8

[Cristina Horotan [:chorotan]]

Backed out 6 changesets (Bug 1843499) for causing xpcshell failures at test_bookmark_engine.js

Backout: https://hg.mozilla.org/integration/autoland/rev/8aafa58b619298c726ab56381e50ff4eeac7ade7

Failure push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=e991da5475e8cf0c85e10d7e29af946ff4f3d453&selectedTaskRun=Ijvy4h2eTgqkwhDbuDq65Q.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=422756381&repo=autoland&lineNumber=2261

Comment 9

[André Bargull [:anba]]

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.

Comment 10

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

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.

Comment 11

[Pulsebot]

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

Comment 12

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/f8b78b751f8e
https://hg.mozilla.org/mozilla-central/rev/1a03aca53b24
https://hg.mozilla.org/mozilla-central/rev/b2c376db1d62
https://hg.mozilla.org/mozilla-central/rev/27944ef8cc57
https://hg.mozilla.org/mozilla-central/rev/1782607d6384