Open Bug 1443544 Opened 2 years ago Updated Yesterday

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

Categories

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

x86
Windows 7
defect

Tracking

(firefox58 wontfix, firefox59 affected, firefox60 affected)

ASSIGNED
Tracking Status
firefox58 --- wontfix
firefox59 --- affected
firefox60 --- affected

People

(Reporter: marcia, Assigned: robwu)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

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

=============================================================
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.
Flags: needinfo?(lgreco)
Forwarding to aswan who worked on the nativeMessaging API code and may have some ideas.
Flags: needinfo?(lgreco) → needinfo?(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.
Flags: needinfo?(aswan)
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?
Crash Signature: [@ AsyncShutdownTimeout | profile-before-change | Native Messaging: Wait for application siteadvisor.mcafee.chrome.extension to exit] → [@ AsyncShutdownTimeout | profile-before-change | Native Messaging: Wait for application siteadvisor.mcafee.chrome.extension to exit] [@ AsyncShutdownTimeout | profile-before-change | Native Messaging: Wait for application amazon_enterprise_access to exi…
Flags: needinfo?(aswan)
Crash Signature: ,Native Messaging: Wait for application AvastAntiTrackPremiumNativeMessaging to exit,Native Messaging: Wait for applicatio...] → ,Native Messaging: Wait for application AvastAntiTrackPremiumNativeMessaging to exit,Native Messaging: Wait for applicatio...] [@ AsyncShutdownTimeout | profile-before-change | Native Messaging: Wait for application com.netiq.slchromehost to exit,Native …
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?
Flags: needinfo?(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?
Flags: needinfo?(mhowell)
Summary: Crash in AsyncShutdownTimeout | profile-before-change | Native Messaging: Wait for application siteadvisor.mcafee.chrome.extension to exit → Crash in "AsyncShutdownTimeout | profile-before-change | Native Messaging: Wait for application * to exit" with multiple applications
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.
Flags: needinfo?(kmaglione+bmo)
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.
Flags: needinfo?(mhowell)

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.

Attachment #9055987 - Attachment description: Bug 1443544 - Remove shutdown blocker for native messaging → Bug 1443544 - Enforce deadline for shutdown blocker of native messaging
Priority: -- → P1

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.

Crash Signature: application net.downloadhelper.coapp to exit] → application net.downloadhelper.coapp to exit] [@ AsyncShutdownTimeout | profile-before-change | Native Messaging: Wait for application com.ant.avdcc.host to exit,Native Messaging: Wait for application com.ant.avdcc.host to exit,Native Messaging: Wait fo…

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

Keywords: regression

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

Keywords: regression
You need to log in before you can comment on or make changes to this bug.