• ccameron's avatar
    Reland "RemoteMacViews: Fix hang at command-Q" · 61c84ff3
    ccameron authored
    This reverts commit 2e1e507d.
    
    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 464fae69.
    > 
    > 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/1371939Reviewed-by: default avatarccameron <ccameron@chromium.org>
    Commit-Queue: ccameron <ccameron@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#615605}
    61c84ff3
bridged_native_widget_impl.mm 51.1 KB