Thanks for elaborating. So if we look at it another way, there are several different problems that ProcessSingleton is currently used to solve:
- Prevent multiple instances of Chromium from referring to the same profile/user data directory. In the general sense, what we need here is a way to acquire and test locks on data directories. This is a cross-platform requirement: all platforms should be able to launch instances of Chromium in different user sessions with different user data / profiles (in order to support fast user switching on Mac & Windows, single Linux boxes with multiple people logged in, etc. Currently, ProcessSingleton really only solves this by accident. It seems cleaner to me to meet this requirement with an explicit mechanism: Something like a "ProfileLock" or "DirectoryLock," for example. This also gives a way to ease this requirement if someone fixes the concurrent-access problem, without affecting other uses of ProcessSingleton. - Testing, since a number of tests are not multiple-instance safe. I would describe this as a defect in those tests or the test framework, which again ProcessSingleton only mitigates by accident. I would prefer we address this by fixing the tests and/or test scripts so that we can indeed run tests in parallel without worrying about them stepping on each other. We have a strong desire to reduce our build cycle time, of which having to run tests serially instead of in parallel is a large component. - Within a user session, ensuring that requests to open URLs get processed by a currently running instance of Chromium instead of launching a new one. This is where platform differences are strongest, and is ProcessSingleton's main function. Registering properties on windows or the like for Windows and X11 seems cleaner to me than putting breadcrumbs into the file system, since window properties are inherently tied to the user session and to an active instance of the application (and thus do not need to be cleaned up on a crash, etc.). On the Mac, this function is provided by the OS, so there's no need for Chromium to duplicate it. --Amanda On Tue, May 5, 2009 at 12:27 PM, Никита Офицеров <[email protected]> wrote: > > I'm sorry for the late response, I've been quite busy last week. > > 2009/4/27 Amanda Walker <[email protected]>: >> "Hacky" is fairly subjective: are there particular things about the >> existing implementation that bother you? Currently they are different >> for each OS because each OS has its own issues surrounding launching >> multiple instances of the browser, different mechanisms for handling >> incoming requests from the OS to open web pages, and so on. >> > > 2009/4/28 stoyan <[email protected]>: >> Well, on Windows code looks racy, does not work well across multiple >> desktops, and probably will create troubles accessing userdata when >> same instance is running in different terminal sessions (with same >> user account). > > The problem with quick, baroque source code is that usually there are > lots of corner cases. > I have rethought the idea after your comments, and there are several > problems with current > Windows implementation: > > 1. As Stoyan mentioned, window messaging does not work across multiple > desktops. > Therefore it doesn't work across multiple terminal sessions. > > Consider the following use case: A user has a version of Windows that > allows multiple > consequent terminal sessions on his PC, and leaves Chrome opened. Then > he opens a > session from his laptop and launches Chrome. Preferred result is > opening a new window > in the second session, but AFAIK it's impossible for the Chrome master > process to open a > window in a different window station. Hence quite a lot of changes are > required for this to > function, like delegating handling of a widget to the newly created > process, and these fall > out of the scope of this idea. But current ProcessSingleton will just > fail to detect another > Chrome instance and crash or show weird behavior. If it worked, we > could at least show some > message telling user that running Chrome from different terminal > sessions is not yet implemented. > So IMHO ProcessSingleton should not rely on window messages, that are > by definition > bound to one window station. > > 2. Another problem with Windows implementation (maybe not really a > problem) is that it > guards Chrome against being launched with the same (luckily, > case-insensitive) user data > dir path. This is a fundamental problem, because ProcessSingleton > should protect not only > against being launched with the same path, but against actually using > the same dir twice > (one directory can be addressed with different paths, because of links > and mounting, even on > Windows NTFS partitions). > Well, I think that without user intervention a situation that exploits > this cannot be created, so > it may be considered as ok, but it's still a problem that IMHO should be > fixed. > > Because of that, I suggest two ideas: > 1. Do almost as I suggested before, but instead of Chrome PID use a > GUID, and write it only once, > on the first run. This GUID will let us distinguish data dirs without > need to rely on their path. So at > normal launch it will be one additional disk read (just several bytes) > and some IPC. If you think that > we can't afford even one extra disk read, then think about my second idea: > > 2. If problem #2 is not really a problem, then instead of GUID or PID > we can just use some hash from > user data dir path as channel_id, and make no disk operations at all. > This is by no means worse than > current implementation and solves the first problem. However, this is > only a partial solution. > > As an additional benefit, ProcessSingleton becomes what it was meant > to be - a facility to ensure that > only one instance with given data dir is running. Opening new URLs and > getting browser PID will be > just IPC messages, without custom protocol parsing or some extra > quirks in ProcessSingleton code. > > 2009/4/27 Amanda Walker <[email protected]>: >> Simply eliminating differences between per-platform implementations is >> not necessarily a large benefit by itself, since the amount of code is >> small and concerns an area where the three platforms behave quite >> differently. > I do agree with this, but it's not only for eliminating differences > between implementations, > but also to improve them. There is already quite a lot of > platform-specific code in IPC, > and it can handle differences quite well. The only remaining thing is > disabling this functionality > completely on Mac when not running tests, and this can be handled by one 'if'. > > As for "racyness", this can also be improved. Current implementation > was racy because singleton > was created with some delay after checking that no Chrome process > existed. That was a bug, and > that was fixed by adding singleton creation in constructor. It isn't > actually atomic, but should work > well. Creating named pipe is atomic operation now > (FILE_FLAG_FIRST_PIPE_INSTANCE), > however, creating UNIX domain socket is not. So this may also be > improved in order to avoid > race conditions. > > Nikita Ofitserov > > > > --~--~---------~--~----~------------~-------~--~----~ Chromium Developers mailing list: [email protected] View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
