Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11099053:
Refactor ProcessSingleton to allow a Desktop process to activate Metro Chrome cleanly. (Closed)
|
Created:
8 years, 2 months ago by gab Modified:
7 years, 9 months ago CC:
chromium-reviews, chrome-win8-eng_google.com, Jói Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRefactor ProcessSingleton to allow a Desktop process to activate Metro Chrome instead of grabbing the singleton (while making sure it simply RDVs if there already is an existing Chrome).
Use a Windows event to let the Metro process notify the Desktop process once it grabbed the singleton (no more Sleep()).
The idea is the following:
0) Any process always tries to rendez-vous before anything else.
1) A Metro process can always try to grab the singleton.
2) A Desktop process might wait if:
a) The metro activation event exists
b) The metro activation event doesn't exist, but ShouldActivate() returns true
- In which case it creates the metro activation event and activates Metro Chrome (which will notify this process via the event once it grabbed the singleton; at which point this process should rendez-vous with it).
All of this is done while holding a global mutex (only releasing it while waiting and re-acquiring it before continuing when notified).
The process singleton acquiring logic has also been moved from the constructor to Create().
Design = http://crbug.com/159869#c17
BUG=155585, 159869
TEST=Process singleton works as before (and better in tricky corner cases -- see http://crbug.com/159869) on Win8.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171105
Patch Set 1 #Patch Set 2 : Do check and activate in ProcessSingleton::NotifyProcess() instead #
Total comments: 12
Patch Set 3 : grt comments #Patch Set 4 : New method #Patch Set 5 : merge up to r167683 #
Total comments: 29
Patch Set 6 : apply comments in place #
Total comments: 8
Patch Set 7 : merge #Patch Set 8 : move constructor logic to Create() and modify NotifyOtherProcessOrCreate() #Patch Set 9 : change comment #
Total comments: 7
Patch Set 10 : @robert: comment tweak #
Total comments: 6
Patch Set 11 : merge up to r170805 #Patch Set 12 : addressed grt's comments #Patch Set 13 : merge up to r170957 #
Total comments: 3
Messages
Total messages: 27 (0 generated)
This works for the uninstall case, but now when I launch from command-line, with pref=Metro, and no running Chrome... Metro screen comes and goes and Chrome launches in Desktop.... any ideas? I was able once to have it go in Metro while debugging so maybe it's a race... couldn't repro though... will see tomorrow, but if any of you has a quick idea?
On 2012/10/10 22:26:29, gab wrote: > This works for the uninstall case, but now when I launch from command-line, with > pref=Metro, and no running Chrome... Metro screen comes and goes and Chrome > launches in Desktop.... any ideas? I was able once to have it go in Metro while > debugging so maybe it's a race... couldn't repro though... will see tomorrow, > but if any of you has a quick idea? Hi Gab I just landed this https://codereview.chromium.org/11066102/ to fix the uninstall and relaunch in desktop issues. FWIW, would removing the DCHECK in ActivateInMetro be good enough for now?. Thanks Ananta
On 2012/10/10 22:31:56, ananta wrote: > On 2012/10/10 22:26:29, gab wrote: > > This works for the uninstall case, but now when I launch from command-line, > with > > pref=Metro, and no running Chrome... Metro screen comes and goes and Chrome > > launches in Desktop.... any ideas? I was able once to have it go in Metro > while > > debugging so maybe it's a race... couldn't repro though... will see tomorrow, > > but if any of you has a quick idea? > > Hi Gab > > I just landed this https://codereview.chromium.org/11066102/ to fix the > uninstall and relaunch in desktop issues. I replied directly on your CL, not sure I like the hack as it just hides the problem which is there for all short-lived chrome.exe switches. > > FWIW, would removing the DCHECK in ActivateInMetro be good enough for now?. Which DCHECK? To fix which problem? If you are referring to http://crbug.com/155079, I already have an upcoming fix for it: http://codereview.chromium.org/11103008/ > > Thanks > Ananta
does it work if you move the immersive launch into ProcessSingleton::NotifyOtherProcessOrCreate and have that func return PROCESS_NOTIFIED so that the desktop chrome exits? this makes more intuitive sense to me. on the surface, it looks like what you have right now will continue to run desktop Chrome, so the metro Chrome will just rendezvouses back to it.
Moved activation check in ProcessSingleton::NotifyProcess(). All currently known activation bugs are still there, but the uninstall bug (and other short-lived commands issues) are gone (i.e. behavior is the same, but short-lived action can now run and exit before Chrome is activated (without requiring hacks to skip activating Chrome when these flags are present... which is bug-prone and not scalable imo...). PTAL. Cheers, Gab
http://codereview.chromium.org/11099053/diff/5002/chrome/browser/process_sing... File chrome/browser/process_singleton.h (right): http://codereview.chromium.org/11099053/diff/5002/chrome/browser/process_sing... chrome/browser/process_singleton.h:175: // constructing this process singleton the please put the multi-line comment on the line above the variable, and consider making it a little less verbose: // Is the |user_data_dir| used at construction the user's default (see // ShouldLaunchInWindows8ImmersiveMode). http://codereview.chromium.org/11099053/diff/5002/chrome/browser/process_sing... File chrome/browser/process_singleton_win.cc (right): http://codereview.chromium.org/11099053/diff/5002/chrome/browser/process_sing... chrome/browser/process_singleton_win.cc:203: // Note: Should this check no longer be required, the there are many cases where a member is used in only one function. i don't believe we denote these with code comments. please remove. http://codereview.chromium.org/11099053/diff/5002/chrome/browser/process_sing... chrome/browser/process_singleton_win.cc:211: if (reg_key.Create(HKEY_CURRENT_USER, chrome::kMetroRegistryPath, KEY_READ) i think it's more readable if this is wrapped just before KEY_READ http://codereview.chromium.org/11099053/diff/5002/chrome/browser/process_sing... chrome/browser/process_singleton_win.cc:213: reg_key.ReadValueDW(chrome::kLaunchModeValue, ®_value) here as well, please wrap at ®_value so that "== ERROR_SUCCESS" isn't all alone down there. :-) http://codereview.chromium.org/11099053/diff/5002/chrome/browser/process_sing... chrome/browser/process_singleton_win.cc:258: is_virtualized_(false), lock_file_(INVALID_HANDLE_VALUE), nit: this line and the next should be aligned with the 'w' in "window_" above http://codereview.chromium.org/11099053/diff/5002/chrome/browser/process_sing... chrome/browser/process_singleton_win.cc:356: return PROCESS_NOTIFIED; i'm totally confused now. i thought the idea was that this would rendezvous with the metro chrome process and send its command line across by way of the singleton. is that not the case?
Addressed comments, but because of a bug in setup.exe (causing me to test the wrong binary...) it turns out what I thought was working actually doesn't. This change works if a Metro Chrome is already running, but it doesn't if no Metro Chrome is running as it fails to activate (most likely because at this point the singleton is already held by the process trying to activate). Further refactoring of the process singleton code will be required: http://crbug.com/155585 http://codereview.chromium.org/11099053/diff/5002/chrome/browser/process_sing... File chrome/browser/process_singleton.h (right): http://codereview.chromium.org/11099053/diff/5002/chrome/browser/process_sing... chrome/browser/process_singleton.h:175: // constructing this process singleton the On 2012/10/12 12:49:08, grt wrote: > please put the multi-line comment on the line above the variable, and consider > making it a little less verbose: > > // Is the |user_data_dir| used at construction the user's default (see > // ShouldLaunchInWindows8ImmersiveMode). Done. http://codereview.chromium.org/11099053/diff/5002/chrome/browser/process_sing... File chrome/browser/process_singleton_win.cc (right): http://codereview.chromium.org/11099053/diff/5002/chrome/browser/process_sing... chrome/browser/process_singleton_win.cc:203: // Note: Should this check no longer be required, the On 2012/10/12 12:49:08, grt wrote: > there are many cases where a member is used in only one function. i don't > believe we denote these with code comments. please remove. Done. http://codereview.chromium.org/11099053/diff/5002/chrome/browser/process_sing... chrome/browser/process_singleton_win.cc:211: if (reg_key.Create(HKEY_CURRENT_USER, chrome::kMetroRegistryPath, KEY_READ) On 2012/10/12 12:49:08, grt wrote: > i think it's more readable if this is wrapped just before KEY_READ Done. http://codereview.chromium.org/11099053/diff/5002/chrome/browser/process_sing... chrome/browser/process_singleton_win.cc:213: reg_key.ReadValueDW(chrome::kLaunchModeValue, ®_value) On 2012/10/12 12:49:08, grt wrote: > here as well, please wrap at ®_value so that "== ERROR_SUCCESS" isn't all > alone down there. :-) Done. http://codereview.chromium.org/11099053/diff/5002/chrome/browser/process_sing... chrome/browser/process_singleton_win.cc:258: is_virtualized_(false), lock_file_(INVALID_HANDLE_VALUE), On 2012/10/12 12:49:08, grt wrote: > nit: this line and the next should be aligned with the 'w' in "window_" above Done. http://codereview.chromium.org/11099053/diff/5002/chrome/browser/process_sing... chrome/browser/process_singleton_win.cc:356: return PROCESS_NOTIFIED; On 2012/10/12 12:49:08, grt wrote: > i'm totally confused now. i thought the idea was that this would rendezvous > with the metro chrome process and send its command line across by way of the > singleton. is that not the case? Right, so the problem is I ended testing the wrong binary because of a bug in setup.exe which lead me into thinking this worked when it doesn't.... Moved this back to NotifyOtherProcessOrCreate() as it makes more sense to Activate there (i.e. it's allowed to create) whereas I feel NotifyOtherProcess() should never take action and create something.
This patch won't work as written. The remote_window_ member contains the HWND of the running chrome.exe process. The new instance which is created attempts to defer to this process. This member is populated in the constructor of the ProcessSingletonWin class. You would need to move this logic out to NotifyOtherProcess for the whole thing to work. We need to find the remote window after the metro chrome process has been activated. This is to pass parameters if any to the metro chrome process.
Folks, PTAL, the new flow chart is up on the white boards in Montreal for you to compare (@ananta I can send you a picture if that would help; I updated the description of this CL with the basic idea). I looked into testing; looks like there is basic testing in place for starting multiple Chromes in process_singleton_browsertest.cc which inherently tests ProcessSingleton. However it wouldn't test any of the added activation logic. It wouldn't be too hard to write basic tests for this, but I don't how to granularly test (i.e. catch a bug on first pass rather than becoming flaky on a regression) the concurrency controls without adding gating code in the implementation; are we fine with some high level testing that would (probably) eventually flake if a regression was introduced (e.g. Set pref to Metro, launch Desktop Chrome, make sure that the only running Chrome process is in Metro; etc.)? Cheers! Gab
FYI, I also noticed while doing this that we use the same window class, mutex, and now event name for Chromium, Chrome, and Canary. Should we rectify that? Cheers, Gab
Note: I posted an image representing the new flow at http://crbug.com/159869#c17. (see other messages above for details) Cheers, Gab
http://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_sin... File chrome/browser/process_singleton_win.cc (right): http://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_sin... chrome/browser/process_singleton_win.cc:289: // As of Win8, it is possible that a Chrome process be launched in Desktop There is too much code executing in the constructor of this class. Is there a way to refactor that out?
looking good, a question and a nit http://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_sin... File chrome/browser/process_singleton_win.cc (right): http://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_sin... chrome/browser/process_singleton_win.cc:292: if (!remote_window_ && !base::win::IsMetroProcess()) { This whole code path only needs to be taken on Win8 right? http://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_sin... chrome/browser/process_singleton_win.cc:296: // to be signaled by Metro Chrome which will do so as soon as it grabbed nit: grabbed -> grabs
round one.
are there any codepaths that instantiate the singleton but don't call
NotifyOtherProcess{,OrCreate}? if so, this change will cause metro chrome to
launch, which would surely be a surprise.
https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si...
File chrome/browser/process_singleton_win.cc (right):
https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si...
chrome/browser/process_singleton_win.cc:260: static const wchar_t mutex_name[] =
L"Local\\ChromeProcessSingletonStartup!";
mutex_name -> kMutexName
https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si...
chrome/browser/process_singleton_win.cc:261: static const wchar_t
metro_activation_event_name[] =
metro_activation_event_name -> kMetroActivationEventName
https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si...
chrome/browser/process_singleton_win.cc:274: DCHECK(only_me.Get() != NULL) <<
"GetLastError = " << GetLastError();
DPCHECK(only_me.IsValid());
https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si...
chrome/browser/process_singleton_win.cc:276: // This is how we acquire the mutex
(as opposed to the initial ownership).
since this change introduces more trickery with the mutex, i'd like to see
something akin to base::AutoLock used here.
https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si...
chrome/browser/process_singleton_win.cc:278: DCHECK(result == WAIT_OBJECT_0) <<
"Result = " << result
DCHECK_EQ(result, WAIT_OBJECT_0) and don't stream out result, or would that
require an ugly cast?
https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si...
chrome/browser/process_singleton_win.cc:289: // As of Win8, it is possible that
a Chrome process be launched in Desktop
suggested comment:
// In Win8+, a new Chrome process launched in Desktop mode may need to be
// transmuted into metro Chrome (see ShouldLaunchInWindows8ImmersiveMode for
// heuristics). To accomplish this, the current Chrome activates metro Chrome,
// releases the startup mutex, and waits for metro Chrome to take the
// singleton. From that point onward, the command line for this Chrome process
// will be sent to metro Chrome by the usual channels.
https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si...
chrome/browser/process_singleton_win.cc:320: BOOL released =
ReleaseMutex(only_me);
something akin to base::AutoUnlock should be used here.
https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si...
chrome/browser/process_singleton_win.cc:321: DCHECK(released) << "GetLastError =
" << GetLastError();
DPCHECK(released);
https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si...
chrome/browser/process_singleton_win.cc:334: // the singleton and launch in
Desktop mode).
remove close paren
https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si...
chrome/browser/process_singleton_win.cc:378: // succeeded (i.e. this is the
Metro process) or failed.
comma after "i.e."
https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si...
chrome/browser/process_singleton_win.cc:379: base::win::ScopedHandle
metro_activation_event(
move this up between lines 374 and 375 so that other procs are only notified if
the window was created. also, only do this on win8+.
https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si...
chrome/browser/process_singleton_win.cc:385: DCHECK(released) << "GetLastError =
" << GetLastError();
DPCHECK(released);
Addressed comments and moved "grabbing the singleton" from the constructor to Create(); it doesn't make any difference in browser startup as processes that couldn't grab the singleton in the constructor would still continue until NotifyOtherProcessOrCreate() was later called at chrome_browser_main.cc:1001. To keep things easily reviewable: patch set 6 contains all the code changes, patch set 8 is a pure code move from the constructor to Create(). joi@ (CC'ed) originally moved the Create() code to the constructor as part of http://crrev.com/121480; seems like goal was to make sure the mutex was held for the entire process of grabbing the singleton (which it is still now since the entire process was moved to Create()) Rephrasing one of brettw's TODO (cc'ed for FYI); I think the linux implementation of NotifyOtherProcessOrCreate() could be made identical to the Windows implementation. In a perfect world, NotifyOtherProcessOrCreate() and maybe even the tests could be non-platform-specific imo. PTAL! Cheers, Gab https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:260: static const wchar_t mutex_name[] = L"Local\\ChromeProcessSingletonStartup!"; On 2012/11/16 03:04:41, grt wrote: > mutex_name -> kMutexName Done. https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:261: static const wchar_t metro_activation_event_name[] = On 2012/11/16 03:04:41, grt wrote: > metro_activation_event_name -> kMetroActivationEventName Done. https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:274: DCHECK(only_me.Get() != NULL) << "GetLastError = " << GetLastError(); On 2012/11/16 03:04:41, grt wrote: > DPCHECK(only_me.IsValid()); Done. https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:276: // This is how we acquire the mutex (as opposed to the initial ownership). On 2012/11/16 03:04:41, grt wrote: > since this change introduces more trickery with the mutex, i'd like to see > something akin to base::AutoLock used here. Done, introduced new local classes AutoLockMutex and AutoUnlockMutex. https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:278: DCHECK(result == WAIT_OBJECT_0) << "Result = " << result On 2012/11/16 03:04:41, grt wrote: > DCHECK_EQ(result, WAIT_OBJECT_0) and don't stream out result, or would that > require an ugly cast? Made them all DPCHECKs instead, removing the GetLastError() call in each place, DPCHECK_EQ doesn't exist, I was tempted to add it, but these don't look like a simple set of macros... https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:289: // As of Win8, it is possible that a Chrome process be launched in Desktop On 2012/11/16 03:04:41, grt wrote: > suggested comment: > // In Win8+, a new Chrome process launched in Desktop mode may need to be > // transmuted into metro Chrome (see ShouldLaunchInWindows8ImmersiveMode for > // heuristics). To accomplish this, the current Chrome activates metro Chrome, > // releases the startup mutex, and waits for metro Chrome to take the > // singleton. From that point onward, the command line for this Chrome process > // will be sent to metro Chrome by the usual channels. Done. https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:292: if (!remote_window_ && !base::win::IsMetroProcess()) { On 2012/11/16 01:51:26, robertshield wrote: > This whole code path only needs to be taken on Win8 right? Yes, although anything pre-Win8 is guaranteed to not decide to activate (or wait on an activation event) I still added the Win8+ conditional to make it clearer to the reader. https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:296: // to be signaled by Metro Chrome which will do so as soon as it grabbed On 2012/11/16 01:51:26, robertshield wrote: > nit: grabbed -> grabs Done. https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:320: BOOL released = ReleaseMutex(only_me); On 2012/11/16 03:04:41, grt wrote: > something akin to base::AutoUnlock should be used here. Done. https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:321: DCHECK(released) << "GetLastError = " << GetLastError(); On 2012/11/16 03:04:41, grt wrote: > DPCHECK(released); Done. https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:334: // the singleton and launch in Desktop mode). On 2012/11/16 03:04:41, grt wrote: > remove close paren Done. https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:378: // succeeded (i.e. this is the Metro process) or failed. On 2012/11/16 03:04:41, grt wrote: > comma after "i.e." Done. https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:379: base::win::ScopedHandle metro_activation_event( On 2012/11/16 03:04:41, grt wrote: > move this up between lines 374 and 375 so that other procs are only notified if > the window was created. I prefer to keep it here as if this Chrome failed to create the window, the other Chromes should not keep waiting; instead they should be released (and likely also fail to grab the profile lock). >also, only do this on win8+. Done. https://codereview.chromium.org/11099053/diff/21001/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:385: DCHECK(released) << "GetLastError = " << GetLastError(); On 2012/11/16 03:04:41, grt wrote: > DPCHECK(released); Done.
https://codereview.chromium.org/11099053/diff/29004/chrome/browser/process_si... File chrome/browser/process_singleton.h (right): https://codereview.chromium.org/11099053/diff/29004/chrome/browser/process_si... chrome/browser/process_singleton.h:73: // specfic was the linux implementation slightly tweaked. nit: "specific was the linux implementation slightly tweaked." -> "specific if the linux implementation was slightly tweaked." Not sure if adding this TODO is useful if Brett hasn't signed up to do this. https://codereview.chromium.org/11099053/diff/29004/chrome/browser/process_si... chrome/browser/process_singleton.h:135: // continue with this process. nit: "this process" -> "the current process" https://codereview.chromium.org/11099053/diff/29004/chrome/browser/process_si... chrome/browser/process_singleton.h:136: // On Windows, Create() has to be called before this. Is this enforced?
See replies below. https://codereview.chromium.org/11099053/diff/29004/chrome/browser/process_si... File chrome/browser/process_singleton.h (right): https://codereview.chromium.org/11099053/diff/29004/chrome/browser/process_si... chrome/browser/process_singleton.h:73: // specfic was the linux implementation slightly tweaked. On 2012/12/03 19:41:00, robertshield wrote: > nit: "specific was the linux implementation slightly tweaked." -> "specific if > the linux implementation was slightly tweaked." > > Not sure if adding this TODO is useful if Brett hasn't signed up to do this. This is essentially moving and rewording his previous TODO removed from line 140 below. https://codereview.chromium.org/11099053/diff/29004/chrome/browser/process_si... chrome/browser/process_singleton.h:135: // continue with this process. On 2012/12/03 19:41:00, robertshield wrote: > nit: "this process" -> "the current process" Done. https://codereview.chromium.org/11099053/diff/29004/chrome/browser/process_si... chrome/browser/process_singleton.h:136: // On Windows, Create() has to be called before this. On 2012/12/03 19:41:00, robertshield wrote: > Is this enforced? Yes, since NotifyOtherProcess() is now hidden from the public interface and is only called from NotifyOtherProcessOrCreate() after Create() has been called.
lgtm https://codereview.chromium.org/11099053/diff/29004/chrome/browser/process_si... File chrome/browser/process_singleton.h (right): https://codereview.chromium.org/11099053/diff/29004/chrome/browser/process_si... chrome/browser/process_singleton.h:73: // specfic was the linux implementation slightly tweaked. On 2012/12/03 19:46:22, gab wrote: > On 2012/12/03 19:41:00, robertshield wrote: > > nit: "specific was the linux implementation slightly tweaked." -> "specific if > > the linux implementation was slightly tweaked." > > > > Not sure if adding this TODO is useful if Brett hasn't signed up to do this. > > This is essentially moving and rewording his previous TODO removed from line 140 > below. Ok. Please correct the sentence structure.
https://codereview.chromium.org/11099053/diff/27002/chrome/browser/process_si... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/11099053/diff/27002/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:52: // scope. As documented, it's clearer to NOT request ownership on creation The "As documented..." portion of this comment fits more naturally with the construction of the mutex rather than with this helper class. Please move it back to where it was. https://codereview.chromium.org/11099053/diff/27002/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:57: explicit AutoLockMutex(const HANDLE& mutex) : mutex_(mutex) { pass the HANDLE by value https://codereview.chromium.org/11099053/diff/27002/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:76: explicit AutoUnlockMutex(const HANDLE& mutex) : mutex_(mutex) { pass the HANDLE by value https://codereview.chromium.org/11099053/diff/27002/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:358: // activated). Ignore timeout waiting for |metro_activation_event|. By i think the last sentence ("By using...") is superfluous. https://codereview.chromium.org/11099053/diff/32004/chrome/browser/process_si... File chrome/browser/process_singleton.h (right): https://codereview.chromium.org/11099053/diff/32004/chrome/browser/process_si... chrome/browser/process_singleton.h:32: class FilePath; remove this forward decl in favor of simply including the header. each of the platforms now has FilePath member in ProcessSingleton, so each of them need the full definition. https://codereview.chromium.org/11099053/diff/32004/chrome/browser/process_si... chrome/browser/process_singleton.h:61: NotificationCallback; the previous indentation was correct, please revert. https://codereview.chromium.org/11099053/diff/32004/chrome/browser/process_si... chrome/browser/process_singleton.h:73: // specfic was the linux implementation slightly tweaked. this TODO comment doesn't make sense.
Thanks, done. PTAL, Gab https://codereview.chromium.org/11099053/diff/27002/chrome/browser/process_si... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/11099053/diff/27002/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:52: // scope. As documented, it's clearer to NOT request ownership on creation On 2012/12/03 21:02:03, grt wrote: > The "As documented..." portion of this comment fits more naturally with the > construction of the mutex rather than with this helper class. Please move it > back to where it was. Done. https://codereview.chromium.org/11099053/diff/27002/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:57: explicit AutoLockMutex(const HANDLE& mutex) : mutex_(mutex) { On 2012/12/03 21:02:03, grt wrote: > pass the HANDLE by value Done. https://codereview.chromium.org/11099053/diff/27002/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:76: explicit AutoUnlockMutex(const HANDLE& mutex) : mutex_(mutex) { On 2012/12/03 21:02:03, grt wrote: > pass the HANDLE by value Done. https://codereview.chromium.org/11099053/diff/27002/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:358: // activated). Ignore timeout waiting for |metro_activation_event|. By On 2012/12/03 21:02:03, grt wrote: > i think the last sentence ("By using...") is superfluous. Removed last sentence. https://codereview.chromium.org/11099053/diff/32004/chrome/browser/process_si... File chrome/browser/process_singleton.h (right): https://codereview.chromium.org/11099053/diff/32004/chrome/browser/process_si... chrome/browser/process_singleton.h:32: class FilePath; On 2012/12/03 21:02:03, grt wrote: > remove this forward decl in favor of simply including the header. each of the > platforms now has FilePath member in ProcessSingleton, so each of them need the > full definition. Nice catch :). https://codereview.chromium.org/11099053/diff/32004/chrome/browser/process_si... chrome/browser/process_singleton.h:61: NotificationCallback; On 2012/12/03 21:02:03, grt wrote: > the previous indentation was correct, please revert. Done. https://codereview.chromium.org/11099053/diff/32004/chrome/browser/process_si... chrome/browser/process_singleton.h:73: // specfic was the linux implementation slightly tweaked. On 2012/12/03 21:02:03, grt wrote: > this TODO comment doesn't make sense. Reworded, hopefully this makes more sense. Looking at process_singleton_linux.cc, I think this is fairly simple; I'm just not familiar with the Linux side of things enough to do it. Mac is trivial since NotifyOtherProcess() is a no-op already.
lgtm
LGTM % nit https://codereview.chromium.org/11099053/diff/28011/chrome/browser/process_si... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/11099053/diff/28011/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:457: base::win::ScopedHandle only_me(CreateMutex(NULL, FALSE, kMutexName)); I think the mutex name needs to be formed off the user data dir to avoid this code path from needlessly blocking a chrome browser process which starts up from a different user data dir.
Thanks, +brettw for OWNER approval. FYI, I removed one of your old TODO that seemed related to Linux only and added one instead in which I suggest we should make NotifyOtherProcessOrCreate() non-platform-specific and that the only thing blocking this is to have the Linux implementation use the same logic as the Windows one. Cheers! Gab https://codereview.chromium.org/11099053/diff/28011/chrome/browser/process_si... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/11099053/diff/28011/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:457: base::win::ScopedHandle only_me(CreateMutex(NULL, FALSE, kMutexName)); On 2012/12/04 19:11:54, ananta wrote: > I think the mutex name needs to be formed off the user data dir to avoid this > code path from needlessly blocking a chrome browser process which starts up from > a different user data dir. As discussed offline, 1) This has always been the case. 2) This is not a big issue since this mutex is only held for a very short amount of time. 3) The user_data_dir is essentially user input which is potentially too long and potentially contains invalid characters for a mutex name. All in all, maybe we want to do this, but I don't think this should go in this change.
https://codereview.chromium.org/11099053/diff/28011/chrome/browser/process_si... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/11099053/diff/28011/chrome/browser/process_si... chrome/browser/process_singleton_win.cc:457: base::win::ScopedHandle only_me(CreateMutex(NULL, FALSE, kMutexName)); On 2012/12/04 19:44:18, gab wrote: > On 2012/12/04 19:11:54, ananta wrote: > > I think the mutex name needs to be formed off the user data dir to avoid this > > code path from needlessly blocking a chrome browser process which starts up > from > > a different user data dir. > > As discussed offline, > > 1) This has always been the case. > 2) This is not a big issue since this mutex is only held for a very short amount > of time. > 3) The user_data_dir is essentially user input which is potentially too long and > potentially contains invalid characters for a mutex name. > > All in all, maybe we want to do this, but I don't think this should go in this > change. We switched to using a global mutex about two years ago in http://crrev.com/69944. I don't think moving away from this is a good idea until there's data showing that contention on it is a problem.
owners lgtm rubberstamp
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gab@chromium.org/11099053/28011
Message was sent while issue was closed.
Change committed as 171105 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
