1443544 - Crash in "AsyncShutdownTimeout | profile-before-change | Native Messaging: Wait for application * to exit" with multiple applications

Status: RESOLVED FIXED

(External Software Affecting Firefox :: Other, defect, P1)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-f9f99e1a-d684-4d97-a138-385ea0180306.
=============================================================

Seen while looking at release crash data: http://bit.ly/2Fg5unF. Affects 58 and 59.

(100.0% in signature vs 03.19% overall) Addon "McAfee SiteAdvisor" = true

Top 10 frames of crashing thread:

0 mozglue.dll mozalloc_abort memory/mozalloc/mozalloc_abort.cpp:33
1 xul.dll Abort xpcom/base/nsDebugImpl.cpp:461
2 xul.dll NS_DebugBreak xpcom/base/nsDebugImpl.cpp:448
3 xul.dll nsDebugImpl::Abort xpcom/base/nsDebugImpl.cpp:148
4 xul.dll NS_InvokeByIndex xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_x86_msvc.asm:54
5 xul.dll XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:1282
6 xul.dll XPC_WN_CallMethod js/xpconnect/src/XPCWrappedNativeJSOps.cpp:929
7 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:473
8 xul.dll InternalCall js/src/vm/Interpreter.cpp:522
9 xul.dll Interpret js/src/vm/Interpreter.cpp:3098

=============================================================

Comment 1

[Marco Castelluccio [:marco]]

Luca, can you take a look here or redirect? Even if there is a bug in the McAfee extension, I guess we should just stop waiting at some point.

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Forwarding to aswan who worked on the nativeMessaging API code and may have some ideas.

Comment 3

[Andrew Swan [:aswan]]

At shutdown we close the pipe to the native process and give the native process some time to exit.  If it doesn't exit cleanly we kill it and continue to wait for it to exit.  I don't know the specific Windows APIs involved but I suspect McAfee does something to block other processes from killing it.

We could make the browser just exit in this case without waiting for the native process, but that would be a shame (other applications that exit properly but just take a while wouldn't necessarily get cleaned up properly).  Perhaps somebody who knows Windows better could tell if we can detect the case that our attempt to kill the process has been blocked.  In that case we can just give up.  Or McAfee could fix their native application.

Comment 4

[Andrew Swan [:aswan]]

Kris, maybe we could check the return value here?
https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/toolkit/modules/subprocess/subprocess_worker_win.js#341

Comment 5

[Marco Castelluccio [:marco]]

We have a lot of shutdown crashes for extensions waiting for native applications to exit. Andrew, if the fix is as simple as you said, perhaps we could try it?

Comment 6

[Andrew Swan [:aswan]]

I'm not sure, that's a question for somebody who knows Windows more deeply than I do.
You say native application_s_, are there others besides McAfee that are causing shutdown crashes?

Comment 7

[Andrew Swan [:aswan]]

(In reply to Andrew Swan [:aswan] from comment #6)
> You say native application_s_, are there others besides McAfee that are
> causing shutdown crashes?

Er, I replied before noticing all the crash signatures that were added.  It looks like the answer is yes, there are several different native applications affected.

Still, this is a question for somebody who knows Windows better than I do.  Matt, reading comment 3 and comment 4, do you have enough background to be able to give some guidance here?

Comment 8

[Kris Maglione [:kmag]]

We should probably just stop waiting for the process to exit at shutdown after we call kill. It's true that it may mean we leave the processes alive, but waiting for them to die before we exit doesn't really change that.

Comment 9

[Molly Howell (she/her) (mostly inactive)]

I'm with Kris; we might be able to detect whatever these things are doing and give up in those cases, but it would be tough and probably unreliable (especially with AV apps there's the inherent problem of them having more permissions than we do), and I don't see that we're accomplishing enough with this wait to make it worth it.

Comment 10

[Rob Wu [:robwu]]

In the past month, 6562 of such crash reports were received, involving more applications than were listed in the "crash signature" field: https://crash-stats.mozilla.org/search/?signature=~AsyncShutdownTimeout%20%7C%20profile-before-change%20%7C%20Native%20Messaging%3A%20Wait%20for%20application&date=%3E%3D2019-03-04T17%3A35%3A00.000Z&date=%3C2019-04-04T17%3A35%3A00.000Z&_facets=signature&_sort=-date&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

Comment 11

[Rob Wu [:robwu]]

Attachment: Bug 1443544 - Enforce deadline for shutdown blocker of native messaging

Native messaging hosts may ignore requests to be killed. Firefox should
not block its own shutdown on an uncooperative child process.
Besides, the previous implementation did not gurantee that child
processes terminate at Firefox shutdown, e.g. if Firefox was killed
while a child process was starting, but before the Subprocess API
has returned control to NativeMessaging.jsm.

This patch also refactors the _cleanup method; it may repeatedly be
called (as seen in bug 1481225), but killing once should be sufficient.

Comment 12

[[:philipp]]

any way to move the patch/review forward?

two days ago there was an update to version 4.4 of ant video downloader (https://addons.mozilla.org/en-US/firefox/addon/video-downloader-player/) which apparently adds another 100 daily crash reports onto the baseline we're already seeing with these signatures.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 14

[Rob Wu [:robwu]]

I'll ask a reviewer to take a look at the patch.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8f5c15a05405
Enforce deadline for shutdown blocker of native messaging r=zombie

Comment 16

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/8f5c15a05405