Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7273038:
ChromeOs: Call BrowserList::SessionEnding when shutting down via SIGTERM (Closed)
|
Created:
9 years, 5 months ago by oshima Modified:
9 years, 5 months ago Reviewers:
achuithb CC:
chromium-reviews, joi+watch-content_chromium.org, jam, kkania, Paweł Hajdan Jr., jar (doing other things) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionChromeOs: Call BrowserList::SessionEnding when shutting down via SIGTERM
This will make sure exit_cleanly is written.
CleanUP: renamed MarkAsCleanShutdown to MarkAsCleanShutdowInUserProfiles to reflect what the method really does.
BUG=85936
TEST=Along with crosbug.com/16458 fix, ChromeOS's UMA crash number will decrease to the level of
other platforms.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91394
Patch Set 1 : " #
Total comments: 3
Patch Set 2 : " #Patch Set 3 : " #
Total comments: 5
Patch Set 4 : " #Patch Set 5 : " #
Total comments: 6
Patch Set 6 : " #Messages
Total messages: 12 (0 generated)
It would be nice if the name could be shorter, but I understand that the shorter name is mis-leading. LGTM from me.
This does not yet look good. Please respond the comments, and do not land yet despite the review by achuith.bhandarkar. Thanks, Jim http://codereview.chromium.org/7273038/diff/1023/chrome/browser/metrics/metri... File chrome/browser/metrics/metrics_service.cc (right): http://codereview.chromium.org/7273038/diff/1023/chrome/browser/metrics/metri... chrome/browser/metrics/metrics_service.cc:1235: RecordBooleanPrefValue(prefs::kStabilityExitedCleanly, true); Two things: I think this should ONLY be called on teh UI thread. This was less of an issue when this was private (and we could assert the proper thread in the calling code). Since this is now public, please add an assertion that this is on the proper thread. Second item: This does not push the flag to the disk. It does not even schedule the pushing of the flag to the disk in a "coalesced push of several flags." The name suggests it is much more aggressive, but the code does not do more (push immediately, or even schedule a push). As a result, an immediate shutdown after this call will STILL be listed as a crash in UMA. http://codereview.chromium.org/7273038/diff/1023/chrome/browser/metrics/metri... File chrome/browser/metrics/metrics_service.h (right): http://codereview.chromium.org/7273038/diff/1023/chrome/browser/metrics/metri... chrome/browser/metrics/metrics_service.h:126: void LogCleanShutdown(); Can you motivate better why we want to expose this interface publicly? It seems to shout out to invite callers to call it, and claim we exited cleanly (which it doesn't even do!) http://codereview.chromium.org/7273038/diff/1023/chrome/browser/ui/browser_li... File chrome/browser/ui/browser_list.cc (right): http://codereview.chromium.org/7273038/diff/1023/chrome/browser/ui/browser_li... chrome/browser/ui/browser_list.cc:285: if (!signout) return; style nit: put return on next line, to be more consistent wit the rest of the file (example: line 443).
After studying the shutdown code closely and talked to a couple of people, I'm now convinced that we should simply call BrowserList::SessionEnding when it received signal. This is used to shutdown cleanly when user is logging out on windows and x server has some error on linux, and it does set exit_cleanly bit and makes sure it's written. Jim, we no longer need changes to metrics service. Thank you for steering to right direction. Achuith, could you review this again? I believe this is correct and better.
Am I reading the code incorrectly, or are none of the browser windows closed on shutdown now? http://codereview.chromium.org/7273038/diff/9001/chrome/browser/ui/browser_li... File chrome/browser/ui/browser_list.cc (right): http://codereview.chromium.org/7273038/diff/9001/chrome/browser/ui/browser_li... chrome/browser/ui/browser_list.cc:373: browser_shutdown::GetShutdownType() == browser_shutdown::END_SESSION; session_ending is true when this function is called from SessionEnding(). http://codereview.chromium.org/7273038/diff/9001/chrome/browser/ui/browser_li... chrome/browser/ui/browser_list.cc:377: force_exit = true; force_exit is true http://codereview.chromium.org/7273038/diff/9001/chrome/browser/ui/browser_li... chrome/browser/ui/browser_list.cc:390: return; We return here because force_exit is true. None of the browser windows are actually closed. http://codereview.chromium.org/7273038/diff/9001/chrome/browser/ui/browser_li... chrome/browser/ui/browser_list.cc:490: browser_shutdown::OnShutdownStarting(browser_shutdown::END_SESSION); We set shutdown_type to END_SESSION here. http://codereview.chromium.org/7273038/diff/9001/chrome/browser/ui/browser_li... chrome/browser/ui/browser_list.cc:500: BrowserList::CloseAllBrowsers(); This doesn't actually close any browser windows, as far as I can tell.
Do you think this is the right direction? I will fix force_exit issue if you agree.
I'm hesitant about this for a few reasons. SessionEnding() is an uncommon path right now on ChromeOS, and now it becomes the default shutdown path. This is a pretty big change for some fragile code. SessionEnding() now becomes an alternative exit point for the code. There is some code duplication in BrowserMain() and also CloseAllBrowsersAndExit(). If you look in function BrowserMain(), beyond line 1990 is some more shutdown code that will no longer be called: oom_priority_manager doesn't get destructed. Maybe BrowsesrMainParts too. There may be other auto-vars in this function that are not destructed. singletons are not destructed properly. WatchDogThread tasks aren't stopped. metrics aren't stopped. browser_process isn't released before Shutdown() is called. Logout metrics aren't recorded in the BootTimesLoader.
Ok, uploaded new patch PTAL. On 2011/06/29 22:29:05, achuith.bhandarkar wrote: > I'm hesitant about this for a few reasons. > > SessionEnding() is an uncommon path right now on ChromeOS, and now it becomes > the default shutdown path. This is a pretty big change for some fragile code. > > SessionEnding() now becomes an alternative exit point for the code. There is > some code duplication in BrowserMain() and also CloseAllBrowsersAndExit(). Let's think in terms of what's right instead of if it's fragile. I'm not going to merge this to branch, and we should just fix issue if this is right path. > If you look in function BrowserMain(), beyond line 1990 is some more shutdown > code that will no longer be called: > oom_priority_manager doesn't get destructed. Maybe BrowsesrMainParts too. There > may be other auto-vars in this function that are not destructed. > singletons are not destructed properly. > WatchDogThread tasks aren't stopped. > metrics aren't stopped. > browser_process isn't released before Shutdown() is called. > Logout metrics aren't recorded in the BootTimesLoader. Ok, that's valid point, so I changed to use EndSession().
http://codereview.chromium.org/7273038/diff/6007/chrome/browser/ui/browser_li... File chrome/browser/ui/browser_list.cc (right): http://codereview.chromium.org/7273038/diff/6007/chrome/browser/ui/browser_li... chrome/browser/ui/browser_list.cc:476: MarkAsCleanShutdown(); Are we sure we don't want to call g_browser_process->EndSession() here? If we end up doing that, we should just delete the function MarkAsCleanShutdown. We probably should create a non-blocking version of EndSession that doesn't wait on the FILE thread and call that instead. http://codereview.chromium.org/7273038/diff/6007/chrome/browser/ui/browser_li... chrome/browser/ui/browser_list.cc:484: g_browser_process->EndSession(); This is good. It seems unnecessary that EndSession waits on the FILE thread for upto 10 seconds before it continues with the rest of the shutdown sequence. Do you think that matters?
http://codereview.chromium.org/7273038/diff/6007/chrome/browser/ui/browser_li... File chrome/browser/ui/browser_list.cc (right): http://codereview.chromium.org/7273038/diff/6007/chrome/browser/ui/browser_li... chrome/browser/ui/browser_list.cc:476: MarkAsCleanShutdown(); On 2011/06/30 20:27:55, achuith.bhandarkar wrote: > Are we sure we don't want to call g_browser_process->EndSession() here? > If we end up doing that, we should just delete the function MarkAsCleanShutdown. > We probably should create a non-blocking version of EndSession that doesn't wait > on the FILE thread and call that instead. Yes, this is intentional. We still do not want to show crash infobar when chrome crashes during shutdown, yet we want to report crash to UMA on non-chromeos platform. Blocking is by design to make sure the file gets written before proceeding to next step. http://codereview.chromium.org/7273038/diff/6007/chrome/browser/ui/browser_li... chrome/browser/ui/browser_list.cc:484: g_browser_process->EndSession(); On 2011/06/30 20:27:55, achuith.bhandarkar wrote: > This is good. It seems unnecessary that EndSession waits on the FILE thread for > upto 10 seconds before it continues with the rest of the shutdown sequence. Do > you think that matters? It MAY wait for 10 seconds (i'm most case not). Note that it uses SavePersistentPrefs but not ScheulePersistentPrefs, so it should write it immediately. This is just an timeout for the case that somehow FILE thread didn't finish in 10 seconds.
http://codereview.chromium.org/7273038/diff/6007/chrome/browser/ui/browser_li... File chrome/browser/ui/browser_list.cc (right): http://codereview.chromium.org/7273038/diff/6007/chrome/browser/ui/browser_li... chrome/browser/ui/browser_list.cc:476: MarkAsCleanShutdown(); Ok, that's what I thought - I just wanted to make sure it was intentional. http://codereview.chromium.org/7273038/diff/6007/chrome/browser/ui/browser_li... chrome/browser/ui/browser_list.cc:484: g_browser_process->EndSession(); On 2011/06/30 21:26:33, oshima wrote: > On 2011/06/30 20:27:55, achuith.bhandarkar wrote: > > This is good. It seems unnecessary that EndSession waits on the FILE thread > for > > upto 10 seconds before it continues with the rest of the shutdown sequence. Do > > you think that matters? > > It MAY wait for 10 seconds (i'm most case not). Note that it uses > SavePersistentPrefs but not ScheulePersistentPrefs, so it should write it > immediately. This is just an timeout for the case that somehow FILE thread > didn't finish in 10 seconds. The wait seems unnecessary and unhelpful in our case, since we don't really care when the FILE thread finishes. Don't you agree? Do you think it makes sense to pull out the code to MarkAsCleanShutdown and save metrics in a separate common function (basically EndSession without the blocking part) and call that function instead?
LGTM after talking in person with oshima. |
