1632845 - Firefox gets stuck when quit from the dock if the user cancels quitting from a beforeunload prompt

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Jeff Muizelaar [:jrmuizel]]

I've had this happen to me twice in the last two days. To get into this situation I try to quit Firefox from the dock while having a multiple tabs with https://sql.telemetry.mozilla.org/queries/ which has an onbeforeunload that prompts about saving.

I try to save things and then Firefox mostly stops responding.

If I try to quit from the dock again I get very high cpu usage in the parent process and a profile with a stack that looks sort of like this:

1.23 min  100.0%	0 s	 	firefox (8587)
46.42 s   63.1%	0 s	 	 Main Thread  0x71564
45.72 s   62.2%	0 s	 	  start
45.72 s   62.2%	0 s	 	   main
45.72 s   62.2%	0 s	 	    XRE_main(int, char**, mozilla::BootstrapConfig const&)
45.72 s   62.2%	0 s	 	     XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&)
45.72 s   62.2%	0 s	 	      XREMain::XRE_mainRun()
45.72 s   62.2%	0 s	 	       nsAppStartup::Run()
45.72 s   62.2%	0 s	 	        nsAppShell::Run()
45.72 s   62.2%	0 s	 	         -[NSApplication run]
45.72 s   62.2%	0 s	 	          -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:]
45.72 s   62.2%	0 s	 	           -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:]
45.72 s   62.2%	0 s	 	            _DPSNextEvent
45.72 s   62.2%	0 s	 	             AEProcessAppleEvent
45.72 s   62.2%	0 s	 	              aeProcessAppleEvent
45.72 s   62.2%	0 s	 	               0x7fff2f56f790
45.72 s   62.2%	0 s	 	                0x7fff2f56f816
45.72 s   62.2%	0 s	 	                 _NSAppleEventManagerGenericHandler
45.72 s   62.2%	0 s	 	                  -[NSAppleEventManager dispatchRawAppleEvent:withRawReply:handlerRefCon:]
45.72 s   62.2%	0 s	 	                   -[NSApplication(NSAppleEventHandling) _handleCoreEvent:withReplyEvent:]
45.72 s   62.2%	0 s	 	                    -[NSApplication(NSAppleEventHandling) _handleAEQuit]
45.72 s   62.2%	0 s	 	                     -[NSApplication _shouldTerminate]
45.72 s   62.2%	0 s	 	                      -[NSDocumentController(NSInternal) __closeAllDocumentsWithDelegate:shouldTerminateSelector:]
45.72 s   62.2%	0 s	 	                       -[NSDocumentController(NSInternal) _closeAllDocumentsWithDelegate:shouldTerminateSelector:]
45.72 s   62.2%	0 s	 	                        __91-[NSDocumentController(NSInternal) _closeAllDocumentsWithDelegate:shouldTerminateSelector:]_block_invoke
45.72 s   62.2%	0 s	 	                         -[NSApplication _docController:shouldTerminate:]
45.72 s   62.2%	0 s	 	                          -[MacApplicationDelegate applicationShouldTerminate:]
45.72 s   62.2%	0 s	 	                           nsAppStartup::Quit(unsigned int)
45.72 s   62.2%	0 s	 	                            nsGlobalWindowOuter::CanClose()
45.72 s   62.2%	0 s	 	                             SharedStub
45.72 s   62.2%	0 s	 	                              PrepareAndDispatch
45.72 s   62.2%	0 s	 	                               nsXPCWrappedJS::CallMethod(unsigned short, nsXPTMethodInfo const*, nsXPTCMiniVariant*)
45.72 s   62.2%	0 s	 	                                JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>)
45.72 s   62.2%	0 s	 	                                 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason)
45.72 s   62.2%	0 s	 	                                  Interpret(JSContext*, js::RunState&)
45.72 s   62.2%	0 s	 	                                   js::jit::MaybeEnterJit(JSContext*, js::RunState&)
45.72 s   62.2%	0 s	 	                                    0x395b7a9716ae
45.72 s   62.2%	0 s	 	                                     0x13d272dcf
45.72 s   62.2%	0 s	 	                                      0x395b7a971524
45.72 s   62.2%	0 s	 	                                       0x13d29bb1f
45.72 s   62.2%	0 s	 	                                        0x395b7aef9d66
45.72 s   62.2%	0 s	 	                                         0x13cfb69ff
45.72 s   62.2%	0 s	 	                                          0x395b7aed9caf
45.72 s   62.2%	0 s	 	                                           XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)
45.72 s   62.2%	0 s	 	                                            XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode)
45.72 s   62.2%	0 s	 	                                             NS_InvokeByIndex
45.68 s   62.1%	190.00 ms	 	                                              nsThreadManager::SpinEventLoopUntilInternal(nsINestedEventLoopCondition*, bool)
37.09 s   50.4%	1.36 s	 	                                               nsThread::ProcessNextEvent(bool, bool*)
15.21 s   20.6%	54.00 ms	 	                                                mozilla::BackgroundHangThread::NotifyActivity()
14.49 s   19.7%	17.00 ms	 	                                                 mozilla::detail::ConditionVariableImpl::notify_all()
14.46 s   19.6%	268.00 ms	 	                                                  pthread_cond_broadcast
14.02 s   19.0%	14.02 s	 	                                                   __psynch_cvbroad
170.00 ms    0.2%	168.00 ms	 	                                                   _pthread_cond_updateval

Comment 1

[Jeff Muizelaar [:jrmuizel]]

This happened to me again today.

Comment 2

[Stephen A Pohl [:spohl]]

This seems to get stuck in BrowserUtils' canCloseWindow[1]. It looks like bugs in that code typically fall into the Firefox > General component, so I'm tentatively sending over there for a look.

[1] https://searchfox.org/mozilla-central/source/toolkit/modules/BrowserUtils.jsm#613

Comment 3

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

The severity field is not set for this bug.
:Dolske, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[Mike de Boer [:mikedeboer]]

While there's a high likelihood that this is not quite actionable for you, Gijs, and will require more investigation as to why canCloseWindow() is returning false here, I'm flagging you just in case.

Comment 5

[:Gijs (he/him)]

(In reply to Stephen A Pohl [:spohl] from comment #2)

This seems to get stuck in BrowserUtils' canCloseWindow

Where do you see this? AFAICT the stack in comment #0 is from nsGlobalWindowOuter::CanClose which calls BrowserDOMWindow's canClose, which calls CanCloseWindow, which is https://searchfox.org/mozilla-central/rev/ea7f70dac1c5fd18400f6d2a92679777d4b21492/browser/base/content/browser.js#7809 . The code you linked iterates over same-process child docshells, which shouldn't exist in the e10s browser case.

Anyway, I can reproduce, but the frontend JS code here seems to behave correctly. We send a permitunload check to the child, that shows the dialog in the parent, the user clicks "stay on page", and we bail from the loop here, which means we should be bailing at https://searchfox.org/mozilla-central/rev/ea7f70dac1c5fd18400f6d2a92679777d4b21492/browser/base/content/browser.js#6278-6280 and then at https://searchfox.org/mozilla-central/rev/ea7f70dac1c5fd18400f6d2a92679777d4b21492/toolkit/components/startup/nsAppStartup.cpp#352 . So I don't understand why we'd hang. I don't see re-entrancy, and the JS code all seems to behave as expected.

I tried breaking into xcode when the hang occurs, but I just end up in the main run loop. I don't see CPU usage, either, but the main app stops responding to user input in a reasonable timeframe. I haven't tried quitting again at that point... I think something is already wonky before then, and we should probably figure out what that is...

Do we know if this is a regression? Maybe a regression timeframe could help us track down what's breaking here.

Comment 6

[:Gijs (he/him)]

OK, so minimal STR:

1. open firefox on a clean profile
2. open a new tab with http://dolske.net/mozilla/tests/prompt/onbeforeunload.html
3. click in the content area (or we won't prompt for beforeunload)
4. quit from the dock
5. accept the "multiple tabs" dialog
6. click "stay on page" on the per-page dialog that comes up

Now Firefox is really sluggish, but uses very little cpu

7. quit from the dock again
8. try to click "leave page"

very sad panda.

This was already broken in a 2019-01-01 build... (edit: and 2017-01-01... so clearly not a recent regression)

Comment 7

[:Gijs (he/him)]

With exactly the same steps as comment #6, but using cmd-q from within the app to quit, everything works fine.

So I think this is an issue with our dock integration. Moving back to Cocoa land.

Comment 8

[:Gijs (he/him)]

In particular, I think the bug is probably that https://searchfox.org/mozilla-central/rev/ea7f70dac1c5fd18400f6d2a92679777d4b21492/toolkit/xre/MacApplicationDelegate.mm#301-303 should establish whether calling the app service's Quit method actually led to us quitting, and if not, it shouldn't tell cocoa return NSTerminateNow;

Comment 9

[Tim Spurway [:tspurway]]

S1 or S2 bugs need an assignee - could you find someone for this bug?

Comment 10

[Stephen A Pohl [:spohl]]

I'm looking into this bug, but this is not an S2.

Comment 11

[Stephen A Pohl [:spohl]]

(In reply to :Gijs (he/him) from comment #8)

In particular, I think the bug is probably that https://searchfox.org/mozilla-central/rev/ea7f70dac1c5fd18400f6d2a92679777d4b21492/toolkit/xre/MacApplicationDelegate.mm#301-303 should establish whether calling the app service's Quit method actually led to us quitting, and if not, it shouldn't tell cocoa return NSTerminateNow;

Agreed. I've looked into this a bit but haven't been able to come up with a good solution so far. There is some other work that has become a greater priority, so I will let this return to our backlog of bugs.

Comment 12

[Haik Aftandilian [:haik]]

Attachment: Bug 1632845 - Firefox gets stuck when quit from the dock if the user cancels quitting from a beforeunload prompt r?gijs,spohl

Return NSTerminateCancel from applicationShouldTerminate when the user chooses to "Stay on Page".

Comment 13

[Haik Aftandilian [:haik]]

Following Gij's tip from comment 8, the posted patches change applicationShouldTerminate to return NSTerminateCancel when the shutdown is cancelled by the user choosing to "Stay on Page". The changes use the return value from the DOM window CanClose() method called from nsAppStartup::Quit() using a new out parameter. Most callers of nsAppStartup::Quit() ignore the return value so I haven't changed them to use the new out parameter. JS callers didn't need to be changed.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/228e65b06c33
Firefox gets stuck when quit from the dock if the user cancels quitting from a beforeunload prompt r=Gijs,spohl,necko-reviewers,geckoview-reviewers,dragana,agi

Comment 15

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/228e65b06c33

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b3eb5b294a08
add userAllowedQuit argument to appStartup->Quit call in Thunderbird. rs=bustage-fix

Comment 17

[Alexandru Trif, Desktop QA [:atrif]]

Reproduced the issue using Firefox 77.0a1 (20200424214433) on macOS 10.12 and steps from comment 6.
The issue is verified fixed using Firefox 79.0b2 (20200630191632) on macOS 10.12. No hangs are presented and Firefox behaves as expected after following the above STR.