Continue browser startup after killing a hung browser instance.

Continue browser startup after killing a hung browser instance.

Chrome will defer to an existing browser via the process singleton
during startup. When the existing browser appears hung, Chrome will
prompt the user to kill it. If the user consents, Chrome will now try to
acquire the singleton and continue with startup rather than exit.

BUG=603698
TEST=see bug

Committed: https://crrev.com/668156784598d74a8747b7767c1e565e03d9dc89
Cr-Commit-Position: refs/heads/master@{#415121}

[gcomanici, 2016-08-23 20:28:39]

Description was changed from

==========
Fix failed browser rendevous.

Change addresses a failed browser rendevous. When a Chrome relaunch is
expected while chrome is hung, the modification allows for the relaunch
to proceed.

BUG=603698
==========

to

==========
Fix failed browser rendezvous.

Change addresses a failed browser rendezvous. When a Chrome relaunch is
expected while chrome is hung, the modification allows for the relaunch
to proceed.

What was the sequence of events that was creating the problem?
(1) Launch chrome. Hang it by going to chrome://uithreadhang
(2) Launch chrome again. Get offered the relaunch dialog. Click yes.

Behavior before fix? 
The hung chrome would get killed but would not relaunch.

Behavior after the fix?
Chrome relaunches.

BUG=603698
==========

[gcomanici, 2016-08-23 20:28:39]

gcomanici@chromium.org changed reviewers:
+ sky@chromium.org

[gcomanici, 2016-08-23 20:33:08]

gcomanici@chromium.org changed reviewers:
+ grt@chromium.org
- sky@chromium.org

[grt (UTC plus 2), 2016-08-24 10:01:27]

This is going the right direction. Please add a test for this scenario. Also,
please keep the CL description limited to the reason for the change and what it
accomplishes. It can be useful to put a TEST= line with test instructions for
the test engineering team. If the steps to repro/test are in the bug, you can
just put "TEST=See bug" so as to not have redundant info. I think a good
description for this CL could be (while there's no rule for it, wrapping
descriptions at 72 chars makes for friendly git logs.):

Continue browser startup after killing a hung browser instance.

Chrome will defer to an existing browser via the process singleton
during startup. When the existing browser appears hung, Chrome will
prompt the user to kill it. If the user consents, Chrome will now try to
acquire the singleton and continue with startup rather than exit.

BUG=603698
TEST=See bug

https://codereview.chromium.org/2271833002/diff/1/chrome/browser/process_sing...
File chrome/browser/process_singleton_win.cc (right):

https://codereview.chromium.org/2271833002/diff/1/chrome/browser/process_sing...
chrome/browser/process_singleton_win.cc:293: result = NotifyOtherProcess();
this will return PROCESS_NONE if AttemptToNotifyRunningChrome fails for any
reason, not only for hangs. i think it makes sense to retry in these cases, but
not an infloop like this.

this change also removes PROFILE_IN_USE entirely, which doesn't seem right. i
think there are still cases where there's an existing browser process that has
the singleton and isn't (or can't be) terminated.

https://codereview.chromium.org/2271833002/diff/1/chrome/browser/process_sing...
chrome/browser/process_singleton_win.cc:295:
g_browser_process->platform_part()->PlatformSpecificCommandLineProcessing(
this must only happen when Create() returns true

[gcomanici, 2016-08-24 13:57:01]

Description was changed from

==========
Fix failed browser rendezvous.

Change addresses a failed browser rendezvous. When a Chrome relaunch is
expected while chrome is hung, the modification allows for the relaunch
to proceed.

What was the sequence of events that was creating the problem?
(1) Launch chrome. Hang it by going to chrome://uithreadhang
(2) Launch chrome again. Get offered the relaunch dialog. Click yes.

Behavior before fix? 
The hung chrome would get killed but would not relaunch.

Behavior after the fix?
Chrome relaunches.

BUG=603698
==========

to

==========
Continue browser startup after killing a hung browser instance.

Chrome will defer to an existing browser via the process singleton
during startup. When the existing browser appears hung, Chrome will
prompt the user to kill it. If the user consents, Chrome will now try to
acquire the singleton and continue with startup rather than exit.

BUG=603698
TEST=see bug
==========

[gcomanici, 2016-08-25 19:26:07]

The unit tests were updated to reflect the behavior that will solve the problem.
Returning PROFILE_IN_USE after a hung process is killed will not change the
behavior (i.e. the browser startup does not proceed).

https://codereview.chromium.org/2271833002/diff/1/chrome/browser/process_sing...
File chrome/browser/process_singleton_win.cc (right):

https://codereview.chromium.org/2271833002/diff/1/chrome/browser/process_sing...
chrome/browser/process_singleton_win.cc:293: result = NotifyOtherProcess();
On 2016/08/24 10:01:26, grt (UTC plus 2) wrote:
> this will return PROCESS_NONE if AttemptToNotifyRunningChrome fails for any
> reason, not only for hangs. i think it makes sense to retry in these cases,
but
> not an infloop like this.
> 
> this change also removes PROFILE_IN_USE entirely, which doesn't seem right. i
> think there are still cases where there's an existing browser process that has
> the singleton and isn't (or can't be) terminated.

The new patch now runs this for the case when the process was hung and killed.
Also, it attempts only once instead of the infinite loop.

https://codereview.chromium.org/2271833002/diff/1/chrome/browser/process_sing...
chrome/browser/process_singleton_win.cc:295:
g_browser_process->platform_part()->PlatformSpecificCommandLineProcessing(
On 2016/08/24 10:01:26, grt (UTC plus 2) wrote:
> this must only happen when Create() returns true

Done.

[grt (UTC plus 2), 2016-08-26 09:26:43]

https://codereview.chromium.org/2271833002/diff/20001/chrome/browser/process_...
File chrome/browser/process_singleton_win.cc (right):

https://codereview.chromium.org/2271833002/diff/20001/chrome/browser/process_...
chrome/browser/process_singleton_win.cc:294: if (!Create()) {
nit: omit braces for conditionals where the condition and the body are each one
line.

https://codereview.chromium.org/2271833002/diff/20001/chrome/browser/process_...
chrome/browser/process_singleton_win.cc:295: result = NotifyOtherProcess();
instead of duplicating the calls to create and notify, i think it's cleaner to
use a loop:

  for (int i = 0; i < 2; ++i) {
    if (Create())
      return PROCESS_NONE;  // This is the single browser process.
    NotifyResult result = NotifyOtherProcess();
    if (result == PROCESS_NOTIFIED || result == LOCK_ERROR) {
      // The single browser process was notified, the user chose not to
      // terminate a hung browser, or the lock file could not be created.
      // Nothing more to do.
      return result;
    }
    DCHECK_EQ(PROCESS_NONE, result);
    // The process could not be notified for some reason, or it was hung and
    // terminated. Retry once if this is the first time; otherwise, fall through
    // to report that the process must exit because the profile is in use.
  }
  return PROFILE_IN_USE;

https://codereview.chromium.org/2271833002/diff/20001/chrome/browser/process_...
chrome/browser/process_singleton_win.cc:306:
g_browser_process->platform_part()->PlatformSpecificCommandLineProcessing(
looking at the design in here a bit more, it seems to me that the intent is to
not have dependencies on Chrome: this is what ChromeProcessSingleton is for. i
suggest moving this call into
ChromeProcessSingleton::NotifyOtherProcessOrCreate() (and clean up the #includes
in this file).

https://codereview.chromium.org/2271833002/diff/20001/chrome/browser/process_...
File chrome/browser/process_singleton_win_unittest.cc (right):

https://codereview.chromium.org/2271833002/diff/20001/chrome/browser/process_...
chrome/browser/process_singleton_win_unittest.cc:268: // The hung process was
killed and the nitification is equivalent to
notification

[gcomanici, 2016-08-26 17:18:31]

Patchset #3 (id:40001) has been deleted

[gcomanici, 2016-08-26 17:27:00]

Thank you for the the useful comments+tips. Please let me know if you think that
PlatformSpecificCommandLineProcessing should be in
ChromeProcessSingleton::NotifyOtherProcessOrCreate() instead of
ChromeBrowserMainParts::PreMainMessageLoopRunImpl() as the latest patch does.

https://codereview.chromium.org/2271833002/diff/20001/chrome/browser/process_...
File chrome/browser/process_singleton_win.cc (right):

https://codereview.chromium.org/2271833002/diff/20001/chrome/browser/process_...
chrome/browser/process_singleton_win.cc:294: if (!Create()) {
On 2016/08/26 09:26:43, grt (UTC plus 2) wrote:
> nit: omit braces for conditionals where the condition and the body are each
one
> line.

Acknowledged.

https://codereview.chromium.org/2271833002/diff/20001/chrome/browser/process_...
chrome/browser/process_singleton_win.cc:295: result = NotifyOtherProcess();
On 2016/08/26 09:26:43, grt (UTC plus 2) wrote:
> instead of duplicating the calls to create and notify, i think it's cleaner to
> use a loop:
> 
>   for (int i = 0; i < 2; ++i) {
>     if (Create())
>       return PROCESS_NONE;  // This is the single browser process.
>     NotifyResult result = NotifyOtherProcess();
>     if (result == PROCESS_NOTIFIED || result == LOCK_ERROR) {
>       // The single browser process was notified, the user chose not to
>       // terminate a hung browser, or the lock file could not be created.
>       // Nothing more to do.
>       return result;
>     }
>     DCHECK_EQ(PROCESS_NONE, result);
>     // The process could not be notified for some reason, or it was hung and
>     // terminated. Retry once if this is the first time; otherwise, fall
through
>     // to report that the process must exit because the profile is in use.
>   }
>   return PROFILE_IN_USE;

Done.

https://codereview.chromium.org/2271833002/diff/20001/chrome/browser/process_...
chrome/browser/process_singleton_win.cc:306:
g_browser_process->platform_part()->PlatformSpecificCommandLineProcessing(
On 2016/08/26 09:26:43, grt (UTC plus 2) wrote:
> looking at the design in here a bit more, it seems to me that the intent is to
> not have dependencies on Chrome: this is what ChromeProcessSingleton is for. i
> suggest moving this call into
> ChromeProcessSingleton::NotifyOtherProcessOrCreate() (and clean up the
#includes
> in this file).

The new patch has this call in chrome_browser_main.cc. I looked into
ChromeProcessSingleton::NotifyOtherProcessOrCreate() and there is no need to
have the call inside NotifyOtherProcessOrCreate(). It would only create more
imports in chrome_process_singleton.cc.

https://codereview.chromium.org/2271833002/diff/20001/chrome/browser/process_...
File chrome/browser/process_singleton_win_unittest.cc (right):

https://codereview.chromium.org/2271833002/diff/20001/chrome/browser/process_...
chrome/browser/process_singleton_win_unittest.cc:268: // The hung process was
killed and the nitification is equivalent to
On 2016/08/26 09:26:43, grt (UTC plus 2) wrote:
> notification

Done.

[gcomanici, 2016-08-26 18:37:54]

The CQ bit was checked by gcomanici@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 18:38:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 20:04:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 20:04:48]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2016-08-26 20:24:01]

i like where you moved the silly command line thing. nice change. lgtm.

https://codereview.chromium.org/2271833002/diff/80001/chrome/browser/process_...
File chrome/browser/process_singleton_win.cc (right):

https://codereview.chromium.org/2271833002/diff/80001/chrome/browser/process_...
chrome/browser/process_singleton_win.cc:285: if (Create()) {
nit: omit braces for single-line conditional and body

[gcomanici, 2016-08-26 20:46:50]

gcomanici@chromium.org changed reviewers:
+ jochen@chromium.org

[gcomanici, 2016-08-26 20:48:47]

Asking for an Owners Review on the modified files.

[jochen (gone - plz use gerrit), 2016-08-29 09:23:34]

lgtm

[gcomanici, 2016-08-29 13:42:51]

The CQ bit was checked by gcomanici@chromium.org

[gcomanici, 2016-08-29 13:42:51]

The CQ bit was unchecked by gcomanici@chromium.org

[gcomanici, 2016-08-29 13:43:00]

The CQ bit was checked by gcomanici@chromium.org

[commit-bot: I haz the power, 2016-08-29 13:43:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-29 14:51:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-29 14:51:17]

Exceeded global retry quota

[manzagop (departed), 2016-08-29 18:13:08]

The CQ bit was checked by manzagop@chromium.org

[commit-bot: I haz the power, 2016-08-29 18:24:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 04:54:28]

Description was changed from

==========
Continue browser startup after killing a hung browser instance.

Chrome will defer to an existing browser via the process singleton
during startup. When the existing browser appears hung, Chrome will
prompt the user to kill it. If the user consents, Chrome will now try to
acquire the singleton and continue with startup rather than exit.

BUG=603698
TEST=see bug
==========

to

==========
Continue browser startup after killing a hung browser instance.

Chrome will defer to an existing browser via the process singleton
during startup. When the existing browser appears hung, Chrome will
prompt the user to kill it. If the user consents, Chrome will now try to
acquire the singleton and continue with startup rather than exit.

BUG=603698
TEST=see bug
==========

[commit-bot: I haz the power, 2016-08-30 04:54:31]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-08-30 04:56:01]

Description was changed from

==========
Continue browser startup after killing a hung browser instance.

Chrome will defer to an existing browser via the process singleton
during startup. When the existing browser appears hung, Chrome will
prompt the user to kill it. If the user consents, Chrome will now try to
acquire the singleton and continue with startup rather than exit.

BUG=603698
TEST=see bug
==========

to

==========
Continue browser startup after killing a hung browser instance.

Chrome will defer to an existing browser via the process singleton
during startup. When the existing browser appears hung, Chrome will
prompt the user to kill it. If the user consents, Chrome will now try to
acquire the singleton and continue with startup rather than exit.

BUG=603698
TEST=see bug

Committed: https://crrev.com/668156784598d74a8747b7767c1e565e03d9dc89
Cr-Commit-Position: refs/heads/master@{#415121}
==========

[commit-bot: I haz the power, 2016-08-30 04:56:02]

Patchset 4 (id:??) landed as
https://crrev.com/668156784598d74a8747b7767c1e565e03d9dc89
Cr-Commit-Position: refs/heads/master@{#415121}