Improve the UI for handling profile lock contention.

Improve the UI for handling profile lock contention.

Linux/GTK only.

BUG=282509
TEST=ln -s fake.com-123 SingletonLock, then run Chromium, then check that [ Quit ] and [ Unlock and Relaunch ] work.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222159

[palmer, 2013-08-30 22:01:09]

Say, erg, am I even vaguely on the right track?

One problem: Chrome seems to hang if you select the Unlock Profile and Relaunch
option. I can't figure out why; I assume the MessageLoop doesn't get the signal
that it should continue, or something. Little help? Thanks!

[palmer, 2013-08-30 22:28:43]

Seems to be working correctly now. PTAL, thanks!

[palmer, 2013-08-30 23:55:44]

Adding estade as an OWNER for chrome/browser/ui/gtk

[Elliot Glaysher, 2013-09-03 18:04:43]

Do we actually want to make unlocking the profile this easy?

Should there be some more dedicated checking for the hostname? Or verifying if
the PID exists?

[palmer, 2013-09-03 20:28:55]

> Do we actually want to make unlocking the profile this easy?

Yes, because annoying people by making them 

* copy the pathname from the dialog
* open a terminal
* run rm

is more of annoyance than it is a correctness/safety guarantee.

> Should there be some more dedicated checking for the hostname? Or verifying if
> the PID exists?

Well, the block that fires off the dialog is already guarded by

    if (hostname != net::GetHostName()) {

but I can add

    if (hostname != net::GetHostName() && !IsChromeProcess(pid)) {

for belt-and-suspenders goodness, if you like. With both of those checks in
place, we should be able to be quite sure that it is a stale lock file and not a
real one.

[Elliot Glaysher, 2013-09-03 20:44:41]

lgtm++ conditional on:

On 2013/09/03 20:28:55, Chromium Palmer wrote:
> but I can add
> 
>     if (hostname != net::GetHostName() && !IsChromeProcess(pid)) {
> 
> for belt-and-suspenders goodness, if you like. With both of those checks in
> place, we should be able to be quite sure that it is a stale lock file and not
a
> real one.

[commit-bot: I haz the power, 2013-09-04 01:18:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/23506011/10001

[palmer, 2013-09-04 01:25:49]

Say, thakis, can you please  review for
chrome/browser/process_singleton_linux.cc ? Thanks.

[commit-bot: I haz the power, 2013-09-04 01:34:29]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Nico, 2013-09-04 15:58:03]

https://codereview.chromium.org/23506011/diff/10001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/23506011/diff/10001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:13737: +      The profile appears to be in
use by another <ph name="PRODUCT_NAME">$3<ex>Google Chrome</ex></ph> process
(<ph name="PROCESS_ID">$1<ex>12345</ex></ph>) on another computer (<ph
name="HOST_NAME">$2<ex>example.com</ex></ph>). (To protect against data
corruption, <ph name="PRODUCT_NAME">$3<ex>Google Chrome</ex></ph> locks profiles
so that only one instance of <ph name="PRODUCT_NAME">$3<ex>Google
Chrome</ex></ph> can use the profile.) If you are sure no other processes are
using this profile, click Unlock Profile and Relaunch. Otherwise, click Quit.
This won't localize well. Put this into the chromium / chrome grd files. See
comment 0 in https://code.google.com/p/chromium/issues/detail?id=111425

[palmer, 2013-09-04 22:28:06]

> This won't localize well. Put this into the chromium / chrome grd files. See
> comment 0 in https://code.google.com/p/chromium/issues/detail?id=111425

Done.

[Nico, 2013-09-04 22:32:58]

Have you checked the dialog text with chrome-ui-review? it looks a bit wordy

https://codereview.chromium.org/23506011/diff/24001/chrome/browser/process_si...
File chrome/browser/process_singleton_linux.cc (right):

https://codereview.chromium.org/23506011/diff/24001/chrome/browser/process_si...
chrome/browser/process_singleton_linux.cc:318: return 0;
don't return 0 from a function returning bool (not sure why the clang bot didn't
catch this)

https://codereview.chromium.org/23506011/diff/24001/chrome/browser/process_si...
chrome/browser/process_singleton_linux.cc:742: if (hostname !=
net::GetHostName() && !IsChromeProcess(pid)) {
Now we won't show the dialog if the process holding the lock is a running chrome
process, which is probably the common case for this dialog, no?

(or am i misreading this?)

[palmer, 2013-09-04 23:12:25]

https://codereview.chromium.org/23506011/diff/24001/chrome/browser/process_si...
File chrome/browser/process_singleton_linux.cc (right):

https://codereview.chromium.org/23506011/diff/24001/chrome/browser/process_si...
chrome/browser/process_singleton_linux.cc:318: return 0;
On 2013/09/04 22:32:59, Nico wrote:
> don't return 0 from a function returning bool (not sure why the clang bot
didn't
> catch this)

Done.

https://codereview.chromium.org/23506011/diff/24001/chrome/browser/process_si...
chrome/browser/process_singleton_linux.cc:742: if (hostname !=
net::GetHostName() && !IsChromeProcess(pid)) {
> Now we won't show the dialog if the process holding the lock is a running
chrome
> process, which is probably the common case for this dialog, no?
> 
> (or am i misreading this?)

You are reading it correctly, but that is not the common case for this dialog.
The common case (arguably, the only case) is when the lock file is stale (e.g.
computer crashed), not live.

[Nico, 2013-09-04 23:13:59]

On 2013/09/04 23:12:25, Chromium Palmer wrote:
>
https://codereview.chromium.org/23506011/diff/24001/chrome/browser/process_si...
> File chrome/browser/process_singleton_linux.cc (right):
> 
>
https://codereview.chromium.org/23506011/diff/24001/chrome/browser/process_si...
> chrome/browser/process_singleton_linux.cc:318: return 0;
> On 2013/09/04 22:32:59, Nico wrote:
> > don't return 0 from a function returning bool (not sure why the clang bot
> didn't
> > catch this)
> 
> Done.
> 
>
https://codereview.chromium.org/23506011/diff/24001/chrome/browser/process_si...
> chrome/browser/process_singleton_linux.cc:742: if (hostname !=
> net::GetHostName() && !IsChromeProcess(pid)) {
> > Now we won't show the dialog if the process holding the lock is a running
> chrome
> > process, which is probably the common case for this dialog, no?
> > 
> > (or am i misreading this?)
> 
> You are reading it correctly, but that is not the common case for this dialog.
> The common case (arguably, the only case) is when the lock file is stale (e.g.
> computer crashed), not live.

Are you sure about this? As far as I know, evan@ added it after lots of people
complained about chrome silently not launching when the ssh -Y'd into their
machines and forgot they are already running chrome in another terminal.

[palmer, 2013-09-04 23:20:33]

> Have you checked the dialog text with chrome-ui-review? it looks a bit wordy

It's no more wordy than it was before (3 lines before and after). Before, it
required you to copy and paste a pathname from the dialog box into a terminal
window and run rm; now, it removes the file for you if you click the button for
that. So it's strictly easier to use now, although sure we could get the wording
reviewed. I go by the principal that people pay more attention to the buttons
than to the text anyway, and now there are usable buttons.

I'll run this by chrome-ui-review though.

[palmer, 2013-09-04 23:23:36]

> > You are reading it correctly, but that is not the common case for this
dialog.
> > The common case (arguably, the only case) is when the lock file is stale
(e.g.
> > computer crashed), not live.
> 
> Are you sure about this? As far as I know, evan@ added it after lots of people
> complained about chrome silently not launching when the ssh -Y'd into their
> machines and forgot they are already running chrome in another terminal.

Hmm, OK. Well, I added the extra check at erg's request. I get this:

cybergoat:~/chromium/src $ export DISPLAY=:0
cybergoat:~/chromium/src $ google-chrome                                        
                                                                                
Created new window in existing browser session.

which seems like explanation enough for the "ssh -Y" case?

[Nico, 2013-09-04 23:32:55]

On 2013/09/04 23:23:36, Chromium Palmer wrote:
> > > You are reading it correctly, but that is not the common case for this
> dialog.
> > > The common case (arguably, the only case) is when the lock file is stale
> (e.g.
> > > computer crashed), not live.
> > 
> > Are you sure about this? As far as I know, evan@ added it after lots of
people
> > complained about chrome silently not launching when the ssh -Y'd into their
> > machines and forgot they are already running chrome in another terminal.
> 
> Hmm, OK. Well, I added the extra check at erg's request. I get this:
> 
> cybergoat:~/chromium/src $ export DISPLAY=:0
> cybergoat:~/chromium/src $ google-chrome                                      
 
>                                                                               
 
> Created new window in existing browser session.
> 
> which seems like explanation enough for the "ssh -Y" case?

Ah, then I'm just misremembering things. lgtm, with a possible future string
change depending on what the ui peeps say. Thanks!

[commit-bot: I haz the power, 2013-09-09 21:39:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/23506011/36001

[commit-bot: I haz the power, 2013-09-09 22:07:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/23506011/39001

[commit-bot: I haz the power, 2013-09-10 00:51:24]

Change committed as 222159

[please use gerrit instead, 2013-09-10 02:03:50]

On 2013/09/10 00:51:24, I haz the power (commit-bot) wrote:
> Change committed as 222159

This change appears to have broken three unit tests:

ProcessSingletonLinuxTest.NotifyOtherProcessDifferingHost
ProcessSingletonLinuxTest.NotifyOtherProcessOrCreate_BadCookie
ProcessSingletonLinuxTest.NotifyOtherProcessOrCreate_DifferingHost

http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29...

Please revert, fix, and reland.

[please use gerrit instead, 2013-09-10 02:05:22]

On 2013/09/10 02:03:50, Rouslan Solomakhin wrote:
> On 2013/09/10 00:51:24, I haz the power (commit-bot) wrote:
> > Change committed as 222159
> 
> This change appears to have broken three unit tests:
> 
> ProcessSingletonLinuxTest.NotifyOtherProcessDifferingHost
> ProcessSingletonLinuxTest.NotifyOtherProcessOrCreate_BadCookie
> ProcessSingletonLinuxTest.NotifyOtherProcessOrCreate_DifferingHost
> 
>
http://build.chromium.org/p/chromium.linux/builders/Linux%2520Tests%2520%2528...
> 
> Please revert, fix, and reland.

Also here:
http://build.chromium.org/p/chromium.linux/builders/Linux%20Clang%20%28dbg%29...

I'll revert.