Closed Bug 1617527 Opened 4 months ago Closed 4 months ago

Crash in [@ JS::SetPendingExceptionAndStack]

Categories

(Core :: Security, defect)

Unspecified
Windows 10
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla75
Tracking Status
firefox-esr68 --- unaffected
firefox73 --- unaffected
firefox74 --- unaffected
firefox75 --- fixed

People

(Reporter: calixte, Assigned: bzbarsky)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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

Flags: needinfo?(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...

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.

Flags: needinfo?(sphink)

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.

Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Flags: needinfo?(bzbarsky)

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.

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.

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?

Flags: needinfo?(sphink)

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

Attachment #9128704 - Attachment description: Bug 1617527. Wrap the exception stack into the compartment SpiderMonkey expects it to be in. → Bug 1617527. Remove the same-compartment checks when setting an exception on a JSContext
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9364941c6d53
Remove the same-compartment checks when setting an exception on a JSContext r=sfink
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
You need to log in before you can comment on or make changes to this bug.