Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
218883008:
Use process_singleton_linux on Mac. (Closed)
|
Created:
6 years, 8 months ago by jackhou1 Modified:
6 years, 7 months ago CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUse process_singleton_linux on Mac.
The current process singleton implementation on Linux does what we want for
Mac. I.e. it creates a unix domain socket in a temporary directory and a symlink
to it in the user data dir. The motivation for this in Linux is to support having
the user data dir on a networked filesystem. On Mac, it circumvents the path
length limit of unix domain sockets (104 characters). A similar solution is used
to allow app shims to connect to Chrome.
BUG=148465, 353047, 367612
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267512
Patch Set 1 #Patch Set 2 : Fix compile on android #
Total comments: 13
Patch Set 3 : Sync and rebase #Patch Set 4 : Address comments (mostly) #
Total comments: 13
Patch Set 5 : Sync and rebase #Patch Set 6 : Address comments #Patch Set 7 : CreateUniqueTempDir is fine. DCHECK socket path length. #
Total comments: 17
Patch Set 8 : Address comments #
Total comments: 1
Patch Set 9 : Sync and rebase #Patch Set 10 : Handle old SingletonLock #
Total comments: 18
Patch Set 11 : Sync and rebase #Patch Set 12 : Address comments #
Total comments: 9
Patch Set 13 : Address comments #
Total comments: 4
Patch Set 14 : Address comments #Patch Set 15 : Sync and rebase #
Messages
Total messages: 45 (0 generated)
After addressing comments, I'd maybe loop in mark@chromium (and/or shess@) -- this is a pretty fundamental change to how Chrome works on Mac. https://codereview.chromium.org/218883008/diff/40001/base/process/process_han... File base/process/process_handle_mac.cc (right): https://codereview.chromium.org/218883008/diff/40001/base/process/process_han... base/process/process_handle_mac.cc:7: #include <errno.h> nit: I don't think you need all these - maybe just libproc.h? https://codereview.chromium.org/218883008/diff/40001/base/process/process_han... base/process/process_handle_mac.cc:35: if (proc_pidpath(process, pathbuf, sizeof(pathbuf))) http://www.opensource.apple.com/source/Libc/Libc-583/darwin/libproc.c seems to return 0 on error, so this condition doesn't look right.. I can't really find good documentation for this function though o_O Is there a test that covers this path? https://codereview.chromium.org/218883008/diff/40001/chrome/app/chromium_stri... File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/218883008/diff/40001/chrome/app/chromium_stri... chrome/app/chromium_strings.grd:1118: <if expr="is_posix"> This might put an unnecessary string in the android resource bundle (and I think there's a script someone runs checking this..) - maybe is_linux or is_macosx? https://codereview.chromium.org/218883008/diff/40001/chrome/browser/process_s... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/218883008/diff/40001/chrome/browser/process_s... chrome/browser/process_singleton_posix.cc:315: return ShowProcessSingletonDialog(error, relaunch_button_text); Does this need an implementation on mac as well? I guess a log message is not so different to what users get now.. but does the linux singleton have the same assurance/recovery for a dangling profile lock? That is, currently there's a good guarantee on mac that Force-Quitting Chrome will make the 'Unable to obtain profile lock' error go away. Is there any flow where that could leave a dangling lock, requiring user intervention? (like, user connects to a different network and their hostname changes, causing the logic to think an old process is on a different computer and not auto-kill the currently running (or dead) process?) Also, what's an example string you get on Mac? https://codereview.chromium.org/218883008/diff/40001/chrome/browser/process_s... chrome/browser/process_singleton_posix.cc:325: base::FilePath(chrome::kBrowserProcessExecutableName)); This might not work on mac? -- chrome::kBrowserProcessExecutableName is the official release name. Mac uses different names for Chromium/Official (whereas on linux they are both 'chrome.exe'). Do you need to check for kBrowserProcessExecutableNameChromium in some cases? https://codereview.chromium.org/218883008/diff/40001/chrome/browser/process_s... File chrome/browser/process_singleton_posix_unittest.cc (right): https://codereview.chromium.org/218883008/diff/40001/chrome/browser/process_s... chrome/browser/process_singleton_posix_unittest.cc:34: class ProcessSingletonLinuxTest : public testing::Test { nit: this should probably be renamed Linux -> Posix
https://codereview.chromium.org/218883008/diff/40001/base/process/process_han... File base/process/process_handle_mac.cc (right): https://codereview.chromium.org/218883008/diff/40001/base/process/process_han... base/process/process_handle_mac.cc:7: #include <errno.h> On 2014/04/08 17:56:44, tapted wrote: > nit: I don't think you need all these - maybe just libproc.h? Done. https://codereview.chromium.org/218883008/diff/40001/base/process/process_han... base/process/process_handle_mac.cc:35: if (proc_pidpath(process, pathbuf, sizeof(pathbuf))) On 2014/04/08 17:56:44, tapted wrote: > http://www.opensource.apple.com/source/Libc/Libc-583/darwin/libproc.c seems to > return 0 on error, so this condition doesn't look right.. I can't really find > good documentation for this function though o_O > > Is there a test that covers this path? You're right. I've inverted the logic and manually confirmed that this method returns the correct path. This path is important during the race condition when, while chrome process 0 is starting up, it writes the SingletonLock symlink, but has not yet connected to SingletonSocket. Chrome process 1 starts up, fails to connect to the socket, and reads the symlink for process 0's pid. If that pid's path is not a chrome binary, process 1 will assume the singleton lock is orphaned and remove it. There's no test for this case at the moment. https://codereview.chromium.org/218883008/diff/40001/chrome/app/chromium_stri... File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/218883008/diff/40001/chrome/app/chromium_stri... chrome/app/chromium_strings.grd:1118: <if expr="is_posix"> On 2014/04/08 17:56:44, tapted wrote: > This might put an unnecessary string in the android resource bundle (and I think > there's a script someone runs checking this..) - maybe is_linux or is_macosx? Done. https://codereview.chromium.org/218883008/diff/40001/chrome/browser/process_s... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/218883008/diff/40001/chrome/browser/process_s... chrome/browser/process_singleton_posix.cc:315: return ShowProcessSingletonDialog(error, relaunch_button_text); On 2014/04/08 17:56:44, tapted wrote: > Does this need an implementation on mac as well? I guess a log message is not so > different to what users get now.. but does the linux singleton have the same > assurance/recovery for a dangling profile lock? It's actually less common than what users get now. At the moment, a log message occurs whenever you start another chrome process with the same UDD. After this change, that case will just work. The message here only occurs if the existing process is not running on the same host. > That is, currently there's a good guarantee on mac that Force-Quitting Chrome > will make the 'Unable to obtain profile lock' error go away. Is there any flow > where that could leave a dangling lock, requiring user intervention? (like, user > connects to a different network and their hostname changes, causing the logic to > think an old process is on a different computer and not auto-kill the currently > running (or dead) process?) Interesting, I think this case could happen on linux too, except it would let the user decide to usurp the lock. I wonder if we could use something more constant than hostname instead? > Also, what's an example string you get on Mac? From the unit test: The profile appears to be in use by another Chromium process (1234) on another computer (FAKEFOOHOST). Chromium has locked the profile so that it doesn't get corrupted. If you are sure no other processes are using this profile, you can unlock the profile and relaunch Chromium. https://codereview.chromium.org/218883008/diff/40001/chrome/browser/process_s... chrome/browser/process_singleton_posix.cc:325: base::FilePath(chrome::kBrowserProcessExecutableName)); On 2014/04/08 17:56:44, tapted wrote: > This might not work on mac? -- chrome::kBrowserProcessExecutableName is the > official release name. Mac uses different names for Chromium/Official (whereas > on linux they are both 'chrome.exe'). Do you need to check for > kBrowserProcessExecutableNameChromium in some cases? chrome::kBrowserProcessExecutableName seems to be "Chromium" on Mac. I think kBrowserProcessExecutableNameChromium is used when you want to refer to "Chromium" from an official build. https://codereview.chromium.org/218883008/diff/40001/chrome/browser/process_s... File chrome/browser/process_singleton_posix_unittest.cc (right): https://codereview.chromium.org/218883008/diff/40001/chrome/browser/process_s... chrome/browser/process_singleton_posix_unittest.cc:34: class ProcessSingletonLinuxTest : public testing::Test { On 2014/04/08 17:56:44, tapted wrote: > nit: this should probably be renamed Linux -> Posix Done.
mark, shess, would you mind taking a look at this CL too? Is this a reasonable thing to do? Are there any other concerns? One issue tapted raised is what happens if the hostname changes, and then the existing chrome process crashes. A new chrome process would think the lock is taken by a process on another host and be unable to recover. The implementation on Linux shows a dialog for the user to decide. I'm not sure how process_singleton_mac handled locking from another host, if at all.
On 2014/04/14 04:29:14, jackhou1 wrote: > mark, shess, would you mind taking a look at this CL too? > > Is this a reasonable thing to do? Are there any other concerns? Unfortunate that it didn't match the _posix implementation as a diff. I'll circle back and poke at that later in the morning. From what I recall, I think this was where I stopped poking at the issue: https://code.google.com/p/chromium/issues/detail?id=148465#c7 I'm pretty sure when I tried a tmpdir version on the Mac bots, there was test breakage, so you probably should spin off a try run while you're waiting on me. > One issue tapted raised is what happens if the hostname changes, and then the > existing chrome process crashes. A new chrome process would think the lock is > taken by a process on another host and be unable to recover. The implementation > on Linux shows a dialog for the user to decide. I'm not sure how > process_singleton_mac handled locking from another host, if at all. I don't have a good canned response there. I think the resolution was that we just don't care about profiles on network mounts on Mac, as it's a very low-likelihood use case. Anyone currently doing this is probably broken badly, your change won't regress that.
AFAICT, the _posix stuff is exactly what I had tried, so mainly make sure you check the buildbots pretty aggressively. The problem there is that OSX generates very deep tmpdirs, like: /var/folders/00/015t8000h01000cxqpysvccm0004q9/T/ at least as compared to most systems. That uses up half the 100 (or 104?) bytes available for the unix-domain socket, so if tests additionally nest things a bit in generating profiles, it's easy to hit the limits. https://codereview.chromium.org/218883008/diff/80001/base/process/process_han... File base/process/process_handle.h (right): https://codereview.chromium.org/218883008/diff/80001/base/process/process_han... base/process/process_handle.h:84: #if defined(OS_LINUX) || defined(OS_ANDROID) || defined(OS_BSD) || defined(OS_MACOSX) This seems to be present in exactly the same places as GetParentProcessId below. https://codereview.chromium.org/218883008/diff/80001/base/process/process_han... File base/process/process_handle_mac.cc (right): https://codereview.chromium.org/218883008/diff/80001/base/process/process_han... base/process/process_handle_mac.cc:25: return info.kp_eproc.e_ppid; Weird, I wonder why this isn't just getppid()? The Linux version is also a bit more convoluted than seems reasonable. https://codereview.chromium.org/218883008/diff/80001/base/process/process_han... base/process/process_handle_mac.cc:28: excess newline. https://codereview.chromium.org/218883008/diff/80001/base/process/process_han... base/process/process_handle_mac.cc:30: char pathbuf[PROC_PIDPATHINFO_MAXSIZE]; This seems weird - this constant is defined as 4*MAXPATHLEN, and proc_pidpath defines the buffer as void*. Is there any chance that this is actually giving your 32-bit chars? Looking at libproc.c, it's using strlen() on the buffer, so I guess not. https://codereview.chromium.org/218883008/diff/80001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/218883008/diff/80001/chrome/chrome_browser.gy... chrome/chrome_browser.gypi:1718: 'browser/process_singleton_posix.cc', Order below "modal". https://codereview.chromium.org/218883008/diff/80001/chrome/chrome_tests_unit... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/218883008/diff/80001/chrome/chrome_tests_unit... chrome/chrome_tests_unit.gypi:2470: 'browser/process_singleton_posix_unittest.cc', c before f, except after ... no, always c before f!
> > One issue tapted raised is what happens if the hostname changes, and then the > > existing chrome process crashes. A new chrome process would think the lock is > > taken by a process on another host and be unable to recover. The > implementation > > on Linux shows a dialog for the user to decide. I'm not sure how > > process_singleton_mac handled locking from another host, if at all. > > I don't have a good canned response there. I think the resolution was that we > just don't care about profiles on network mounts on Mac, as it's a very > low-likelihood use case. Anyone currently doing this is probably broken badly, > your change won't regress that. Sounds good. I've changed it to always usurp the lock in this case on Mac. > AFAICT, the _posix stuff is exactly what I had tried, so mainly make sure you > check the buildbots pretty aggressively. > > The problem there is that OSX generates very deep tmpdirs, like: > /var/folders/00/015t8000h01000cxqpysvccm0004q9/T/ > at least as compared to most systems. That uses up half the 100 (or 104?) bytes > available for the unix-domain socket, so if tests additionally nest things a bit > in generating profiles, it's easy to hit the limits. Ah yeah, this was the exact problem we needed to solve for app shims. I've changed the code here to make it the same as what we did for app shims. I.e. put the ScopedTempDir under /tmp and check that the directory is only user-writable. https://codereview.chromium.org/218883008/diff/80001/base/process/process_han... File base/process/process_handle.h (right): https://codereview.chromium.org/218883008/diff/80001/base/process/process_han... base/process/process_handle.h:84: #if defined(OS_LINUX) || defined(OS_ANDROID) || defined(OS_BSD) || defined(OS_MACOSX) On 2014/04/14 19:21:33, shess wrote: > This seems to be present in exactly the same places as GetParentProcessId below. Done. https://codereview.chromium.org/218883008/diff/80001/base/process/process_han... File base/process/process_handle_mac.cc (right): https://codereview.chromium.org/218883008/diff/80001/base/process/process_han... base/process/process_handle_mac.cc:25: return info.kp_eproc.e_ppid; On 2014/04/14 19:21:33, shess wrote: > Weird, I wonder why this isn't just getppid()? The Linux version is also a bit > more convoluted than seems reasonable. Yeah, I'm not sure. I'll rope one of the authors of process_singleton_linux to take a look at this CL later. https://codereview.chromium.org/218883008/diff/80001/base/process/process_han... base/process/process_handle_mac.cc:28: On 2014/04/14 19:21:33, shess wrote: > excess newline. Done. https://codereview.chromium.org/218883008/diff/80001/base/process/process_han... base/process/process_handle_mac.cc:30: char pathbuf[PROC_PIDPATHINFO_MAXSIZE]; On 2014/04/14 19:21:33, shess wrote: > This seems weird - this constant is defined as 4*MAXPATHLEN, and proc_pidpath > defines the buffer as void*. Is there any chance that this is actually giving > your 32-bit chars? > > Looking at libproc.c, it's using strlen() on the buffer, so I guess not. Yeah I think 4* MAXPATHLEN just happens to be the upper bound. Looks like the buffer size needs to be at minimum MAXPATHLEN (PROC_PIDPATHINFO_SIZE). https://codereview.chromium.org/218883008/diff/80001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/218883008/diff/80001/chrome/chrome_browser.gy... chrome/chrome_browser.gypi:1718: 'browser/process_singleton_posix.cc', On 2014/04/14 19:21:33, shess wrote: > Order below "modal". Done. https://codereview.chromium.org/218883008/diff/80001/chrome/chrome_tests_unit... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/218883008/diff/80001/chrome/chrome_tests_unit... chrome/chrome_tests_unit.gypi:2470: 'browser/process_singleton_posix_unittest.cc', On 2014/04/14 19:21:33, shess wrote: > c before f, except after ... no, always c before f! Done.
mattm, I think you reviewed a lot of process_singleton_linux, could you take a look at this CL? (or redirect if there's someone more appropriate) mark, shess, PTAL BTW, I'm in Sydney, so please pile all your comments on and I'll address them at the same time.
> > AFAICT, the _posix stuff is exactly what I had tried, so mainly make sure you > > check the buildbots pretty aggressively. > > > > The problem there is that OSX generates very deep tmpdirs, like: > > /var/folders/00/015t8000h01000cxqpysvccm0004q9/T/ > > at least as compared to most systems. That uses up half the 100 (or 104?) > bytes > > available for the unix-domain socket, so if tests additionally nest things a > bit > > in generating profiles, it's easy to hit the limits. > > Ah yeah, this was the exact problem we needed to solve for app shims. I've > changed the code here to make it the same as what we did for app shims. I.e. put > the ScopedTempDir under /tmp and check that the directory is only user-writable. Oops, tapted just pointed out that we ended up using PathService::Get(base::DIR_TEMP), which does the same as CreateUniqueTempDir. I remember now, it returns a relatively constant path that's short enough.
https://codereview.chromium.org/218883008/diff/140001/base/process/process_ha... File base/process/process_handle.h (right): https://codereview.chromium.org/218883008/diff/140001/base/process/process_ha... base/process/process_handle.h:84: #if defined(OS_LINUX) || defined(OS_ANDROID) || defined(OS_BSD) || defined(OS_MACOSX) maybe just OS_POSIX? https://codereview.chromium.org/218883008/diff/140001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/218883008/diff/140001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:318: } should this closing paren be before the #if? https://codereview.chromium.org/218883008/diff/140001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:921: DCHECK_GT(104u, socket_target_path.value().length()); This might only make sense on Mac, where the string length of NSTemporaryDirectory() is somewhat predictable. Not sure if Linux makes many guarantees about $TMP.
https://codereview.chromium.org/218883008/diff/40001/chrome/browser/process_s... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/218883008/diff/40001/chrome/browser/process_s... chrome/browser/process_singleton_posix.cc:315: return ShowProcessSingletonDialog(error, relaunch_button_text); On 2014/04/14 04:12:10, jackhou1 wrote: > On 2014/04/08 17:56:44, tapted wrote: > > Does this need an implementation on mac as well? I guess a log message is not > so > > different to what users get now.. but does the linux singleton have the same > > assurance/recovery for a dangling profile lock? > > It's actually less common than what users get now. At the moment, a log message > occurs whenever you start another chrome process with the same UDD. After this > change, that case will just work. The message here only occurs if the existing > process is not running on the same host. > > > That is, currently there's a good guarantee on mac that Force-Quitting Chrome > > will make the 'Unable to obtain profile lock' error go away. Is there any flow > > where that could leave a dangling lock, requiring user intervention? (like, > user > > connects to a different network and their hostname changes, causing the logic > to > > think an old process is on a different computer and not auto-kill the > currently > > running (or dead) process?) > > Interesting, I think this case could happen on linux too, except it would let > the user decide to usurp the lock. I wonder if we could use something more > constant than hostname instead? I'm not sure about osx, but linux doesn't normally change the hostname on a network change. > > Also, what's an example string you get on Mac? > > From the unit test: > The profile appears to be in use by another Chromium process (1234) on another > computer (FAKEFOOHOST). Chromium has locked the profile so that it doesn't get > corrupted. If you are sure no other processes are using this profile, you can > unlock the profile and relaunch Chromium. https://codereview.chromium.org/218883008/diff/80001/base/process/process_han... File base/process/process_handle_mac.cc (right): https://codereview.chromium.org/218883008/diff/80001/base/process/process_han... base/process/process_handle_mac.cc:25: return info.kp_eproc.e_ppid; On 2014/04/17 07:55:54, jackhou1 wrote: > On 2014/04/14 19:21:33, shess wrote: > > Weird, I wonder why this isn't just getppid()? The Linux version is also a > bit > > more convoluted than seems reasonable. > > Yeah, I'm not sure. I'll rope one of the authors of process_singleton_linux to > take a look at this CL later. Heh, I had to stare at it a while too. It's because this allows you to get the parent pid of an arbitrary process, not just the current one. It's obvious once you look at how it's used: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro... https://codereview.chromium.org/218883008/diff/140001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/218883008/diff/140001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:912: dir_mode != base::FILE_PERMISSION_USER_MASK) { Should log an error here? https://codereview.chromium.org/218883008/diff/140001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:921: DCHECK_GT(104u, socket_target_path.value().length()); SetupSocket already has a CHECK() of the path length, does having a similar dcheck here add anything? Also shouldn't use a magic number.
When I patch this in and run it in a Chromium which had already run, it fails because the SingletonLock is an existing file which is not a symlink. WRT using NSTemporaryDirectory() for the Unix-domain socket, I think it's fine if it works on the bots and in real life. Just make sure you are testing with something that really breaks on the bots if it's not working, rather than gracefully dealing with the problem. After deleting the existing SingletonLock, here's my path for Chromium: /var/folders/00/015t8000h01000cxqpysvccm0004q9/T/.org.chromium.Chromium.Wbp8Wx/SingletonSocket That's 94 characters, which is pretty long in the tooth. I'm not sure what an official build of Chrome would have, maybe ".com.google.Google Chrome"? Canary maybe ".com.google.Google Chrome Canary"? The latter is 104 on the dot. [I did not make an official build just to test that out, I'm just thinking out loud.] https://codereview.chromium.org/218883008/diff/140001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/218883008/diff/140001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:312: return ShowProcessSingletonDialog(error, relaunch_button_text); Indentation is wrong in here. I think having the #if/#endif outside the if() block entirely would be cleaner. Actually - it would be even cleaner to have: if (g_disable_prompt) return false; Then put the other cases below that, ending with: NOTREACHED() return false; https://codereview.chromium.org/218883008/diff/140001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:314: #if defined(OS_MACOSX) I think #elif defined(OS_MACOSX) would make sense, here, to clarify that these are mutually-exclusive. https://codereview.chromium.org/218883008/diff/140001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:912: dir_mode != base::FILE_PERMISSION_USER_MASK) { On 2014/04/17 21:44:55, mattm wrote: > Should log an error here? As long as it doesn't cruft up buildbot or other logs. I'm not really sure what good any logging in here will do, as the users who's systems are messed up won't see the logs or won't understand what to do about it. That said, is this a security check, or a sanity check? I can't really think of how the create can succeed but this can fail, which implies to me that you're checking for a race condition. In that case, it should be expected to never happen, so I think you should CHECK() or something, rather than masking it. https://codereview.chromium.org/218883008/diff/140001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:921: DCHECK_GT(104u, socket_target_path.value().length()); On 2014/04/17 08:36:14, tapted wrote: > This might only make sense on Mac, where the string length of > NSTemporaryDirectory() is somewhat predictable. Not sure if Linux makes many > guarantees about $TMP. I would think that would be why you'd want the check, because if it's too long it will break the Unix domain socket. I think it would be reasonable to just have a more practical check, that the socket creation doesn't return ETOOLONG or whatever error it returns in that case. https://codereview.chromium.org/218883008/diff/140001/chrome/chrome_tests_uni... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/218883008/diff/140001/chrome/chrome_tests_uni... chrome/chrome_tests_unit.gypi:2472: 'browser/process_singleton_posix_unittest.cc', process before profiles.
> When I patch this in and run it in a Chromium which had already run, it fails > because the SingletonLock is an existing file which is not a symlink. Added code and tests to handle this. > After deleting the existing SingletonLock, here's my path for Chromium: > /var/folders/00/015t8000h01000cxqpysvccm0004q9/T/.org.chromium.Chromium.Wbp8Wx/SingletonSocket > > That's 94 characters, which is pretty long in the tooth. I'm not sure what an > official build of Chrome would have, maybe ".com.google.Google Chrome"? Canary > maybe ".com.google.Google Chrome Canary"? The latter is 104 on the dot. [I did > not make an official build just to test that out, I'm just thinking out loud.] Good point. The path for canary is ".com.google.Chrome.canary", which is short enough. https://codereview.chromium.org/218883008/diff/140001/base/process/process_ha... File base/process/process_handle.h (right): https://codereview.chromium.org/218883008/diff/140001/base/process/process_ha... base/process/process_handle.h:84: #if defined(OS_LINUX) || defined(OS_ANDROID) || defined(OS_BSD) || defined(OS_MACOSX) On 2014/04/17 08:36:14, tapted wrote: > maybe just OS_POSIX? Done. https://codereview.chromium.org/218883008/diff/140001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/218883008/diff/140001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:312: return ShowProcessSingletonDialog(error, relaunch_button_text); On 2014/04/18 20:51:33, shess wrote: > Indentation is wrong in here. I think having the #if/#endif outside the if() > block entirely would be cleaner. > > Actually - it would be even cleaner to have: > > if (g_disable_prompt) > return false; > > Then put the other cases below that, ending with: > > NOTREACHED() > return false; Done. https://codereview.chromium.org/218883008/diff/140001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:314: #if defined(OS_MACOSX) On 2014/04/18 20:51:33, shess wrote: > I think #elif defined(OS_MACOSX) would make sense, here, to clarify that these > are mutually-exclusive. Done. https://codereview.chromium.org/218883008/diff/140001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:318: } On 2014/04/17 08:36:14, tapted wrote: > should this closing paren be before the #if? It matches with the "if (!g_disable_prompt) {", but that's changed now. https://codereview.chromium.org/218883008/diff/140001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:912: dir_mode != base::FILE_PERMISSION_USER_MASK) { On 2014/04/18 20:51:33, shess wrote: > On 2014/04/17 21:44:55, mattm wrote: > > Should log an error here? > > As long as it doesn't cruft up buildbot or other logs. I'm not really sure what > good any logging in here will do, as the users who's systems are messed up won't > see the logs or won't understand what to do about it. > > That said, is this a security check, or a sanity check? I can't really think of > how the create can succeed but this can fail, which implies to me that you're > checking for a race condition. In that case, it should be expected to never > happen, so I think you should CHECK() or something, rather than masking it. We don't want to create a socket in an unsafe directory. Changed this to a CHECK with message. https://codereview.chromium.org/218883008/diff/140001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:921: DCHECK_GT(104u, socket_target_path.value().length()); On 2014/04/17 21:44:55, mattm wrote: > SetupSocket already has a CHECK() of the path length, does having a similar > dcheck here add anything? > Also shouldn't use a magic number. You're right, the check in SetupSocketAddr does the same thing. https://codereview.chromium.org/218883008/diff/140001/chrome/chrome_tests_uni... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/218883008/diff/140001/chrome/chrome_tests_uni... chrome/chrome_tests_unit.gypi:2472: 'browser/process_singleton_posix_unittest.cc', On 2014/04/18 20:51:33, shess wrote: > process before profiles. Done.
Thanks for all the comments. PTAL
minor comments, I'll try to respond quickly for next review so you can get it landed. Thanks for working on this. https://codereview.chromium.org/218883008/diff/160001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/218883008/diff/160001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:916: << "Temp directory mode is not 700: " << std::oct << dir_mode; std::oct? Searches ... you just blew my mind! I'd have probably ended up with some StringPrintf() construct! https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:415: O_RDWR | O_CREAT | O_SYMLINK, 0644)); This indentation is wrong, the O_RDWR is w/in the open(). https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:427: } Close lock_fd, or use a ScopedFD on it. Actually, use a ScopedFD on it, someone should close it! https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:434: // version has run. What happens if this case occurs? Is the user's browser stuck, or does it just give up and optimistically run anyhow? https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:436: if (!temp_lock_dir.CreateUniqueTempDirUnderPath(lock_path.DirName())) { My guess is that this code will never crash. BUT, if some other thread crashes between here and the rename, then this potentially leaves random cruft in the profile. I think just choosing a fixed path (like lock_path with ".tmp" appended) would be nicer, because that will keep trying until it succeeds. I _think_ SymlinkPath() will work right if this scenario happens, given the double-check it does on failure. https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:947: // TODO(jackhou): Remove this case once this code is stable on Mac. Log a tracking bug for this, and reference it here. After one release cycle I think it could be something like unlinking on failure and trying again. But since it doesn't work at all if the existing file is there, I think it probably has to keep some sort of handler forever (I don't think someone keeping an old Chrome running for a long time and launching in parallel is likely, but it is likely that someone will try to run Chrome after a long period of no launches). I find myself wondering what happens if someone creates something that persistently fails. The SymlinkPath() code doesn't appear to check for specific codes known to happen when failing the race, so it's possible there are a bunch of other errors which actually should lead to unlink.
https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:447: rc = rename(temp_lock_path.value().c_str(), lock_path.value().c_str()); Is all of this even necessary? The rest of the code already assumes that creating the symlink can race, so why not just unlink the old lockfile and try again to create the new one the normal way? https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:951: return ReplaceOldSingletonLock(symlink_content, lock_path_); Unconditionally returning here doesn't seem right. If the old lock was successfully replaced, you still need to create the socket and stuff below. https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... File chrome/browser/process_singleton_posix_unittest.cc (right): https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... chrome/browser/process_singleton_posix_unittest.cc:406: EXPECT_FALSE(process_singleton->Create()); maybe also double check that after that the lockpath still exists and still is a normal file
https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:447: rc = rename(temp_lock_path.value().c_str(), lock_path.value().c_str()); On 2014/04/24 22:58:51, mattm wrote: > Is all of this even necessary? The rest of the code already assumes that > creating the symlink can race, so why not just unlink the old lockfile and try > again to create the new one the normal way? It's atomically locking against the previous browser version.
Right, I could have been more clear. I meant just the part about creating a temp dir and temp name for the symlink and then renaming on top of the existing lock. Once we did the flock and verified that there isn't an old version currently running, I don't think its necessary to try to do the rest of it atomically. (I assume that if an old version runs at any time after the new Singleton is in place something bad will happen, so just keeping it locked that tiny extra bit isn't useful.) On Apr 25, 2014 8:26 AM, <shess@chromium.org> wrote: > > https://codereview.chromium.org/218883008/diff/200001/ > chrome/browser/process_singleton_posix.cc > File chrome/browser/process_singleton_posix.cc (right): > > https://codereview.chromium.org/218883008/diff/200001/ > chrome/browser/process_singleton_posix.cc#newcode447 > chrome/browser/process_singleton_posix.cc:447: rc = > rename(temp_lock_path.value().c_str(), lock_path.value().c_str()); > On 2014/04/24 22:58:51, mattm wrote: > >> Is all of this even necessary? The rest of the code already assumes >> > that > >> creating the symlink can race, so why not just unlink the old lockfile >> > and try > >> again to create the new one the normal way? >> > > It's atomically locking against the previous browser version. > > https://codereview.chromium.org/218883008/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:415: O_RDWR | O_CREAT | O_SYMLINK, 0644)); On 2014/04/24 21:54:43, shess wrote: > This indentation is wrong, the O_RDWR is w/in the open(). Done. https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:427: } On 2014/04/24 21:54:43, shess wrote: > Close lock_fd, or use a ScopedFD on it. > > Actually, use a ScopedFD on it, someone should close it! Done. https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:434: // version has run. On 2014/04/24 21:54:43, shess wrote: > What happens if this case occurs? Is the user's browser stuck, or does it just > give up and optimistically run anyhow? The older version of Chrome will replace the symlink as if it didn't exist and continue running. https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:436: if (!temp_lock_dir.CreateUniqueTempDirUnderPath(lock_path.DirName())) { On 2014/04/24 21:54:43, shess wrote: > My guess is that this code will never crash. BUT, if some other thread crashes > between here and the rename, then this potentially leaves random cruft in the > profile. I think just choosing a fixed path (like lock_path with ".tmp" > appended) would be nicer, because that will keep trying until it succeeds. > > I _think_ SymlinkPath() will work right if this scenario happens, given the > double-check it does on failure. Putting it in the same directory ensures it's on the same volume, otherwise rename() doesn't work. However, mattm made a good point that replacing the lock instead of just deleting it doesn't buy much, so I'll just do that. https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:447: rc = rename(temp_lock_path.value().c_str(), lock_path.value().c_str()); On 2014/04/25 18:27:27, mattm wrote: > Once we did the flock and verified that there isn't an old version > currently running, I don't think its necessary to try to do the rest of it > atomically. (I assume that if an old version runs at any time after the new > Singleton is in place something bad will happen, so just keeping it locked > that tiny extra bit isn't useful.) Changed to just deleting the old SingletonLock. https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:947: // TODO(jackhou): Remove this case once this code is stable on Mac. On 2014/04/24 21:54:43, shess wrote: > Log a tracking bug for this, and reference it here. After one release cycle I > think it could be something like unlinking on failure and trying again. But > since it doesn't work at all if the existing file is there, I think it probably > has to keep some sort of handler forever (I don't think someone keeping an old > Chrome running for a long time and launching in parallel is likely, but it is > likely that someone will try to run Chrome after a long period of no launches). > > I find myself wondering what happens if someone creates something that > persistently fails. The SymlinkPath() code doesn't appear to check for specific > codes known to happen when failing the race, so it's possible there are a bunch > of other errors which actually should lead to unlink. Done. http://crbug.com/367612 Yeah, it's probably ok to delete any non-symlink lock file on linux. I'll look into making that the default when resolving that bug. https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:951: return ReplaceOldSingletonLock(symlink_content, lock_path_); On 2014/04/24 22:58:51, mattm wrote: > Unconditionally returning here doesn't seem right. If the old lock was > successfully replaced, you still need to create the socket and stuff below. Fixed. Also updated test. https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... File chrome/browser/process_singleton_posix_unittest.cc (right): https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... chrome/browser/process_singleton_posix_unittest.cc:406: EXPECT_FALSE(process_singleton->Create()); On 2014/04/24 22:58:51, mattm wrote: > maybe also double check that after that the lockpath still exists and still is a > normal file Done.
https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/218883008/diff/200001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:436: if (!temp_lock_dir.CreateUniqueTempDirUnderPath(lock_path.DirName())) { On 2014/04/28 05:20:16, jackhou1 wrote: > On 2014/04/24 21:54:43, shess wrote: > > My guess is that this code will never crash. BUT, if some other thread > crashes > > between here and the rename, then this potentially leaves random cruft in the > > profile. I think just choosing a fixed path (like lock_path with ".tmp" > > appended) would be nicer, because that will keep trying until it succeeds. > > > > I _think_ SymlinkPath() will work right if this scenario happens, given the > > double-check it does on failure. > > Putting it in the same directory ensures it's on the same volume, otherwise > rename() doesn't work. However, mattm made a good point that replacing the lock > instead of just deleting it doesn't buy much, so I'll just do that. Sounds good. https://codereview.chromium.org/218883008/diff/230001/chrome/browser/process_... File chrome/browser/process_singleton_posix_unittest.cc (right): https://codereview.chromium.org/218883008/diff/230001/chrome/browser/process_... chrome/browser/process_singleton_posix_unittest.cc:132: ssize_t len = readlink(lock_path_.value().c_str(), buf, PATH_MAX); readlink() will return -1 with EINVAL if it's not a link. Would it be worthwhile to make this function more concise? Otherwise, I think using base::IsLink() to check for link, and base::ReadSymbolicLink() for read would be better for those cases. https://codereview.chromium.org/218883008/diff/230001/chrome/browser/process_... chrome/browser/process_singleton_posix_unittest.cc:408: O_RDWR | O_CREAT | O_EXLOCK, 0644)); You should close this file. Which means ... a scoper! https://codereview.chromium.org/218883008/diff/230001/chrome/browser/process_... chrome/browser/process_singleton_posix_unittest.cc:424: EXPECT_TRUE(base::IsLink(lock_path_)); This IsLink() should be redundant WRT VerifyFiles().
https://codereview.chromium.org/218883008/diff/230001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/218883008/diff/230001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:942: } hm, this might be cleaner like: if (errno == EEXIST && !base::IsLink(lock_path_) && ReplaceOldSingletonLock(symlink_content, lock_path_)) { // Successfully replaced an old lock, fall through. } else { return false; } What do you think? (You could go even further to remove the else by inverting the conditional, but I think that makes it harder to follow.)
https://codereview.chromium.org/218883008/diff/230001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/218883008/diff/230001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:942: } On 2014/04/28 22:19:00, mattm wrote: > hm, this might be cleaner like: > > if (errno == EEXIST && !base::IsLink(lock_path_) && > ReplaceOldSingletonLock(symlink_content, lock_path_)) { > // Successfully replaced an old lock, fall through. > } else { > return false; > } > > What do you think? > > (You could go even further to remove the else by inverting the conditional, but > I think that makes it harder to follow.) Or you could run it together into a single if() with three parts. Or even: if (!base::IsLink(lock_path_) && !ROSL(s, l)) { return false; } I don't think the errno check really adds much, and it removes the question of whether any PLOG(ERROR) cases might change errno.
https://codereview.chromium.org/218883008/diff/230001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/218883008/diff/230001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:942: } On 2014/04/28 22:28:13, shess wrote: > On 2014/04/28 22:19:00, mattm wrote: > > hm, this might be cleaner like: > > > > if (errno == EEXIST && !base::IsLink(lock_path_) && > > ReplaceOldSingletonLock(symlink_content, lock_path_)) { > > // Successfully replaced an old lock, fall through. > > } else { > > return false; > > } > > > > What do you think? > > > > (You could go even further to remove the else by inverting the conditional, > but > > I think that makes it harder to follow.) > > Or you could run it together into a single if() with three parts. Or even: > > if (!base::IsLink(lock_path_) && !ROSL(s, l)) { > return false; > } > > I don't think the errno check really adds much, and it removes the question of > whether any PLOG(ERROR) cases might change errno. Done. Added more commentary. https://codereview.chromium.org/218883008/diff/230001/chrome/browser/process_... File chrome/browser/process_singleton_posix_unittest.cc (right): https://codereview.chromium.org/218883008/diff/230001/chrome/browser/process_... chrome/browser/process_singleton_posix_unittest.cc:132: ssize_t len = readlink(lock_path_.value().c_str(), buf, PATH_MAX); On 2014/04/28 17:41:39, shess wrote: > readlink() will return -1 with EINVAL if it's not a link. Would it be > worthwhile to make this function more concise? > > Otherwise, I think using base::IsLink() to check for link, and > base::ReadSymbolicLink() for read would be better for those cases. This was just moved out of TEST_F(ProcessSingletonLinuxTest, CheckSocketFile), so I'd prefer to leave it as-is. https://codereview.chromium.org/218883008/diff/230001/chrome/browser/process_... chrome/browser/process_singleton_posix_unittest.cc:408: O_RDWR | O_CREAT | O_EXLOCK, 0644)); On 2014/04/28 17:41:39, shess wrote: > You should close this file. Which means ... a scoper! Done. https://codereview.chromium.org/218883008/diff/230001/chrome/browser/process_... chrome/browser/process_singleton_posix_unittest.cc:424: EXPECT_TRUE(base::IsLink(lock_path_)); On 2014/04/28 17:41:39, shess wrote: > This IsLink() should be redundant WRT VerifyFiles(). Done.
lgtm
LGTM! Besides, I'm getting complaint fatigue. I need to work on my stamina, maybe.
Thanks everyone for your patience with a long CL!
thakis, please review for OWNERS rsesek, please review for SECURITY
LGTM https://codereview.chromium.org/218883008/diff/250001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/218883008/diff/250001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:307: LOG(ERROR) << base::SysWideToNativeMB(base::UTF16ToWide(error)).c_str(); string16 should be able to print to a std::ostream without conversion. https://codereview.chromium.org/218883008/diff/250001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:421: int rc = HANDLE_EINTR(flock(lock_fd.get(), LOCK_EX|LOCK_NB)); nit: You put spaces around | on line 415 but not here. Please be consistent.
lgtm
https://codereview.chromium.org/218883008/diff/250001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/218883008/diff/250001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:307: LOG(ERROR) << base::SysWideToNativeMB(base::UTF16ToWide(error)).c_str(); On 2014/04/30 15:01:43, rsesek wrote: > string16 should be able to print to a std::ostream without conversion. Done. https://codereview.chromium.org/218883008/diff/250001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:421: int rc = HANDLE_EINTR(flock(lock_fd.get(), LOCK_EX|LOCK_NB)); On 2014/04/30 15:01:43, rsesek wrote: > nit: You put spaces around | on line 415 but not here. Please be consistent. Done.
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/218883008/290001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/218883008/290001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/218883008/290001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/218883008/290001
Message was sent while issue was closed.
Change committed as 267512 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
