1115005 - Consider adding more !cx->isExceptionPending() asserts

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Right now we assert this only in a few places. I added some asserts to the frontend and that seems to have caught a bunch of issues (bug 1106543 for instance, fuzzers have many more of them), but those asserts are annoying to fix because a lot of code can run between the actual bug (ignoring a pending exception) and the assertion failure.

If we added more of those asserts (like whenever we enter or leave the interpreter, JIT compilation, parser, GC, important VM functions) it'd be a lot easier for the fuzzers to create (small) tests for this.

Comment 1

[Jan de Mooij [:jandem]]

The static analysis should help with this too, but we can do both of course.

Potential risk is introducing more orange...

Comment 2

[Terrence Cole [:terrence]]

The browser is terrible about handling errors -- much worse than SM internals -- so adding this assertion to our normal suite of API-level assertions might be sufficient.

Comment 7

[Terrence Cole [:terrence]]

Nope: the Try run for the browser is just very dirty and I haven't had a free span of days to start tackling the issues.

Comment 8

[Jesse Ruderman]

I'm surprised to see hidden comments on this bug. When I see a "!cx->isExceptionPending()" assertion, should I assume it's a security hole?

Comment 9

[Terrence Cole [:terrence]]

Getting better: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ee5ab456b4e

(In reply to Jesse Ruderman from comment #8)
> I'm surprised to see hidden comments on this bug. When I see a
> "!cx->isExceptionPending()" assertion, should I assume it's a security hole?

It depends what the stack is. Probably not in general -- the cases discussed above were somewhat special because we do not normally conflate failure and retry.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Marking this [fuzzblocker] not because it is a fuzzblocker directly, but it is making us ignore cx->IsExceptionPending asserts and its variants due to their current unreliability. Having this fixed could allow us to have more of such asserts that are reliable.

Comment 11

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc383504624e

Comment 12

[Terrence Cole [:terrence]]

Errormageddon should completely obviate this and fix all dependent bugs.

Comment 13

[Marco Castelluccio [:marco]]

(In reply to Terrence Cole [:terrence] from comment #12)

Errormageddon should completely obviate this and fix all dependent bugs.

Bug 1277368 was fixed, is this still relevant or can we close it?

Comment 14

[Jan de Mooij [:jandem]]

(In reply to Marco Castelluccio [:marco] from comment #13)

(In reply to Terrence Cole [:terrence] from comment #12)

Errormageddon should completely obviate this and fix all dependent bugs.

Bug 1277368 was fixed, is this still relevant or can we close it?

Still relevant. Bug 1277368 added the initial Result type but we're not using it a lot in the VM.

Comment 15

[Jason Kratzer [:jkratzer]]

We are no longer bound by the limitations of comment 10. Removing fuzzblocker from the whiteboard.