New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 12
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug

Blocked on:
issue 913181



Sign in to add a comment
link

Issue 902583: PWA hangs when attempting to quit with Cmd-Q

Reported by dharan@google.com, Nov 7 Project Member

Issue description

Chrome 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
 

Comment 1 by ccameron@chromium.org, Nov 7

Cc: mgiuca@chromium.org
Labels: proj-MacPwa
I can reproduce the "Photos hangs indefinitely with Cmd-Q" issue in Canary. Going to see if I can make it also happen in a Chromium build.

WRT "chat opens in the wrong window" ... that may be a CCT issue (adding mgiuca).

Comment 2 by ccameron@chromium.org, Nov 7

Labels: -proj-MacPwa

Comment 3 by ellyjo...@chromium.org, Nov 12

Cc: ccameron@chromium.org
Labels: -Pri-3 Target-73 M-73 Pri-1
Owner: sdy@chromium.org
Status: Assigned (was: Untriaged)
Mac triage: marking Assigned to sdy@ and moving ccameron@ (who is OOO) to CC.

Comment 4 by ccameron@chromium.org, Dec 5

Cc: sdy@chromium.org
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

Comment 5 by ccameron@chromium.org, 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]

Comment 6 by ccameron@chromium.org, Dec 6

Cc: rsesek@chromium.org
+rsesek

Comment 7 by rsesek@chromium.org, 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.

Comment 8 by ccameron@chromium.org, 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.

Comment 9 by bugdroid1@chromium.org, Dec 8

Project Member
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

Comment 10 by ccameron@chromium.org, Dec 8

Status: Fixed (was: Assigned)

Comment 11 by viswa.karala@chromium.org, Dec 10

Labels: Needs-Feedback
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!

Comment 12 by fhorschig@chromium.org, Dec 11

Blockedon: 913181

Comment 13 by bugdroid1@chromium.org, Dec 11

Project Member
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

Comment 14 by fhorschig@chromium.org, Dec 11

Status: Assigned (was: Fixed)
The fix caused heavy failures in issue 913181. Please have another look!

Comment 15 by bugdroid1@chromium.org, Dec 11

Project Member
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

Comment 16 by ccameron@chromium.org, Dec 11

Owner: ccameron@chromium.org

Comment 17 by ccameron@chromium.org, Dec 12

Status: Fixed (was: Assigned)

Sign in to add a comment