Implement lock file for windows profiles.

Implement lock file for windows profiles.

BUG=120389
TEST=Start chrome from two PCs at the same time with shared profile and observe the second instance not starting at all.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147121

[pastarmovj, 2012-07-11 23:02:02]

Hi guys,
this is my proposal for making chrome respect a lock file on windows too. It
seems having a file opened with sharing restriction for reading only and opened
for writing works like a nice verification for single usage. This gives us the
chance to detect the situation where a lock file was created from a machine that
died and couldn't delete it. I observed that files marked for deletion are
actually removed eventually from the OS and until this is done the second open
request will block so the OS (Win7/8) gives us enough guarantees for consistency
across the network.

Please disregard the LOGs in the source code they are there for me to observe
what happens between machines and are not meant to stay there for when this CL
is ready to land.

[cpu_(ooo_6.6-7.5), 2012-07-13 23:16:53]

http://codereview.chromium.org/10702159/diff/4001/chrome/browser/process_sing...
File chrome/browser/process_singleton_win.cc (right):

http://codereview.chromium.org/10702159/diff/4001/chrome/browser/process_sing...
chrome/browser/process_singleton_win.cc:178: // We have to make sure there is no
Chrome instance running on another
why do it if the directory is not remote?

http://codereview.chromium.org/10702159/diff/4001/chrome/browser/process_sing...
chrome/browser/process_singleton_win.cc:246: CloseHandle(lock_file_);
why close it here?

http://codereview.chromium.org/10702159/diff/4001/chrome/browser/process_sing...
chrome/browser/process_singleton_win.cc:341: 
and not sure we need this here either..

[pastarmovj, 2012-07-14 01:21:58]

Thanks for the review, Carlos! Please have a look at my comments on your
questions in the code?

http://codereview.chromium.org/10702159/diff/4001/chrome/browser/process_sing...
File chrome/browser/process_singleton_win.cc (right):

http://codereview.chromium.org/10702159/diff/4001/chrome/browser/process_sing...
chrome/browser/process_singleton_win.cc:178: // We have to make sure there is no
Chrome instance running on another
On 2012/07/13 23:16:53, cpu wrote:
> why do it if the directory is not remote?

To check if a path contains a reparse point hence it is potentially a network
share we have to create(or have) a non-empty file, memory map it and then ask
the OS to tell us if this memory mapped file has reparse points in its name. It
seemed to me cheaper to just try and create an empty file regardless whether we
are on a net share or not as this will actually be cheaper than to check first.
WDYT?
BTW the code below can be made to open or create as needed in a single call
instead of doing it twice in a row. I just did it this way to be able to
investigate and validate the expectations on the error codes set by both
operations separately. So the end code will be as simple as try to open  and let
it be.

http://codereview.chromium.org/10702159/diff/4001/chrome/browser/process_sing...
chrome/browser/process_singleton_win.cc:246: CloseHandle(lock_file_);
On 2012/07/13 23:16:53, cpu wrote:
> why close it here?

Mmmm...clean up...I guess :-) . 
Well basically the file is marked wirh FILE_FLAG_DELETE_ON_CLOSE and the OS will
close the file when we die but why risk it when we can in a controlled way close
the file here?

http://codereview.chromium.org/10702159/diff/4001/chrome/browser/process_sing...
chrome/browser/process_singleton_win.cc:341: 
On 2012/07/13 23:16:53, cpu wrote:
> and not sure we need this here either..

This actually will make the newly started process die if there is already
another chrome instance running somewhere else.
What really misses here is UI to to notify the user why we just exited silently.
But I think for this kind of notification we can't do much better but to show a
message box as we don't really want to power up any of the heavy duty browser
stuff. Even localization might be a tough issue provided that we can't check the
user's pref for locale but only rely on the system locale at best.

[cpu_(ooo_6.6-7.5), 2012-07-16 20:36:31]

http://codereview.chromium.org/10702159/diff/10001/chrome/browser/process_sin...
File chrome/browser/process_singleton_win.cc (right):

http://codereview.chromium.org/10702159/diff/10001/chrome/browser/process_sin...
chrome/browser/process_singleton_win.cc:191: DWORD error = GetLastError();
this version I like better. Now for two local chrome instances how does this
work? it seems that now we have two mechanisms for process exclusion so I don't
think we need to do the file if the remote_window is there and secondly it feels
that if we can't open the file we should query it to see if it is remote.

[pastarmovj, 2012-07-16 21:17:53]

Addressed your question in the code. PTAL.

http://codereview.chromium.org/10702159/diff/10001/chrome/browser/process_sin...
File chrome/browser/process_singleton_win.cc (right):

http://codereview.chromium.org/10702159/diff/10001/chrome/browser/process_sin...
chrome/browser/process_singleton_win.cc:191: DWORD error = GetLastError();
On 2012/07/16 20:36:31, cpu wrote:
> this version I like better. Now for two local chrome instances how does this
> work? it seems that now we have two mechanisms for process exclusion so I
don't
> think we need to do the file if the remote_window is there and secondly it
feels
> that if we can't open the file we should query it to see if it is remote.

For two local instances the first one will acquire the lock file and create the
windows. Whereas the second will find the remote window and will fail to acquire
the lock file and will exit but will first send the appropriate command to the 
other process via the message channel.

I changed the logic as you suggested to only try to create the lock file if the
remote_window_ is null but I don't see why should we query the lock file if it
is remote or not? This information is implicitly avilable to us though the check
for the remote window - we have three cases:
1. No local chrome + no remote chrome: Neither remote window  present nor lock
file - we create both the window and the lock file.
2. Local chrome + no remote chrome: We have remote window and lock file. We
don't even try to create the lock file (as per the newly updated logic) and we
send a message to the other process through the window handle.
3. No local chrome + remote chrome: We have no remote window but we have a lock
file: We fail to recreate the lock file and exit the process writing a log entry
about the reason.

The chance of having both local and remote instance shouldn't happen as this is
exactly what we are guarding against.

[cpu_(ooo_6.6-7.5), 2012-07-17 19:50:26]

lgtm

https://chromiumcodereview.appspot.com/10702159/diff/15002/chrome/browser/pro...
File chrome/browser/process_singleton_win.cc (right):

https://chromiumcodereview.appspot.com/10702159/diff/15002/chrome/browser/pro...
chrome/browser/process_singleton_win.cc:208: // Set the window's title to the
path of our user data directory so oth-
don't break words, move 'other' to next line.

[pastarmovj, 2012-07-17 21:14:27]

Fixed the comment and tested with my filer samba share and all seems to be good.
The share is really because it is located in Munich but the lockfile behaved
correctly and disappeared on its own even when I killed the Chrome process
forcefully.

https://chromiumcodereview.appspot.com/10702159/diff/15002/chrome/browser/pro...
File chrome/browser/process_singleton_win.cc (right):

https://chromiumcodereview.appspot.com/10702159/diff/15002/chrome/browser/pro...
chrome/browser/process_singleton_win.cc:208: // Set the window's title to the
path of our user data directory so oth-
On 2012/07/17 19:50:26, cpu wrote:
> don't break words, move 'other' to next line.

Done.

[commit-bot: I haz the power, 2012-07-17 21:14:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/10702159/14002

[commit-bot: I haz the power, 2012-07-17 22:45:22]

Change committed as 147121