Closed Bug 1346012 Opened 3 years ago Closed 3 years ago

Crash in PromiseReactionRecord::setHandlerArg

Categories

(Core :: JavaScript Engine, defect, critical)

53 Branch
x86
Windows
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox52 --- unaffected
firefox-esr52 54+ fixed
firefox53 + wontfix
firefox54 + fixed
firefox55 + fixed

People

(Reporter: philipp, Assigned: till)

References

Details

(Keywords: crash, regression, sec-audit, Whiteboard: [post-critsmash-triage][adv-main54+][adv-esr52.2+])

Crash Data

Attachments

(3 files, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-c55a539f-82a5-471b-b93b-271132170309.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	PromiseReactionRecord::setHandlerArg(JS::Value&) 	js/src/builtin/Promise.cpp:226
1 	xul.dll 	EnqueuePromiseReactionJob 	js/src/builtin/Promise.cpp:470
2 	xul.dll 	TriggerPromiseReactions 	js/src/builtin/Promise.cpp:804
3 	xul.dll 	ResolvePromise 	js/src/builtin/Promise.cpp:588
4 	xul.dll 	RejectMaybeWrappedPromise 	js/src/builtin/Promise.cpp:784
5 	xul.dll 	RejectPromiseFunction 	js/src/builtin/Promise.cpp:325
6 	xul.dll 	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp:460
7 	xul.dll 	js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp:524
8 	xul.dll 	js::PromiseObject::reject(JSContext*, JS::Handle<JS::Value>) 	js/src/builtin/Promise.cpp:2618
9 	xul.dll 	JS::RejectPromise(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>) 	js/src/jsapi.cpp:4894
10 	xul.dll 	mozilla::dom::Promise::MaybeReject(JSContext*, JS::Handle<JS::Value>) 	dom/promise/Promise.cpp:288
11 	xul.dll 	AutoRejectPromise::~AutoRejectPromise() 	js/xpconnect/loader/mozJSSubScriptLoader.cpp:324
12 	xul.dll 	AsyncScriptLoader::OnStreamComplete(nsIIncrementalStreamLoader*, nsISupports*, nsresult, unsigned int, unsigned char const*) 	js/xpconnect/loader/mozJSSubScriptLoader.cpp:396
13 	xul.dll 	AsyncScriptLoader::OnStreamComplete(nsIIncrementalStreamLoader*, nsISupports*, nsresult, unsigned int, unsigned char const*) 	js/xpconnect/loader/mozJSSubScriptLoader.cpp:400
14 	xul.dll 	nsIncrementalStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) 	netwerk/base/nsIncrementalStreamLoader.cpp:98
15 	xul.dll 	nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, nsresult) 	netwerk/base/nsBaseChannel.cpp:830
16 	xul.dll 	nsInputStreamPump::OnStateStop() 	netwerk/base/nsInputStreamPump.cpp:714
17 	xul.dll 	nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) 	netwerk/base/nsInputStreamPump.cpp:433
18 	xul.dll 	nsInputStreamReadyEvent::Run() 	xpcom/io/nsStreamUtils.cpp:95
19 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1240
20 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:96
21 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:301
22 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:231
23 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:211
24 	xul.dll 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp:156
25 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp:262
26 	xul.dll 	XRE_RunAppShell() 	toolkit/xre/nsEmbedFunctions.cpp:924
27 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:269
28 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:231
29 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:211
30 	xul.dll 	XRE_InitChildProcess(int, char** const, XREChildData const*) 	toolkit/xre/nsEmbedFunctions.cpp:756
31 	xul.dll 	mozilla::BootstrapImpl::XRE_InitChildProcess(int, char** const, XREChildData const*) 	toolkit/xre/Bootstrap.cpp:65
32 	firefox.exe 	content_process_main(mozilla::Bootstrap*, int, char** const) 	ipc/contentproc/plugin-container.cpp:115
33 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:115
34 	firefox.exe 	__scrt_common_main_seh 	f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:253
35 	kernel32.dll 	BaseThreadInitThunk 	
36 	ntdll.dll 	__RtlUserThreadStart 	
37 	ntdll.dll 	_RtlUserThreadStart

these crash reports started occurring two days ago on 2017-03-07 - so far only on 53.0a2 and 53.0b1 on windows.

there isn't any correlation data generated for 53.0b1 yet, but going through some of the reports manually it appears that addons relating to avast are present in crashing installations quite frequently
[Tracking Requested - why for this release]:
signature is regressing in 53 and 1.24% of all content crashes on beta 1 last week.
Jason, can you help find someone to investigate this top crash on beta 53? Thanks.
Flags: needinfo?(jorendorff)
This is still showing up as a content crash in beta. Naveed can you help find someone to look into the issue?
Flags: needinfo?(nihsanullah)
Pretty high correlation to Avast:

100.0% in signature vs 03.43% overall) reason = EXCEPTION_ACCESS_VIOLATION_WRITE
(90.22% in signature vs 09.95% overall) Module "snxhk.dll" = true
(89.82% in signature vs 09.56% overall) Module "aswJsFlt.dll" = true
(87.37% in signature vs 46.37% overall) process_type = content [92.33% vs 21.80% if startup_crash = null]
(74.75% in signature vs 03.50% overall) Addon "Avast Online Security" = true
Till, maybe you can investigate this a bit more. We weren't sure in the platform triage today if this may be security-sensitive, so temporarily hiding the bug.
Group: javascript-core-security
Flags: needinfo?(till)
See Also: → 1347523
I looked into this, and it *might* be a dead proxy wrapper. However, bug 1347523 looks very similar, and there that explanation doesn't make much sense. I'll work on a patch that would enable teasing this apart by refactoring the code some and adding release asserts. I'll also explicitly handle dead wrappers, so in theory that could fix it.
Flags: needinfo?(nihsanullah)
Flags: needinfo?(jorendorff)
Flags: needinfo?(till)
This patch adds proper handling of dead wrappers to a few more places in Promise code. I'm skeptical that it'll actually fix things here, so I also added a few release asserts to at least narrow down where things go wrong. Not by much, because I don't know how, but at least a bit.
Attachment #8851197 - Flags: review?(shu)
Comment on attachment 8851197 [details] [diff] [review]
Handle dead object wrappers in more places in Promise code

Review of attachment 8851197 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like an explanation of dead wrapper + PromiseReactionRecord interaction for whether we need or do not need to call CheckedUnwrap first. Patch otherwise looks perfectly fine.

::: js/src/builtin/Promise.cpp
@@ +452,5 @@
> +    if (!IsProxy(reactionObj)) {
> +        MOZ_RELEASE_ASSERT(reactionObj->is<PromiseReactionRecord>());
> +        reaction = &reactionObj->as<PromiseReactionRecord>();
> +    } else {
> +        if (JS_IsDeadWrapper(reactionObj)) {

In nsXPCComponents_Utils::IsDeadWrapper, the use of JS_IsDeadWrapper uses CheckedUnwrap and has this comment:

    // Make sure to unwrap first. Once a proxy is nuked, it ceases to be a
    // wrapper, meaning that, if passed to another compartment, we'll generate
    // a CCW for it. Make sure that IsDeadWrapper sees through the confusion.

Do we need that call here as well? I'm not familiar with the use of wrappers on the reaction object here, unfortunately.
Attachment #8851197 - Flags: review?(shu)
I'm going to mark these sec-audit. It isn't clear how bad this actually is. Please clear if it is.
Keywords: sec-audit
We're going to need to land something ASAP if we want it in 53 (and apparently this only affects 53).
Flags: needinfo?(till)
Crashes starting in late 53 aurora (3/6 build?); no crashes on 54/55.
Till, do you want to land this diagnostic work in order to try and figure out what's happening here and in bug 1347523? Because the crash volume spike up in 53 beta I expect this may look like a worse problem once 53 goes to release.
Makes sense to track this for 54 and 55.
Based on the IRC discussion we had just now, this adds CheckedUnwrap to all the JS_IsDeadWrapper calls.
Attachment #8851197 - Attachment is obsolete: true
Flags: needinfo?(till)
Attachment #8855980 - Flags: review?(shu)
Attachment #8855980 - Flags: review?(shu) → review+
Comment on attachment 8855980 [details] [diff] [review]
Handle dead object wrappers in more places in Promise code. v2

Approval Request Comment
[Feature/Bug causing the regression]: Unknown
[User impact if declined]: Unclear, maybe more crashes, but quite likely nothing
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: No
[Why is the change risky/not risky?]: It adds some additional checks that might or might not help with diagnosing crashes. Those crashes look a lot like they're caused by interactions with external DLLs though, so I think it's not likely this will help much.
[String changes made/needed]: none

(As is perhaps obvious, I'm not entirely convinced that uplifting this makes sense. I also don't see much risk associated with it, though, so perhaps slim chances of it helping are enough?)
Attachment #8855980 - Flags: approval-mozilla-beta?
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e0bd14f23ac9 for mass crashes in js::IsDeadProxyObject - I don't know if there's any particular log that shows it better than the others, but all but one (Android mochitest-42) of https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=35e0a23e1c89bc55aa667915d5af41e006c9ed3f&filter-failure_classification_id=2 should be yours.
Comment on attachment 8855980 [details] [diff] [review]
Handle dead object wrappers in more places in Promise code. v2

Thanks for the patch. But let's hold off on uplift for 53.
Attachment #8855980 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Flags: needinfo?(till)
*Sigh*, so that went well :(

I relanded with the obvious bug fixed of not doing unchecked unwraps before detecting dead wrappers where unchecked unwraps are used otherwise.

Check-in: https://hg.mozilla.org/integration/mozilla-inbound/rev/75cb2fb13752

(Not sure why pulsebot isn't updating the bug.)

I won't ask for uplifts again, as I don't expect this to make a difference for beta one way or another.
Flags: needinfo?(till)
https://hg.mozilla.org/mozilla-central/rev/75cb2fb13752
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Hi :till,
Since this bug is a regression and also affects 54, do you consider to uplift this for 54?
Flags: needinfo?(till)
Approval Request Comment
[Feature/Bug causing the regression]: Unknown
[User impact if declined]: Probably little direct impact, this patch might help with debugging existing crashes
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: n/a
[String changes made/needed]: none
Flags: needinfo?(till)
Attachment #8859061 - Flags: review+
Attachment #8859061 - Flags: approval-mozilla-beta?
Attachment #8859061 - Flags: approval-mozilla-aurora?
Comment on attachment 8859061 [details] [diff] [review]
Handle dead object wrappers in more places in Promise code. v3

This patch might help debug existing crashes. Aurora54+.
Attachment #8859061 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8859061 - Flags: approval-mozilla-beta?
it looks like the crashes are now showing up under the signature [@ EnqueuePromiseReactionJob ] on 54.0b1 after the diagnostic patch has made it to beta.
Crash Signature: [@ PromiseReactionRecord::setHandlerArg] → [@ PromiseReactionRecord::setHandlerArg] [@ EnqueuePromiseReactionJob ]
See Also: → 1347984
(In reply to [:philipp] from comment #25)
> it looks like the crashes are now showing up under the signature [@
> EnqueuePromiseReactionJob ] on 54.0b1 after the diagnostic patch has made it
> to beta.

Yeah, so it's still very much unclear to me how this is possible. I carefully checked all code paths that add entries to the list of promise reactions that's iterated over here, and there's just no way to add anything to that that would fail this assert.

In doing so I realized that there's some optimization potential, I filed bug 1358879 for that. The patch there might or might not change things here, too.
Group: javascript-core-security → core-security-release
Should we blocklist the Avast addon or DLL?
Comment on attachment 8859061 [details] [diff] [review]
Handle dead object wrappers in more places in Promise code. v3

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: blocks backport of bug 1324140, which we want on esr52
User impact if declined: maybe exploitable crashes
Fix Landed on Version: 54
Risk to taking this patch (and alternatives if risky): small
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8859061 - Flags: approval-mozilla-esr52?
Attached patch Patch for esr52Splinter Review
[Approval Request Comment]
See comment 28.
Attachment #8864319 - Flags: review+
Attachment #8864319 - Flags: approval-mozilla-esr52?
Attachment #8859061 - Flags: approval-mozilla-esr52?
Comment on attachment 8864319 [details] [diff] [review]
Patch for esr52

This patch is needed for a sec-high bug, ESR52.2+
Attachment #8864319 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Setting tracking flag for the ESR52 release this is fixed in so people can tell when it was fixed.
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main54+][adv-esr52.2+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.