1346012 - Crash in PromiseReactionRecord::setHandlerArg

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[[:philipp]]

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

Comment 1

[[:philipp]]

[Tracking Requested - why for this release]:
signature is regressing in 53 and 1.24% of all content crashes on beta 1 last week.

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Jason, can you help find someone to investigate this top crash on beta 53? Thanks.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

This is still showing up as a content crash in beta. Naveed can you help find someone to look into the issue?

Comment 4

[Marcia Knous [:marcia]]

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

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 6

[Till Schneidereit [:till]]

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.

Comment 7

[Till Schneidereit [:till]]

Attachment: Handle dead object wrappers in more places in Promise code

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.

Comment 8

[Shu-yu Guo [: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.

Comment 9

[Andrew McCreight [:mccr8]]

I'm going to mark these sec-audit. It isn't clear how bad this actually is. Please clear if it is.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

We're going to need to land something ASAP if we want it in 53 (and apparently this only affects 53).

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Crashes starting in late 53 aurora (3/6 build?); no crashes on 54/55.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 13

[Marcia Knous [:marcia]]

Makes sense to track this for 54 and 55.

Comment 14

[Till Schneidereit [:till]]

Attachment: Handle dead object wrappers in more places in Promise code. v2

Based on the IRC discussion we had just now, this adds CheckedUnwrap to all the JS_IsDeadWrapper calls.

Comment 15

[Till Schneidereit [:till]]

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?)

Comment 16

[Till Schneidereit [:till]]

Check-in: https://hg.mozilla.org/integration/mozilla-inbound/rev/35e0a23e1c89

Comment 17

[Phil Ringnalda (:philor)]

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 18

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 19

[Till Schneidereit [: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.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/75cb2fb13752

Comment 21

[Gerry Chang [:gchang]]

Hi :till,
Since this bug is a regression and also affects 54, do you consider to uplift this for 54?

Comment 22

[Till Schneidereit [:till]]

Attachment: Handle dead object wrappers in more places in Promise code. v3

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

Comment 23

[Gerry Chang [:gchang]]

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+.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4b43e1c02d4ff32f7d45a6c614720660f52927d9

Comment 25

[[:philipp]]

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.

Comment 26

[Till Schneidereit [:till]]

(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.

Comment 27

[Marco Castelluccio [:marco]]

Should we blocklist the Avast addon or DLL?

Comment 28

[Till Schneidereit [:till]]

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.

Comment 29

[Till Schneidereit [:till]]

Attachment: Patch for esr52

[Approval Request Comment]
See comment 28.

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8864319 [details] [diff] [review]
Patch for esr52

This patch is needed for a sec-high bug, ESR52.2+

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/1ce6d0652921

Comment 32

[Al Billings [:abillings - ex-MoCo]]

Setting tracking flag for the ESR52 release this is fixed in so people can tell when it was fixed.