PWA hangs when attempting to quit with Cmd-Q |
|||||||||||
Issue descriptionChrome Version: 72.0.3603.0 (Official Build) canary (64-bit) OS: macOS 10.14.1 What steps will reproduce the problem? (1) Enable experimental PWA support in Chrome Canary. (2) Install Google Photos as a PWA. (3) Open Google Photos PWA. (4) Use Google App Launcher to open a different Chrome app (Chat, in the attached video). (5) Observe that the new app opens in the current PWA frame, rather than in another window. (6) Attempt to navigate back (doesn't seem to work). (7) Quit using cmd-Q. (8) Observe that the App is stuck and must be killed with Activity Monitor. What is the expected result? - Presumably Step (4) should open in a new window, but I guess that's an issue with the Google Photos PWA manifest? - Regardless, it seems like in Step (6) we should be able to get back to Google Photos, and in step (7) we shouldn't hang / leave the app in an unquittable/force-kill-required state. What happens instead? Can't navigate back, can't quit. Screen recording: https://drive.google.com/open?id=1erShH0NtIaq8njOGnn00-_njkPgFI6Rt
,
Nov 7
,
Nov 12
Mac triage: marking Assigned to sdy@ and moving ccameron@ (who is OOO) to CC.
,
Dec 5
So, when I hit command-Q in a release build, and it hangs, the hang is in the following stack:
This is concerning because it's hanging inside AppKit code.
But ... let's look at from #7 -- it's the same as #54. Maybe I should be posting a task at #20? I'll give that a try.
* frame #0: libsystem_kernel.dylib`mach_msg_trap + 10
frame #1: libsystem_kernel.dylib`mach_msg + 60
frame #2: CoreFoundation`__CFRunLoopServiceMachPort + 341
frame #3: CoreFoundation`__CFRunLoopRun + 1783
frame #4: CoreFoundation`CFRunLoopRunSpecific + 487
frame #5: HIToolbox`RunCurrentEventLoopInMode + 286
frame #6: HIToolbox`ReceiveNextEventCommon + 613
frame #7: HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 64
frame #8: AppKit`_DPSNextEvent + 2085
frame #9: AppKit`-[NSApplication
frame #10: AppKit`-[NSApplication _shouldTerminate] + 1367
frame #11: AppKit`-[NSApplication terminate:] + 818
frame #12: AppKit`-[NSApplication
frame #13: AppKit`-[NSMenuItem _corePerformAction] + 323
frame #14: AppKit`-[NSCarbonMenuImpl performActionWithHighlightingForItemAtIndex:] + 114
frame #15: AppKit`-[NSMenu performKeyEquivalent:] + 973
frame #16: AppKit`-[NSMenu performKeyEquivalent:] + 705
frame #17: AppKit`routeKeyEquivalent + 884
frame #18: AppKit`[NSApplication(NSEvent) sendEvent:]
frame #19: libui_base.dylib`::-[CommandDispatcher redispatchKeyEvent:]
frame #20: libviews.dylib`views::BridgedNativeWidgetImpl::RedispatchKeyEvent
frame #21: libviews.dylib`views_bridge_mac::mojom::BridgedNativeWidgetStubDispatch::Accept
frame #22: libbindings.dylib`mojo::InterfaceEndpointClient::HandleValidatedMessage
frame #23: libbindings.dylib`mojo::FilterChain::Accept
frame #24: libbindings.dylib`mojo::InterfaceEndpointClient::HandleIncomingMessage
frame #25: libbindings.dylib`mojo::internal::MultiplexRouter::ProcessIncomingMessage
frame #26: libbindings.dylib`mojo::internal::MultiplexRouter::Accept
frame #27: libbindings.dylib`mojo::FilterChain::Accept
frame #28: libbindings.dylib`mojo::Connector::ReadSingleMessage
frame #29: libbindings.dylib`mojo::Connector::ReadAllAvailableMessages
frame #30: libbindings.dylib`mojo::Connector::OnHandleReadyInternal
frame #31: libbindings.dylib`mojo::SimpleWatcher::DiscardReadyState
frame #32: libbindings.dylib`mojo::SimpleWatcher::DiscardReadyState
frame #33: libmojo_public_system_cpp.dylib`mojo::SimpleWatcher::OnHandleReady
frame #34: libmojo_public_system_cpp.dylib`mojo::SimpleWatcher::OnHandleReady
frame #35: libmojo_public_system_cpp.dylib`void base::internal::Invoker<base::internal::BindState<void
frame #36: libmojo_public_system_cpp.dylib`void base::internal::Invoker<base::internal::BindState<void
frame #37: libmojo_public_system_cpp.dylib`void base::internal::Invoker<base::internal::BindState<void
frame #38: libaccelerated_widget_mac.dylib`base::OnceCallback<void
frame #39: libbase.dylib`base::debug::TaskAnnotator::RunTask
frame #40: libbase.dylib`base::debug::TaskAnnotator::RunTask
frame #41: libbase.dylib`base::MessageLoopImpl::RunTask
frame #42: libbase.dylib`base::MessageLoopImpl::DoWork
frame #43: libbase.dylib`base::MessageLoopImpl::DoWork
frame #44: libbase.dylib`base::MessagePumpCFRunLoopBase::RunWork
frame #45: libbase.dylib`base::mac::CallWithEHFrame
frame #46: libbase.dylib`base::MessagePumpCFRunLoopBase::RunWorkSource
frame #47: CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
frame #48: CoreFoundation`__CFRunLoopDoSource0 + 108
frame #49: CoreFoundation`__CFRunLoopDoSources0 + 208
frame #50: CoreFoundation`__CFRunLoopRun + 1293
frame #51: CoreFoundation`CFRunLoopRunSpecific + 487
frame #52: HIToolbox`RunCurrentEventLoopInMode + 286
frame #53: HIToolbox`ReceiveNextEventCommon + 613
frame #54: HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 64
frame #55: AppKit`_DPSNextEvent + 2085
frame #56: AppKit`-[NSApplication
frame #57: AppKit`-[NSApplication run] + 764
frame #58: libbase.dylib`base::MessagePumpNSApplication::DoRun
frame #59: libbase.dylib`base::MessagePumpCFRunLoopBase::Run
frame #60: libbase.dylib`base::MessageLoopImpl::Run
frame #61: libbase.dylib`base::RunLoop::Run
frame #62: libchrome_dll.dylib`::ChromeAppModeStart_v4
frame #63: app_mode_loader`main [inlined]
frame #64: app_mode_loader`main
frame #65: libdyld.dylib`start + 1
frame #66: libdyld.dylib`start + 1
,
Dec 6
In the above, frames #9 and #56 should read as follows (got a bit aggressive with cutting):
frame #8: AppKit`_DPSNextEvent
frame #9: AppKit`-[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:]
frame #10: AppKit`-[NSApplication _shouldTerminate]
frame #55: AppKit`_DPSNextEvent
frame #56: AppKit`-[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:]
frame #57: AppKit`-[NSApplication run]
,
Dec 6
+rsesek
,
Dec 6
We should figure out what event AppKit is waiting for. I do note that the NSApplicationDelegate for the shim has an implementation of -applicationShouldTerminate: https://cs.chromium.org/chromium/src/chrome/app_shim/app_shim_delegate.mm?l=68&rcl=9b5c0a7e5bf7a73942ba783b6180f83a4d63b1e4 . Does returning NSTerminateNow unconditionally fix it? Since the MessageLoop is running a task, I think if we need another task to be pumped in order to make progress, that won't happen because the MessageLoop protects against reentrancy.
,
Dec 7
Thanks ... I didn't notice that AppShimDelegate was doing that (shame! shame! shame!) Now this all makes sense. So what happens is that we send appShimController_->host()->QuitApp(); which doesn't actually shut anything down, rather, it requests that the browser close all windows for the app, over in ExtensionAppShimHandler::CloseBrowsersForApp This ends up politely requesting that the app shim go close its window in NativeWidgetMac::Close This, in the ordinary run of things, would land in the app shim process, it would close its windows, notify the browser process, whereupon the app shim host would be deleted, closing the connection, allowing the app to terminate. But ... we're in a nested run loop inside handling a mojo method. So the polite request made in NativeWidgetMac::Close never arrives, and the windows never close.
,
Dec 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/464fae69315d22b6bce86525bb73305bd0cc7296 commit 464fae69315d22b6bce86525bb73305bd0cc7296 Author: Christopher Cameron <ccameron@chromium.org> Date: Sat Dec 08 02:32:47 2018 RemoteMacViews: Fix hang at command-Q When we call -[NSApp terminate] in the app shim process, this does not immediately terminate. Rather, it 0. Sends the message appShimController_->host()->QuitApp() and waits for a reply before actually terminating. 1. In the browser, this call hits ExtensionAppShimHandler:: CloseBrowsersForApp 2. Which will end up calling NativeWidgetMac::Close 3. Which will then politely request that the app shim process close the corresponding window via views_bridge_mac::mojom::BridgedNativeWidget::CloseWindow 4. Which will send an ack back to the browser via views_bridge_mac::mojom::BridgedNativeWidgetHost::OnWindowHasClosed 5. Whereupon the browser window will be registered as closed, and a close message will be sent to the app shim, and the wait for termination will break This ends up not working at step 3. Our suspended terminate ends up running a nested message loop inside a mojo message handler, and so it will never execute the CloseWindow message, and therefore never close. Change the logic at [0] to send the QuitApp message, and then immediately terminate the application. Bug: 902583 Change-Id: I60fd7b5fdf9f145369d06219e8ebef36cdcd0f44 Reviewed-on: https://chromium-review.googlesource.com/c/1364220 Reviewed-by: Robert Sesek <rsesek@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#614922} [modify] https://crrev.com/464fae69315d22b6bce86525bb73305bd0cc7296/chrome/app_shim/app_shim_controller.mm [modify] https://crrev.com/464fae69315d22b6bce86525bb73305bd0cc7296/chrome/app_shim/app_shim_delegate.h [modify] https://crrev.com/464fae69315d22b6bce86525bb73305bd0cc7296/chrome/app_shim/app_shim_delegate.mm [modify] https://crrev.com/464fae69315d22b6bce86525bb73305bd0cc7296/ui/views_bridge_mac/bridged_native_widget_impl.mm
,
Dec 8
,
Dec 10
Tried testing the issue on chrome reported version# 72.0.3603.0 using Mac 10.12.6 with steps mentioned below: 1) Launched chrome reported version and Enable experimental PWA support 2) Logged-in into google photos and Installed Google Photos as a PWA. 3) Open Google Photos PWA, it got opened in new window 4) Used Google App Launcher to launch hangouts, hangouts got opened in chrome window Note: As per comment#0 - Step-4, need to open chat app from chrome Apps, chat App seems to be related to CORP account. @Reporter: Could you please try to test this issue on latest chrome# 73.0.3636.0 and help us in verifying the fix. Thanks!
,
Dec 11
,
Dec 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2e1e507d5b922a215e32bf7981e72fe3883997be commit 2e1e507d5b922a215e32bf7981e72fe3883997be Author: Friedrich Horschig [CET] <fhorschig@chromium.org> Date: Tue Dec 11 10:52:00 2018 Revert "RemoteMacViews: Fix hang at command-Q" This reverts commit 464fae69315d22b6bce86525bb73305bd0cc7296. Reason for revert: Causes heavy flakiness (14/17 on Mac 10.11) and attempted fix showed no effect: https://crrev.com/c/1369136 TBR=ccameron@chromium.org Bug: 902583 Original change's description: > RemoteMacViews: Fix hang at command-Q > > When we call -[NSApp terminate] in the app shim process, this does > not immediately terminate. Rather, it > 0. Sends the message appShimController_->host()->QuitApp() and waits > for a reply before actually terminating. > 1. In the browser, this call hits ExtensionAppShimHandler:: > CloseBrowsersForApp > 2. Which will end up calling NativeWidgetMac::Close > 3. Which will then politely request that the app shim process close > the corresponding window via > views_bridge_mac::mojom::BridgedNativeWidget::CloseWindow > 4. Which will send an ack back to the browser via > views_bridge_mac::mojom::BridgedNativeWidgetHost::OnWindowHasClosed > 5. Whereupon the browser window will be registered as closed, and a > close message will be sent to the app shim, and the wait for > termination will break > > This ends up not working at step 3. Our suspended terminate ends up > running a nested message loop inside a mojo message handler, and so > it will never execute the CloseWindow message, and therefore never > close. > > Change the logic at [0] to send the QuitApp message, and then > immediately terminate the application. > > Bug: 902583 > Change-Id: I60fd7b5fdf9f145369d06219e8ebef36cdcd0f44 > Reviewed-on: https://chromium-review.googlesource.com/c/1364220 > Reviewed-by: Robert Sesek <rsesek@chromium.org> > Commit-Queue: ccameron <ccameron@chromium.org> > Cr-Commit-Position: refs/heads/master@{#614922} TBR=ccameron@chromium.org,rsesek@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 902583 Change-Id: I34a3300dc188cdd0de58692042ce131a8639117f Reviewed-on: https://chromium-review.googlesource.com/c/1371475 Reviewed-by: Friedrich Horschig [CET] <fhorschig@chromium.org> Commit-Queue: Friedrich Horschig [CET] <fhorschig@chromium.org> Cr-Commit-Position: refs/heads/master@{#615485} [modify] https://crrev.com/2e1e507d5b922a215e32bf7981e72fe3883997be/chrome/app_shim/app_shim_controller.mm [modify] https://crrev.com/2e1e507d5b922a215e32bf7981e72fe3883997be/chrome/app_shim/app_shim_delegate.h [modify] https://crrev.com/2e1e507d5b922a215e32bf7981e72fe3883997be/chrome/app_shim/app_shim_delegate.mm [modify] https://crrev.com/2e1e507d5b922a215e32bf7981e72fe3883997be/ui/views_bridge_mac/bridged_native_widget_impl.mm
,
Dec 11
The fix caused heavy failures in issue 913181. Please have another look!
,
Dec 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/61c84ff35a0c670044404610107765d3a3951aca commit 61c84ff35a0c670044404610107765d3a3951aca Author: ccameron <ccameron@chromium.org> Date: Tue Dec 11 18:46:56 2018 Reland "RemoteMacViews: Fix hang at command-Q" This reverts commit 2e1e507d5b922a215e32bf7981e72fe3883997be. Reason for revert: Flakey test has been disabled. The behavior that the test is testing is being deprecated in Chrome 73. Original change's description: > Revert "RemoteMacViews: Fix hang at command-Q" > > This reverts commit 464fae69315d22b6bce86525bb73305bd0cc7296. > > Reason for revert: Causes heavy flakiness (14/17 on Mac 10.11) and attempted fix showed no effect: https://crrev.com/c/1369136 > > TBR=ccameron@chromium.org > Bug: 902583 > > Original change's description: > > RemoteMacViews: Fix hang at command-Q > > > > When we call -[NSApp terminate] in the app shim process, this does > > not immediately terminate. Rather, it > > 0. Sends the message appShimController_->host()->QuitApp() and waits > > for a reply before actually terminating. > > 1. In the browser, this call hits ExtensionAppShimHandler:: > > CloseBrowsersForApp > > 2. Which will end up calling NativeWidgetMac::Close > > 3. Which will then politely request that the app shim process close > > the corresponding window via > > views_bridge_mac::mojom::BridgedNativeWidget::CloseWindow > > 4. Which will send an ack back to the browser via > > views_bridge_mac::mojom::BridgedNativeWidgetHost::OnWindowHasClosed > > 5. Whereupon the browser window will be registered as closed, and a > > close message will be sent to the app shim, and the wait for > > termination will break > > > > This ends up not working at step 3. Our suspended terminate ends up > > running a nested message loop inside a mojo message handler, and so > > it will never execute the CloseWindow message, and therefore never > > close. > > > > Change the logic at [0] to send the QuitApp message, and then > > immediately terminate the application. > > > > Bug: 902583 > > Change-Id: I60fd7b5fdf9f145369d06219e8ebef36cdcd0f44 > > Reviewed-on: https://chromium-review.googlesource.com/c/1364220 > > Reviewed-by: Robert Sesek <rsesek@chromium.org> > > Commit-Queue: ccameron <ccameron@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#614922} > > TBR=ccameron@chromium.org,rsesek@chromium.org > > # Not skipping CQ checks because original CL landed > 1 day ago. > > Bug: 902583 > Change-Id: I34a3300dc188cdd0de58692042ce131a8639117f > Reviewed-on: https://chromium-review.googlesource.com/c/1371475 > Reviewed-by: Friedrich Horschig [CET] <fhorschig@chromium.org> > Commit-Queue: Friedrich Horschig [CET] <fhorschig@chromium.org> > Cr-Commit-Position: refs/heads/master@{#615485} TBR=ccameron@chromium.org,rsesek@chromium.org,fhorschig@chromium.org Change-Id: Iceda65f61ac8a63bfd53803f713f0472f90e0d47 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 902583 Reviewed-on: https://chromium-review.googlesource.com/c/1371939 Reviewed-by: ccameron <ccameron@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#615605} [modify] https://crrev.com/61c84ff35a0c670044404610107765d3a3951aca/chrome/app_shim/app_shim_controller.mm [modify] https://crrev.com/61c84ff35a0c670044404610107765d3a3951aca/chrome/app_shim/app_shim_delegate.h [modify] https://crrev.com/61c84ff35a0c670044404610107765d3a3951aca/chrome/app_shim/app_shim_delegate.mm [modify] https://crrev.com/61c84ff35a0c670044404610107765d3a3951aca/ui/views_bridge_mac/bridged_native_widget_impl.mm
,
Dec 11
,
Dec 12
|
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by ccameron@chromium.org
, Nov 7Labels: proj-MacPwa