Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2793443003:
Removed NOTIFICATION_BROWSER_CLOSING notification (Closed)
|
Created:
3 years, 8 months ago by Alexey Seren Modified:
3 years, 7 months ago CC:
chromium-reviews, gab, manzagop (departed) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReplaced NOTIFICATION_BROWSER_CLOSING by BrowserListObserver::OnBrowserCloseStarted event.
R=sky@chromium.org
Review-Url: https://codereview.chromium.org/2793443003
Cr-Commit-Position: refs/heads/master@{#471292}
Committed: https://chromium.googlesource.com/chromium/src/+/c474307ec0c335b7bc6877f0181aa67293fd1d7c
Patch Set 1 #Patch Set 2 : Added browser tests #
Total comments: 16
Patch Set 3 : Updates basing on review comments #Patch Set 4 : Fixed typos #
Total comments: 2
Patch Set 5 : Reset g_shutdown_type on close cancel #
Total comments: 11
Patch Set 6 : Replaced NOTIFICATION_BROWSER_CLOSING with BrowserListObserver event #Patch Set 7 : Removed unused headers from unload controllers #
Total comments: 3
Patch Set 8 : Removed NOTIFICATION_BROWSER_CLOSING as separate patch #
Total comments: 4
Patch Set 9 : Update CL basing on review comments #Patch Set 10 : Update CL basing on review comments (rebase) #Messages
Total messages: 59 (30 generated)
Description was changed from ========== Fix shutdown on Ctrl+Shift+Q. Fix shutdown races CL fixes several issues: 1. On Windows: proper shutdown handle on Ctrl+Shift+Q 2. Fix races on shutdown between BrowserList::RemoveBrowser and Browser::OnWindowClosing functions R=sky@chromium.org BUG=707147 BUG=707144 ========== to ========== Fix shutdown on Ctrl+Shift+Q. Fix shutdown races CL fixes several issues: 1. On Windows: proper shutdown handle on Ctrl+Shift+Q 2. Fix races on shutdown between BrowserList::RemoveBrowser and Browser::OnWindowClosing functions R=sky@chromium.org BUG=707147 BUG=707144 ==========
aseren@yandex-team.ru changed reviewers: + hashimoto@chromium.org
The CQ bit was checked by aseren@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by aseren@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/browser_... File chrome/browser/browser_shutdown_browsertest.cc (right): https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/browser_... chrome/browser/browser_shutdown_browsertest.cc:28: }; private: DISALLOW_.. https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:2564: BrowserList::GetInstance()->currently_closing_browsers_count()) <= 1; Why do you need the <= 1? Shouldn't it be size() + 1 == currently_closing_browsers_count(), with a description as to why the + 1? https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:759: chrome::AttemptExit(); Why is this change necessary? https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.cc:89: chrome::NOTIFICATION_BROWSER_CLOSED, And remove this notification too? https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.h:161: // A vector of the browsers that is currently in closing state. are currently in the https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/unloa... File chrome/browser/ui/unload_controller.cc (right): https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/unloa... chrome/browser/ui/unload_controller.cc:230: content::Source<Browser>(browser_), Is it possible to remove this notification now?
https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser_list_observer.h (right): https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_list_observer.h:27: virtual void OnBrowserWindowClosing(Browser* browser) {} Please move OnBrowserRemoved after this (or move the two new functions you are adding above it) and document the relation among these. Also, rename OnBrowserWindowClosing to OnBrowserCloseStarting. I'm thinking something like: // When a browser closes OnBrowserCloseStarting() is called. This is // followed by one of OnBrowserCloseCanceled() or OnBrowserRemoved(). OnBrowserStartingClose()
Thank you for reviewing this CL. Please see my comments below. https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/browser_... File chrome/browser/browser_shutdown_browsertest.cc (right): https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/browser_... chrome/browser/browser_shutdown_browsertest.cc:28: }; On 2017/03/31 14:23:41, sky wrote: > private: DISALLOW_.. Acknowledged. https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:2564: BrowserList::GetInstance()->currently_closing_browsers_count()) <= 1; On 2017/03/31 14:23:41, sky wrote: > Why do you need the <= 1? Shouldn't it be size() + 1 == > currently_closing_browsers_count(), with a description as to why the + 1? Yes you are right. Equality checking will allow to start shutdown process only once (because Browser::OnWindowClosing is called several times). Hope this comment will be clear: // Start shutdown process only when closing the last window. https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:759: chrome::AttemptExit(); On 2017/03/31 14:23:41, sky wrote: > Why is this change necessary? This is fix of shutdown histograms on windows when user exits via Ctrl+Shift+Q. AttemptUserExit() doesn’t call browser_shutdown::SetTryingToQuit(true). So we cannot start shutdown process correctly and chrome_shutdown_ms.txt is not created. See: https://cs.chromium.org/chromium/src/chrome/browser/lifetime/application_life... https://cs.chromium.org/chromium/src/chrome/browser/lifetime/application_life... chrome::AttemptExit(), оn the contrary, does what is needed. https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.cc:89: chrome::NOTIFICATION_BROWSER_CLOSED, On 2017/03/31 14:23:41, sky wrote: > And remove this notification too? As like as NOTIFICATION_BROWSER_CLOSE_CANCELLED, this notification is used in several places in chrome. I think it is better to do it in separate task. https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.h:161: // A vector of the browsers that is currently in closing state. On 2017/03/31 14:23:41, sky wrote: > are currently in the Acknowledged. https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser_list_observer.h (right): https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_list_observer.h:27: virtual void OnBrowserWindowClosing(Browser* browser) {} On 2017/03/31 14:36:54, sky wrote: > Please move OnBrowserRemoved after this (or move the two new functions you are > adding above it) and document the relation among these. Also, rename > OnBrowserWindowClosing to OnBrowserCloseStarting. I'm thinking something like: > > // When a browser closes OnBrowserCloseStarting() is called. This is > // followed by one of OnBrowserCloseCanceled() or OnBrowserRemoved(). > OnBrowserStartingClose() Acknowledged. https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/unloa... File chrome/browser/ui/unload_controller.cc (right): https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/unloa... chrome/browser/ui/unload_controller.cc:230: content::Source<Browser>(browser_), On 2017/03/31 14:23:41, sky wrote: > Is it possible to remove this notification now? This notification is used in several places in chrome. Should I do it in separate task?
The CQ bit was checked by aseren@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
cc: gab@ and manzagop@ who might be interested in problems related to the "Shutdown.*" histograms. Thank you for working on this problem. Unfortunately shutdown histograms are currently totally broken. As you know, the current implementation fails to record histograms when Ctrl+Shift+Q is pressed, but at the same time it also fails when manually closing all windows (because KeepAliveRegistry::IsKeepingAlive returns true in Browser::OnWindowClosing). The goal of this change is to call browser_shutdown::OnShutdownStarting() in Browser::OnWindowClosing() to set g_shutdown_type, right? If so, instead of setting IsTryingToQuit() to true with AttemptExitInternal(), how about doing either: 1. Fix Browser::OnWindowClosing()'s logic to call browser_shutdown::OnShutdownStarting() when needed. 2. Calling browser_shutdown::OnShutdownStarting() in chrome::Exit() or in chrome::AttemptUserExit(). I'm not sure if #2 is semantically OK, but it's something similar to what ExitCleanly() does on Chrome OS which calls browser_shutdown::OnShutdownStarting before calling AttemptExitInternal(). Also, please note that shutdown behavior is different among platforms (e.g. on Mac, Chrome does not quit even when the last window is closed) and there are a number of different code paths to trigger shutdown (e.g. Ctrl+Shift+Q and closing all windows). https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:759: chrome::AttemptExit(); On 2017/04/02 12:02:35, aseren wrote: > On 2017/03/31 14:23:41, sky wrote: > > Why is this change necessary? > > This is fix of shutdown histograms on windows when user exits via Ctrl+Shift+Q. > AttemptUserExit() doesn’t call browser_shutdown::SetTryingToQuit(true). So we > cannot start shutdown process correctly and chrome_shutdown_ms.txt is not > created. > See: > https://cs.chromium.org/chromium/src/chrome/browser/lifetime/application_life... > https://cs.chromium.org/chromium/src/chrome/browser/lifetime/application_life... > > chrome::AttemptExit(), оn the contrary, does what is needed. This change looks wrong as the comments in application_lifetime.h says AttemptUserExit() should be used when handling user-initiated shutdown. https://codereview.chromium.org/2793443003/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2793443003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:2565: BrowserList::GetInstance()->currently_closing_browsers_count() + 1; A browser close can be cancelled. What happens if a browser close gets cancelled after a shutdown is started?
https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:759: chrome::AttemptExit(); On 2017/04/04 08:54:07, hashimoto wrote: > On 2017/04/02 12:02:35, aseren wrote: > > On 2017/03/31 14:23:41, sky wrote: > > > Why is this change necessary? > > > > This is fix of shutdown histograms on windows when user exits via > Ctrl+Shift+Q. > > AttemptUserExit() doesn’t call browser_shutdown::SetTryingToQuit(true). So we > > cannot start shutdown process correctly and chrome_shutdown_ms.txt is not > > created. > > See: > > > https://cs.chromium.org/chromium/src/chrome/browser/lifetime/application_life... > > > https://cs.chromium.org/chromium/src/chrome/browser/lifetime/application_life... > > > > chrome::AttemptExit(), оn the contrary, does what is needed. > This change looks wrong as the comments in application_lifetime.h says > AttemptUserExit() should be used when handling user-initiated shutdown. > Agreed. The conditional you point at was added here: https://codereview.chromium.org/25603004 . See it for the rationale as to why the conditional.
On 2017/04/04 16:56:49, sky wrote: > https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... > File chrome/browser/ui/browser_commands.cc (right): > > https://codereview.chromium.org/2793443003/diff/20001/chrome/browser/ui/brows... > chrome/browser/ui/browser_commands.cc:759: chrome::AttemptExit(); > On 2017/04/04 08:54:07, hashimoto wrote: > > On 2017/04/02 12:02:35, aseren wrote: > > > On 2017/03/31 14:23:41, sky wrote: > > > > Why is this change necessary? > > > > > > This is fix of shutdown histograms on windows when user exits via > > Ctrl+Shift+Q. > > > AttemptUserExit() doesn’t call browser_shutdown::SetTryingToQuit(true). So > we > > > cannot start shutdown process correctly and chrome_shutdown_ms.txt is not > > > created. > > > See: > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/lifetime/application_life... > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/lifetime/application_life... > > > > > > chrome::AttemptExit(), оn the contrary, does what is needed. > > This change looks wrong as the comments in application_lifetime.h says > > AttemptUserExit() should be used when handling user-initiated shutdown. > > > > Agreed. The conditional you point at was added here: > https://codereview.chromium.org/25603004 . See it for the rationale as to why > the conditional. hashimoto, sky! Thank you for reviewing this CL. Please find my comments and fixed code below. Thank you for the clarification about AttemptExit function call. I have changed it back to AttemptUserExit. However I think Exit() function doesn't do what is expected. We can see it from comments to SetTryingToQuit function: See: https://cs.chromium.org/chromium/src/chrome/browser/browser_shutdown.h?q=brow... So we should call SetTryingToQuit(true) when the user explicitly chooses to shutdown the app (via the "Exit" or "Quit" menu items). But this is not happened in our case despite the fact that Exit() is called by user action. So I have changed function to call SetTryingToQuit(true). Please see the code diff.
https://codereview.chromium.org/2793443003/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2793443003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:2565: BrowserList::GetInstance()->currently_closing_browsers_count() + 1; On 2017/04/04 08:54:07, hashimoto wrote: > A browser close can be cancelled. > What happens if a browser close gets cancelled after a shutdown is started? If browser close is cancelled then browser will be removed from currently_closing_browsers_ list in following method: BrowserList::NotifyBrowserCloseCancelled. Also shutdown state will be reset in BrowserCloseManager::CancelBrowserClose() method by calling of browser_shutdown::SetTryingToQuit(false). One problem here is that g_shutdown_type remains unchanged. I have fixed this by setting g_shutdown_type to NOT_VALID. https://codereview.chromium.org/2793443003/diff/80001/chrome/browser/browser_... File chrome/browser/browser_shutdown.cc (right): https://codereview.chromium.org/2793443003/diff/80001/chrome/browser/browser_... chrome/browser/browser_shutdown.cc:355: g_shutdown_type = NOT_VALID; reset shutdown state on browser close cancel https://codereview.chromium.org/2793443003/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/2793443003/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:762: browser_shutdown::SetTryingToQuit(true); By comments for SetTryingToQuit function we should call SetTryingToQuit(true) when the User explicitly chooses to shutdown the app (via the "Exit" or "Quit" menu items). See: https://cs.chromium.org/chromium/src/chrome/browser/browser_shutdown.h?q=brow...
On 2017/04/04 08:54:07, hashimoto wrote: > cc: gab@ and manzagop@ who might be interested in problems related to the > "Shutdown.*" histograms. > > Thank you for working on this problem. > > Unfortunately shutdown histograms are currently totally broken. > As you know, the current implementation fails to record histograms when > Ctrl+Shift+Q is pressed, but at the same time it also fails when manually > closing all windows (because KeepAliveRegistry::IsKeepingAlive returns true in > Browser::OnWindowClosing). > > The goal of this change is to call browser_shutdown::OnShutdownStarting() in > Browser::OnWindowClosing() to set g_shutdown_type, right? > If so, instead of setting IsTryingToQuit() to true with AttemptExitInternal(), > how about doing either: > 1. Fix Browser::OnWindowClosing()'s logic to call > browser_shutdown::OnShutdownStarting() when needed. > 2. Calling browser_shutdown::OnShutdownStarting() in chrome::Exit() or in > chrome::AttemptUserExit(). > > I'm not sure if #2 is semantically OK, but it's something similar to what > ExitCleanly() does on Chrome OS which calls browser_shutdown::OnShutdownStarting > before calling AttemptExitInternal(). > Also, please note that shutdown behavior is different among platforms (e.g. on > Mac, Chrome does not quit even when the last window is closed) and there are a > number of different code paths to trigger shutdown (e.g. Ctrl+Shift+Q and > closing all windows). hashimoto, Thank you for the deep explanation of the problem. I think that #1 case is correct. The problem here is that when user explicitly exits (on Windows) browser doesn't indicate it at all. And it seems like a bug. Basing on comments for SetTryingToQuit(bool) it should be called when user explicitly exits. It happens on MacOS but it missed on Windows. What do you think about it? Should we call SetTryingToQuit(true) and how can we do it properly?
hashimoto@chromium.org changed reviewers: + sammc@chromium.org
+sammc Sam, in https://codereview.chromium.org/25603004, AttemptUserExit() stopped calling SetTryingToQuit(true), and it resulted in browser_shutdown::OnShutdownStarting() not called in Browser::OnWindowClosing(). Do we still need to keep this behavior? If so, is there any good place to call browser_shutdown::OnShutdownStarting() when performing shutdown after calling AttemptUserExit()? https://codereview.chromium.org/2793443003/diff/80001/chrome/browser/browser_... File chrome/browser/browser_shutdown.cc (right): https://codereview.chromium.org/2793443003/diff/80001/chrome/browser/browser_... chrome/browser/browser_shutdown.cc:355: g_shutdown_type = NOT_VALID; On 2017/04/09 17:51:37, aseren wrote: > reset shutdown state on browser close cancel I'm not sure if it's safe to allow cancelling shutdown after calling OnShutdownStarting(). OnShutdownStarting() also performs things other than setting g_shutdown_type. Shouldn't we call OnShutdownStarting() after we finally decide to do shutdown? https://codereview.chromium.org/2793443003/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2793443003/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:753: chrome::NOTIFICATION_BROWSER_CLOSING, How about moving this notification code to BrowserList::NotifyBrowserCloseStarted? The same goes for NOTIFICATION_BROWSER_CLOSE_CANCELLED in fast_unload_controller.cc and unload_controller.cc. https://codereview.chromium.org/2793443003/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:2565: BrowserList::GetInstance()->currently_closing_browsers_count() + 1; Sorry, I still don't understand why this change is needed. This function is used only in Browser::OnWindowClosing(), and IIUC it eventually returns true when closing the last Browser. https://codereview.chromium.org/2793443003/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/2793443003/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:762: browser_shutdown::SetTryingToQuit(true); On 2017/04/09 17:51:37, aseren wrote: > By comments for SetTryingToQuit function we should call SetTryingToQuit(true) > when the User explicitly chooses to shutdown the app (via the "Exit" or "Quit" > menu items). See: > https://cs.chromium.org/chromium/src/chrome/browser/browser_shutdown.h?q=brow... IIUC the comment discourages to call SetTryingToQuit(true) directly. In https://codereview.chromium.org/25603004, they intentionally stopped calling SetTryingToQuit(true) for AttemptUserExit(). I'm not still sure if it's still relevant.
On 2017/04/14 08:54:37, hashimoto wrote: > +sammc > > Sam, in https://codereview.chromium.org/25603004, AttemptUserExit() stopped > calling SetTryingToQuit(true), and it resulted in > browser_shutdown::OnShutdownStarting() not called in Browser::OnWindowClosing(). > Do we still need to keep this behavior? SetTryingToQuit(true) bypasses keep-alives so we do need to keep that behaviour. > > If so, is there any good place to call browser_shutdown::OnShutdownStarting() > when performing shutdown after calling AttemptUserExit()? > I'm not sure. Shutdown is rather convoluted.
Description was changed from ========== Fix shutdown on Ctrl+Shift+Q. Fix shutdown races CL fixes several issues: 1. On Windows: proper shutdown handle on Ctrl+Shift+Q 2. Fix races on shutdown between BrowserList::RemoveBrowser and Browser::OnWindowClosing functions R=sky@chromium.org BUG=707147 BUG=707144 ========== to ========== Fix shutdown races on Cmd+Q. Fix races on shutdown between BrowserList::RemoveBrowser and Browser::OnWindowClosing functions R=sky@chromium.org BUG=707147 BUG=707144 ==========
PTAL, It seems that currently shutdown by window closing can be detected only on Mac OS. So I have made shutdown browser test to be compiled only when is_mac is defined. The main purpose of this CL is fixing of races happened in Browser::OnWindowClosing when two or more browsers are closing simultaneously. This can happen if all Browser::OnWindowClosing() will be called before any BrowserList::RemoveBrowser() call. Also CL replaces NOTIFICATION_BROWSER_CLOSING notification by BrowserListObserver::OnBrowserCloseStarted event. https://codereview.chromium.org/2793443003/diff/80001/chrome/browser/browser_... File chrome/browser/browser_shutdown.cc (right): https://codereview.chromium.org/2793443003/diff/80001/chrome/browser/browser_... chrome/browser/browser_shutdown.cc:355: g_shutdown_type = NOT_VALID; On 2017/04/14 08:54:37, hashimoto wrote: > On 2017/04/09 17:51:37, aseren wrote: > > reset shutdown state on browser close cancel > > I'm not sure if it's safe to allow cancelling shutdown after calling > OnShutdownStarting(). > OnShutdownStarting() also performs things other than setting g_shutdown_type. > > Shouldn't we call OnShutdownStarting() after we finally decide to do shutdown? You are right. It is not safe. This CL https://codereview.chromium.org/2695233003 makes some fixes in unload controller. So now OnWindowClosing is called only when all unload events are finished and we cannot cancel browser closing at this moment. So now I can remove code related to cancel processing. https://codereview.chromium.org/2793443003/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2793443003/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:753: chrome::NOTIFICATION_BROWSER_CLOSING, On 2017/04/14 08:54:37, hashimoto wrote: > How about moving this notification code to > BrowserList::NotifyBrowserCloseStarted? > The same goes for NOTIFICATION_BROWSER_CLOSE_CANCELLED in > fast_unload_controller.cc and unload_controller.cc. Acknowledged. https://codereview.chromium.org/2793443003/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:2565: BrowserList::GetInstance()->currently_closing_browsers_count() + 1; On 2017/04/14 08:54:37, hashimoto wrote: > Sorry, I still don't understand why this change is needed. > This function is used only in Browser::OnWindowClosing(), and IIUC it eventually > returns true when closing the last Browser. This fixes the races happened when two browsers are closing simultaneously. How it can happen: Suppose that we have already called Browser::OnWindowClosing() for the first browser and lets suppose that Browser::OnWindowClosing() for the second browser was called before the first browser was closed in BrowserList::RemoveBrowser(). In this case when calling OnWindowClosing() for the second browser we will have BrowserList::GetInstance()->size() == 2. So Browser::ShouldStartShutdown() will return false. To fix this we need to keep track of currently closing browsers list. https://codereview.chromium.org/2793443003/diff/120001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2793443003/diff/120001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:2506: "../browser/browser_shutdown_browsertest.cc", Currently shutdown by windows closing can be detected only on Mac OS. So I have moved the test to the "is_mac" section
Sorry for being late to respond. NOTIFICATION_BROWSER_CLOSING removal looks good. Probably, it's better to create a new separate patch to replace NOTIFICATION_BROWSER_CLOSING with observer, as it's less likely to get objection. https://codereview.chromium.org/2793443003/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2793443003/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:2565: BrowserList::GetInstance()->currently_closing_browsers_count() + 1; On 2017/04/22 19:52:54, aseren wrote: > On 2017/04/14 08:54:37, hashimoto wrote: > > Sorry, I still don't understand why this change is needed. > > This function is used only in Browser::OnWindowClosing(), and IIUC it > eventually > > returns true when closing the last Browser. > > This fixes the races happened when two browsers are closing simultaneously. How > it can happen: > Suppose that we have already called Browser::OnWindowClosing() for the first > browser and lets suppose that Browser::OnWindowClosing() for the second browser > was called before the first browser was closed in BrowserList::RemoveBrowser(). > In this case when calling OnWindowClosing() for the second browser we will have > BrowserList::GetInstance()->size() == 2. > So Browser::ShouldStartShutdown() will return false. > > To fix this we need to keep track of currently closing browsers list. Thank you for the explanation. I don't think it's the right thing to do to return true from ShouldStartShutdown() when the window close can be still cancelled. ShouldStartShutdown's only user is Browser::OnWindowClosing(). Can't we stop calling browser_shutdown::OnShutdownStarting() in Browser::OnWindowClosing(), and instead call browser_shutdown::OnShutdownStarting() somewhere else when window close is actually finished (or the last keep alive is released from the KeepAliveRegistry)? https://codereview.chromium.org/2793443003/diff/120001/chrome/browser/ui/brow... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/2793443003/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_commands.cc:14: #include "chrome/browser/browser_shutdown.h" nit: Looks unneeded.
Thank you for reviewing this CL! Should I do NOTIFICATION_BROWSER_CLOSING removal in separate CL or just add the new patch to current CL? https://codereview.chromium.org/2793443003/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2793443003/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:2565: BrowserList::GetInstance()->currently_closing_browsers_count() + 1; On 2017/04/28 09:16:29, hashimoto wrote: > Thank you for the explanation. > I don't think it's the right thing to do to return true from > ShouldStartShutdown() when the window close can be still cancelled. Actually we cannot cancel shutdown after we have entered Browser::OnWindowClosing() once. This functionality was done by following CL: https://codereview.chromium.org/2695233003 > ShouldStartShutdown's only user is Browser::OnWindowClosing(). > Can't we stop calling browser_shutdown::OnShutdownStarting() in > Browser::OnWindowClosing(), and instead call > browser_shutdown::OnShutdownStarting() somewhere else when window close is > actually finished (or the last keep alive is released from the > KeepAliveRegistry)? Ok, I will try to find right place for this. https://codereview.chromium.org/2793443003/diff/120001/chrome/browser/ui/brow... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/2793443003/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_commands.cc:14: #include "chrome/browser/browser_shutdown.h" On 2017/04/28 09:16:29, hashimoto wrote: > nit: Looks unneeded. Acknowledged.
I'm ok with moving removal to a separate patch, as long as you are going to do it. On Wed, May 3, 2017 at 10:19 AM, <aseren@yandex-team.ru> wrote: > Thank you for reviewing this CL! > Should I do NOTIFICATION_BROWSER_CLOSING removal in separate CL or just > add the > new patch to current CL? > > > https://codereview.chromium.org/2793443003/diff/80001/ > chrome/browser/ui/browser.cc > File chrome/browser/ui/browser.cc (right): > > https://codereview.chromium.org/2793443003/diff/80001/ > chrome/browser/ui/browser.cc#newcode2565 > chrome/browser/ui/browser.cc:2565: > BrowserList::GetInstance()->currently_closing_browsers_count() + 1; > On 2017/04/28 09:16:29, hashimoto wrote: > > Thank you for the explanation. > > I don't think it's the right thing to do to return true from > > ShouldStartShutdown() when the window close can be still cancelled. > > Actually we cannot cancel shutdown after we have entered > Browser::OnWindowClosing() once. This functionality was done by > following CL: > https://codereview.chromium.org/2695233003 > > > ShouldStartShutdown's only user is Browser::OnWindowClosing(). > > Can't we stop calling browser_shutdown::OnShutdownStarting() in > > Browser::OnWindowClosing(), and instead call > > browser_shutdown::OnShutdownStarting() somewhere else when window > close is > > actually finished (or the last keep alive is released from the > > KeepAliveRegistry)? > > Ok, I will try to find right place for this. > > https://codereview.chromium.org/2793443003/diff/120001/ > chrome/browser/ui/browser_commands.cc > File chrome/browser/ui/browser_commands.cc (right): > > https://codereview.chromium.org/2793443003/diff/120001/ > chrome/browser/ui/browser_commands.cc#newcode14 > chrome/browser/ui/browser_commands.cc:14: #include > "chrome/browser/browser_shutdown.h" > On 2017/04/28 09:16:29, hashimoto wrote: > > nit: Looks unneeded. > > Acknowledged. > > https://codereview.chromium.org/2793443003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
PTAL, Last patch set contains replacement of NOTIFICATION_BROWSER_CLOSING with BrowserListObserver::OnBrowserCloseStarted event.
Sorry for being late to respond! I was out of office last week. On 2017/05/08 19:01:57, aseren wrote: > PTAL, > > Last patch set contains replacement of NOTIFICATION_BROWSER_CLOSING with > BrowserListObserver::OnBrowserCloseStarted event. Sorry for being unclear, what I have suggested is uploading a new CL for NOTIFICATION_BROWSER_CLOSING removal. That being said, this change itself looks good. Please update the change description to describe what this change actually does.
Description was changed from ========== Fix shutdown races on Cmd+Q. Fix races on shutdown between BrowserList::RemoveBrowser and Browser::OnWindowClosing functions R=sky@chromium.org BUG=707147 BUG=707144 ========== to ========== Replaced NOTIFICATION_BROWSER_CLOSING by BrowserListObserver::OnBrowserCloseStarted event. R=sky@chromium.org ==========
On 2017/05/09 08:22:08, hashimoto wrote: > Sorry for being late to respond! > I was out of office last week. > On 2017/05/08 19:01:57, aseren wrote: > > PTAL, > > > > Last patch set contains replacement of NOTIFICATION_BROWSER_CLOSING with > > BrowserListObserver::OnBrowserCloseStarted event. > > Sorry for being unclear, what I have suggested is uploading a new CL for > NOTIFICATION_BROWSER_CLOSING removal. > That being said, this change itself looks good. > Please update the change description to describe what this change actually does. Ok, done
lgtm, thanks. Please also get reviewed by one of the owners.
https://codereview.chromium.org/2793443003/diff/140001/chrome/browser/ui/brow... File chrome/browser/ui/browser_list_observer.h (right): https://codereview.chromium.org/2793443003/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser_list_observer.h:19: virtual void OnBrowserCloseStarted(Browser* browser) {} Please name this OnBrowserClosing() with a comment something like: // Called when a Browser starts closing. This is called prior to // removing the tabs. Removing the tabs may delay or stop the close. https://codereview.chromium.org/2793443003/diff/140001/chrome/browser/ui/tabs... File chrome/browser/ui/tabs/pinned_tab_service.cc (right): https://codereview.chromium.org/2793443003/diff/140001/chrome/browser/ui/tabs... chrome/browser/ui/tabs/pinned_tab_service.cc:98: if (IsOnlyNormalBrowser(browser)) { Combine conditionals.
PTAL, Thank you. https://codereview.chromium.org/2793443003/diff/140001/chrome/browser/ui/brow... File chrome/browser/ui/browser_list_observer.h (right): https://codereview.chromium.org/2793443003/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser_list_observer.h:19: virtual void OnBrowserCloseStarted(Browser* browser) {} On 2017/05/09 17:19:28, sky wrote: > Please name this OnBrowserClosing() with a comment something like: > > // Called when a Browser starts closing. This is called prior to > // removing the tabs. Removing the tabs may delay or stop the close. Acknowledged. https://codereview.chromium.org/2793443003/diff/140001/chrome/browser/ui/tabs... File chrome/browser/ui/tabs/pinned_tab_service.cc (right): https://codereview.chromium.org/2793443003/diff/140001/chrome/browser/ui/tabs... chrome/browser/ui/tabs/pinned_tab_service.cc:98: if (IsOnlyNormalBrowser(browser)) { On 2017/05/09 17:19:28, sky wrote: > Combine conditionals. Acknowledged.
LGTM
The CQ bit was checked by aseren@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from hashimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2793443003/#ps160001 (title: "Update CL basing on review comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by aseren@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from hashimoto@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2793443003/#ps180001 (title: "Update CL basing on review comments (rebase)")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Rebased on the latest master
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by aseren@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by aseren@yandex-team.ru
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1494594703579940,
"parent_rev": "ae72e1597e0491443aa7fa4428fbeacd91c41cbd", "commit_rev":
"c474307ec0c335b7bc6877f0181aa67293fd1d7c"}
Message was sent while issue was closed.
Description was changed from ========== Replaced NOTIFICATION_BROWSER_CLOSING by BrowserListObserver::OnBrowserCloseStarted event. R=sky@chromium.org ========== to ========== Replaced NOTIFICATION_BROWSER_CLOSING by BrowserListObserver::OnBrowserCloseStarted event. R=sky@chromium.org Review-Url: https://codereview.chromium.org/2793443003 Cr-Commit-Position: refs/heads/master@{#471292} Committed: https://chromium.googlesource.com/chromium/src/+/c474307ec0c335b7bc6877f0181a... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/c474307ec0c335b7bc6877f0181a... |
