Crash in "AsyncShutdownTimeout | profile-before-change | Native Messaging: Wait for application * to exit" with multiple applications
Categories
(External Software Affecting Firefox :: Other, defect, P1)
Tracking
(firefox58 wontfix, firefox59 wontfix, firefox60 wontfix, firefox74 fixed)
People
(Reporter: marcia, Assigned: robwu)
References
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 =============================================================
Comment 1•6 years ago
|
||
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•6 years ago
|
||
Forwarding to aswan who worked on the nativeMessaging API code and may have some ideas.
Comment 3•6 years ago
|
||
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•6 years ago
|
||
Kris, maybe we could check the return value here? https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/toolkit/modules/subprocess/subprocess_worker_win.js#341
Updated•6 years ago
|
Comment 5•5 years ago
|
||
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?
Updated•5 years ago
|
Comment 6•5 years ago
|
||
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•5 years ago
|
||
(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?
Updated•5 years ago
|
Comment 8•5 years ago
|
||
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•5 years ago
|
||
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.
Assignee | ||
Comment 10•5 years ago
|
||
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
Assignee | ||
Comment 11•5 years ago
|
||
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.
Updated•5 years ago
|
Updated•5 years ago
|
Comment 12•4 years ago
•
|
||
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•4 years ago
|
||
Bugbug thinks this bug is a regression, but please revert this change in case of error.
Assignee | ||
Comment 14•4 years ago
|
||
I'll ask a reviewer to take a look at the patch.
Comment 15•4 years ago
|
||
Pushed by rob@robwu.nl: https://hg.mozilla.org/integration/autoland/rev/8f5c15a05405 Enforce deadline for shutdown blocker of native messaging r=zombie
Comment 16•4 years ago
|
||
bugherder |
Updated•4 years ago
|
Description
•