1617527 - Crash in [@ JS::SetPendingExceptionAndStack]

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Calixte Denizet (:calixte)]

This bug is for crash report bp-4e44adcb-374c-477f-b5a9-cf1510200222.

Top 10 frames of crashing thread:

0 xul.dll JS::SetPendingExceptionAndStack js/src/jsapi.cpp:4971
1 xul.dll xpc::FunctionForwarder js/xpconnect/src/ExportHelpers.cpp:398
2 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:569
3 xul.dll js::CallGetter js/src/vm/Interpreter.cpp:773
4 xul.dll js::NativeGetProperty js/src/vm/NativeObject.cpp:2631
5 xul.dll js::GetProperty js/src/vm/Interpreter.cpp:4456
6 xul.dll Interpret js/src/vm/Interpreter.cpp:2727
7 xul.dll js::RunScript js/src/vm/Interpreter.cpp:449
8 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:604
9 xul.dll InternalConstruct js/src/vm/Interpreter.cpp:679

There are 36 crashes (from 17 installations) in nightly 75 starting with buildid 20200222095208. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 267645.

[1] https://hg.mozilla.org/mozilla-central/rev?node=039e8fd2928d

Comment 1

[Boris Zbarsky [:bzbarsky]]

Well, that's silly. JS::GetPendingExceptionStack doesn't wrap into the current compartment, but JS::SetPendingExceptionAndStack asserts that what's passed in is current-compartment. So we're failing the cx->releaseCheck(stack) bit. I'll work around it, but the API is pretty questionable...

Comment 2

[Boris Zbarsky [:bzbarsky]]

And in particular, it's a little weird to need to wrap the stack into the current compartment if the callee is then just going to UncheckedUnwrap it anyway... and if we got it from this same JSContext, and while we were in this compartment, just a few lines up.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1617527. Remove the same-compartment checks when setting an exception on a JSContext

JS::GetPendingExceptionStack can return an object in some random (non-current)
compartment, but then JS::SetPendingExceptionAndStack expects a
current-compartment object. So using one followed by the other without jumping
through some hoops in between leads to compartment assertion failures.

Comment 4

[Bobby Holley (:bholley)]

Am I correct that, when the cx changes compartments, any pending exception does not? If so, then there should be no code making assumptions about the pending exception being same-compartment, and we should just removing these assertions instead.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Am I correct that, when the cx changes compartments, any pending exception does not?

You are correct, though JSContext::getPendingException wraps whatever the pending exception is into the current compartment before returning it.

But yes, there's no real reason to assert the compartment of the args to JS::SetPendingExceptionAndStack, imo.

Also, JSContext::setPendingException does its own check() call that maybe we should remove too, if we remove the checks here.

Comment 6

[Steve Fink [:sfink] [:s:]]

I'm not very familiar with the exception code, but the analysis here makes sense and I agree that the assertions should just be removed for all setPendingException*()s. One thing I still don't understand, though, is why getPendingException() calls setPendingException() with the wrapped exception and stack. What's the point? To save wrapper lookups?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Possibly... I'd have to dig through blame to see who added that and why.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9364941c6d53
Remove the same-compartment checks when setting an exception on a JSContext r=sfink

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/9364941c6d53