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

Issue 118424 link

Starred by 13 users

Issue metadata

Status: Verified
Owner:
Closed: Oct 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug



Sign in to add a comment

Triggering an onbeforeunload confirmation dialog makes chrome never exit

Project Member Reported by atwilson@chromium.org, Mar 15 2012

Issue description

Version: 19.0.1068.1 dev
OS: OSX 10.6.8

What steps will reproduce the problem?
1. Create a page like http://atwilson-macpro.sea/~atwilson/beforeunload.html:

<html>
<body>
Close me.
<script>
window.onbeforeunload = function() {
  return "don't close me bro";
}
</script>
</body>
</html>

2. Open two separate windows - one pointing at the page with onbeforeunload, and another window pointing at some other page (say, www.google.com)
3. Hit Cmd-Q to close chrome
4. Get the confirmation popup, and choose to close the page (I think it's titled "Leave this page").

What is the expected output? What do you see instead?

I expect chrome to exit. What I actually see is the two windows close, but chrome itself does not exit - it stays in the system tray and I have to force-quit it using the Activity Manager. I have reproduced this on Mac, but I haven't tried it on other platforms.

The chrome-doesn't-exit behavior has been driving me crazy - I'm glad to have finally isolated a repro case.


Please use labels and text to provide additional information.
 

Comment 1 Deleted

It is reproduced with latest canary also. This issues is there from the stable build. It is not a Regression. 
It is working fine with FF.

Comment 4 by rsesek@chromium.org, Mar 15 2012

This sounds like either  issue 77781 	or  issue 113964 .
Possibly. Does 77781 prevent chrome from exiting if you try quitting again? It seems weird that we'd leave it unfixed for so long since forcing the user to force-quit is such a bummer.

Not sure that it's 113964 since I don't seem to be hitting a crash, although maybe that bug has extended to cover non-crashing scenarios.

Comment 6 by rsesek@chromium.org, Mar 16 2012

Yes, unfortunately it does. As I write in comment two in 77781, it's a tricky thing to do right. I have some ideas, so maybe I'll take another look. If you have confirm-to-quit enabled, it's almost certainly that bug.
FWIW, I don't have confirm-to-quit enabled.

Comment 8 by rsesek@chromium.org, Mar 19 2012

Cc: -pinkerton@chromium.org
Labels: Feature-Browser
Okay, yes I can repro this without CTQ enabled.

The key piece is having *two* windows. Just having it in one does not repro.
Labels: Mstone-20
Owner: rsesek@chromium.org
Status: Assigned
Labels: -Mstone-20 MovedFrom-20 Mstone-21
M20 has sailed. If this need to be part of M20, add them back with appropriate release blocker labels.

Comment 11 by k...@google.com, May 21 2012

Cc: rsesek@chromium.org mnarula@chromium.org vandanashah@chromium.org srihariraju@chromium.org nikhilalm@chromium.org jayakrishnat@chromium.org gailh@chromium.org arsids@chromium.org
 Issue 124085  has been merged into this issue.
Status: Started
I figured it out
I guess I haven't figured it out yet (boo). But I'm still looking.

From what I can tell, this does happen if the window has been session restored. It will happen if the windows are created new in the session. Hmm.
Labels: -Feature-Browser Feature-Sessions
I built with enable_sessions=0 in build/common.gypi and could *NOT* repro this bug, which leads me to believe it's a problem with SessionService. Will continue investigating.
I have two patches that fix it (not sure which I'm going to go with yet), but I'm still working on the regression test.

Comment 16 by karen@chromium.org, Jul 11 2012

Labels: -Mstone-21 MovedFrom-21 Mstone-22
Moving all non essential bugs to the next Milestone
Labels: -Mstone-22 Mstone-23
An update I forgot to post:

I found some subtle inconsistencies with our shutdown code, which I thought were the root cause of this bug, but turned out to not be -- fixing them just made it a little harder to reproduce. And so now I'm stuck again. I think this may be related to the TabRestoreService/session restore.

Attached is a diagram of Chrome's Cocoa shutdown path, which is what I was investigating for problems.
shutdown.png
792 KB View Download
Um, wow.

Comment 19 by mark@chromium.org, Aug 27 2012

Cc: mark@chromium.org
 Issue 143438  has been merged into this issue.

Comment 20 by sail@chromium.org, Oct 8 2012

Cc: sail@chromium.org
Labels: -Mstone-23 Mstone-24
I have some FANTASTIC news: I've isolated the root cause of this bug. The unfortunate news is that I don't yet have a good idea for a fix.

The diagram in comment 17 may be helpful in understanding this, even though some code has been lightly refactored.

For this bug to reproduce, you must have at least two windows open, one of which will fire an beforeunload handler that pops open a modal alert.

When the user goes to Quit, -[BrowserCrApplication terminate:] is called, which calls -[AppController tryToTerminateApplication:], which then calls browser::CloseAllBrowsers().

In CloseAllBrowsers(), the BrowserList is iterated and BrowserWindow::Close() is called on each Browser::window(). BrowserWindowCocoa calls the standard -[NSWindow performClose:], which fakes a click to the NSWindow's close button. That eventually calls through to -[BrowserWindowController windowShouldClose:]. 
The |-windowShouldClose:| method calls into Browser::ShouldCloseWindow(), which merely thunks to UnloadController::ShouldCloseWindow(), which is responsible for running all the tab unload handlers.

If the window has no unload handlers or if all the unload handlers have been run, then it calls Browser::TabStripEmpty() via the TabStripModel. TabStripEmpty() posts a task Browser::CloseFrame(), which again calls BrowserWindow::Close(). Remember that BrowserWindowCocoa::Close() calls -[NSWindow performClose:], which fakes a click on the NSWindow's close control.

This is where things get a little screwy. browser::CloseAllBrowsers() is iterating over the list sequentially, which is initiating the Browser closing process, which in turn causes various asynchronous tasks to run (for unload handlers and for Browser::CloseFrame()). What happens is that the task to bring up the app-modal JavaScript beforeunload handler alert and the call to -[NSWindow performClose:] get interleaved, such that an app-modal dialog is being brought up while attempting to fake a click on the NSWindow's close button. AppKit of course will block this, since an app-modal dialog is now the key window, but at the same time, the Browser's close process is completed from Chrome's perspective. The problem is thus that the BrowserWindowController, which owns the Browser that resides in the BrowserList, is never released. Since the BrowserList never goes to zero, then the app never terminates completely.

Yikes, right? I have no good ideas on how to fix this right now, but since I just figured this out, I haven't had time to noodle on it.

Attached is a gdb session that attempts to illustrate this -- it may not be helpful. The window entitled "Test for  Bug #77781 " has the beforeunload handler, and the "New Tab" window is just one with an empty tab. The important things to watch are the self= and this= pointers. Note that -[BrowserWindowController windowShouldClose:] is never called a second time from Browser::CloseFrame for the "New Tab" window, and thus the BrowserWindowController is never deallocated.
(attachment forgetfulness)
bug-118424.txt
42.0 KB View Download
I guess one option would be to use -[NSWindow close] directly instead of |-performClose:|. I'm testing that change now.
Fix and regression test out for review: https://codereview.chromium.org/11090029/
Project Member

Comment 25 by bugdroid1@chromium.org, Oct 15 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=161874

------------------------------------------------------------------------
r161874 | rsesek@chromium.org | 2012-10-15T16:45:47.229737Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm?r1=161874&r2=161873&pathrev=161874
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/browser_browsertest.cc?r1=161874&r2=161873&pathrev=161874
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/cocoa/browser_window_cocoa.mm?r1=161874&r2=161873&pathrev=161874

[Mac] In BrowserWindowCocoa::Close() do not use -[NSWindow performClose:].

|-performClose:| will mimic a click on the window's close button, which can
race a JavaScript app-modal alert during shutdown, leaving around an orphaned
Browser object.

BUG= 118424 
TEST=See steps in bug. Regression test added.


Review URL: https://chromiumcodereview.appspot.com/11090029
------------------------------------------------------------------------

Comment 26 by gst...@gmail.com, Oct 15 2012

Yay!

Thank you. I've been hoping to see this fixed for a very long time. Gmail chat windows have been killin' me....
Status: Fixed
Status: Verified
This is working fine on latest M24 build - 24.0.1297.0 (Official Build 162078) - Mac osx 10.6.8 & 10.8.2
Great work!
* Who do I send the 6-pack to on this one for the clever fix?
* When is next GM drop with fix? (roughly?)
Project Member

Comment 30 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Area-UI -Feature-Sessions -Mstone-24 Cr-UI M-24 Cr-UI-Browser-Sessions

Sign in to add a comment