This CL implements the second TODO item of issue 12343:...

This CL implements the second TODO item of issue 12343:
2) We should send back an ACK to the second process.  If the second process doesn't get an ACK in the given timeout, it should kill the first process and go ahead and start.

The approach of this CL is to append process id to the singleton's socket filename, such as "SingletonSocket-12345", and creates a symbol link "SingletonSocket" to the real socket file. In ProcessSingleton::NotifyOtherProcess() if it's successfully connected to "SingletonSocket" but no ACK received, then the original process can be killed by its process id retrieved from the symbol link.

BUG=12343
ProcessSingleton Linux cleanups
TEST=In one terminal, launch chrome and stop the process by pressing ctrl-z, then launch chrome again in another terminal. The second chrome shall be started in 5 seconds, and the first one shall be killed.

[james.su_gmail.com, 2009-07-20 15:05:06]

Hi,
  It's my initial CL to fix the second TODO item of issue 12343. Please help
review it.

Thanks
James Su

[willchan no longer on Chromium, 2009-07-20 16:51:04]

Hi James,

Thanks for sending this out.  I haven't taken a look at the code yet.  First, I
wanted to take care of minor administrative issues.  Have you filled out the
appropriate license agreement on
http://dev.chromium.org/developers/contributing-code?  Also, you should add
yourself to src/AUTHORS.

I'll try to get around to reviewing the code itself later today.

[james.su_gmail.com, 2009-07-21 02:22:17]

Hi,
  I just made a little improvement to this CL to use relative symbol link.

Regards
James Su

[willchan no longer on Chromium, 2009-07-22 00:27:45]

Sorry for the delay.  I only have some nits.

http://codereview.chromium.org/155772/diff/1005/1007
File chrome/browser/process_singleton_linux.cc (right):

http://codereview.chromium.org/155772/diff/1005/1007#newcode9
Line 9: // exits.
Please update this comment to explain the use of symlinks.  If you could perhaps
copy/paste the ls output so the file system layout is obvious, that might be
clearest.

http://codereview.chromium.org/155772/diff/1005/1007#newcode20
Line 20: #include <set>
There should be an include for <string> for std::string.

http://codereview.chromium.org/155772/diff/1005/1007#newcode65
Line 65: DCHECK_EQ(0, rv) << "Error closing socket: " << strerror(errno);
I think we need <string.h> for strerror.

http://codereview.chromium.org/155772/diff/1005/1007#newcode141
Line 141: LOG(FATAL) << "socket() failed: " << strerror(errno);
Nit: why not just CHECK(*sock >= 0) << "..."?

http://codereview.chromium.org/155772/diff/1005/1007#newcode147
Line 147: if (path.length() > sizeof(addr->sun_path) - 1)
ditto here

http://codereview.chromium.org/155772/diff/1005/1007#newcode203
Line 203: // If the path is not a symbol link, try to extrace pid from the path
itself.
s/extrace pid/extract the pid/

http://codereview.chromium.org/155772/diff/1005/1007#newcode204
Line 204: if (!real_path.length())
real_path.empty()

http://codereview.chromium.org/155772/diff/1005/1007#newcode218
Line 218: kill(static_cast<base::ProcessHandle>(pid), SIGKILL);
How about base::KillProcess()?  It'll use SIGTERM first, and if that fails, use
SIGKILL.

http://codereview.chromium.org/155772/diff/1005/1007#newcode274
Line 274: public:
There shouldn't be any reason to have public: twice in the class definition. 
It's already there at the beginning.  You should also reorder the sections
appropriately.  In particular, type definitions come first, so the SocketReader
class should move to the beginning of the public: section.

http://codereview.chromium.org/155772/diff/1005/1007#newcode364
Line 364: SetNonBlocking(connection_socket);
You should check the return value.

http://codereview.chromium.org/155772/diff/1005/1007#newcode507
Line 507: 
No need for this newline here.

http://codereview.chromium.org/155772/diff/1005/1007#newcode509
Line 509: // No necessary to care about the return value.
s/No/Not/

http://codereview.chromium.org/155772/diff/1005/1007#newcode540
Line 540: CloseSocket(socket);
Small nit/suggestion, feel free to disagree/ignore:
Perhaps you can write a small SocketCloser class so and instantiate it on the
stack here, so the socket is guaranteed to close on all exit paths?

http://codereview.chromium.org/155772/diff/1005/1007#newcode575
Line 575: // TODO(james.su@gmail.com): Is it necessary?
This is necessary right?  The SocketReader reads until read() returns 0.  We use
shutdown() here to signal the end of writes.

http://codereview.chromium.org/155772/diff/1005/1007#newcode603
Line 603: NOTREACHED() << "The other process returns unknown message: " << buf;
s/returns/returned/.

[james.su_gmail.com, 2009-07-22 03:12:25]

Thanks for your comment. I'll update the CL asap.

Regards
James Su

http://codereview.chromium.org/155772/diff/1005/1007
File chrome/browser/process_singleton_linux.cc (right):

http://codereview.chromium.org/155772/diff/1005/1007#newcode9
Line 9: // exits.
On 2009/07/22 00:27:45, willchan wrote:
> Please update this comment to explain the use of symlinks.  If you could
perhaps
> copy/paste the ls output so the file system layout is obvious, that might be
> clearest.
Ok.

http://codereview.chromium.org/155772/diff/1005/1007#newcode20
Line 20: #include <set>
On 2009/07/22 00:27:45, willchan wrote:
> There should be an include for <string> for std::string.

Ok.

http://codereview.chromium.org/155772/diff/1005/1007#newcode65
Line 65: DCHECK_EQ(0, rv) << "Error closing socket: " << strerror(errno);
On 2009/07/22 00:27:45, willchan wrote:
> I think we need <string.h> for strerror.

<cstring> would be better.

http://codereview.chromium.org/155772/diff/1005/1007#newcode141
Line 141: LOG(FATAL) << "socket() failed: " << strerror(errno);
On 2009/07/22 00:27:45, willchan wrote:
> Nit: why not just CHECK(*sock >= 0) << "..."?

I'll change it. It's actually the original code :-)

http://codereview.chromium.org/155772/diff/1005/1007#newcode147
Line 147: if (path.length() > sizeof(addr->sun_path) - 1)
On 2009/07/22 00:27:45, willchan wrote:
> ditto here

Will change it.

http://codereview.chromium.org/155772/diff/1005/1007#newcode204
Line 204: if (!real_path.length())
On 2009/07/22 00:27:45, willchan wrote:
> real_path.empty()

It's better :-)

http://codereview.chromium.org/155772/diff/1005/1007#newcode218
Line 218: kill(static_cast<base::ProcessHandle>(pid), SIGKILL);
On 2009/07/22 00:27:45, willchan wrote:
> How about base::KillProcess()?  It'll use SIGTERM first, and if that fails,
use
> SIGKILL.

base::KillProcess() only works on child processes, not suitable for this case.

http://codereview.chromium.org/155772/diff/1005/1007#newcode274
Line 274: public:
On 2009/07/22 00:27:45, willchan wrote:
> There shouldn't be any reason to have public: twice in the class definition. 
> It's already there at the beginning.  You should also reorder the sections
> appropriately.  In particular, type definitions come first, so the
SocketReader
> class should move to the beginning of the public: section.
I'll rearrange this part. Keep SocketReader private would be better.

http://codereview.chromium.org/155772/diff/1005/1007#newcode540
Line 540: CloseSocket(socket);
On 2009/07/22 00:27:45, willchan wrote:
> Small nit/suggestion, feel free to disagree/ignore:
> Perhaps you can write a small SocketCloser class so and instantiate it on the
> stack here, so the socket is guaranteed to close on all exit paths?

Agree.

http://codereview.chromium.org/155772/diff/1005/1007#newcode575
Line 575: // TODO(james.su@gmail.com): Is it necessary?
On 2009/07/22 00:27:45, willchan wrote:
> This is necessary right?  The SocketReader reads until read() returns 0.  We
use
> shutdown() here to signal the end of writes.

I see, will remove the comment.

[james.su_gmail.com, 2009-07-22 07:53:49]

CL has been updated according to your comment. Please help review it again.

Regards
James Su

[willchan no longer on Chromium, 2009-07-22 18:51:02]

lgtm now, although I just realized that there are no tests (we should have added
tests before).  Can you add some basic tests?  I'm fine with TODOs for the
timeouts/acks tests, but I'd like tests to ensure the basic functionality still
works.  Feel free to put me on the TODOs for more tests in this area, since I
failed to add tests when I modified it.

http://codereview.chromium.org/155772/diff/1011/9
File chrome/browser/process_singleton_linux.cc (right):

http://codereview.chromium.org/155772/diff/1011/9#newcode21
Line 21: // Whe the second process sends the current directory and command line
flags to
s/Whe/When/

http://codereview.chromium.org/155772/diff/1011/9#newcode23
Line 23: // for certain time. If there is no ACK message back in time, then the
first
s/for certain/for a certain/

http://codereview.chromium.org/155772/diff/1011/9#newcode24
Line 24: // process will be considered as hanged by some reason. The second
process then
s/hanged by some reason/hung for some reason/

http://codereview.chromium.org/155772/diff/1011/9#newcode165
Line 165: CHECK(path.length() < sizeof(addr->sun_path))
arraysize is preferred to sizeof

http://codereview.chromium.org/155772/diff/1011/9#newcode167
Line 167: base::strlcpy(addr->sun_path, path.c_str(), sizeof(addr->sun_path));
ditto

http://codereview.chromium.org/155772/diff/1011/9#newcode236
Line 236: kill(static_cast<base::ProcessHandle>(pid), SIGKILL);
You should probably DCHECK that the kill() succeeded.

[james.su_gmail.com, 2009-07-23 03:12:57]

Hi,
  CL has been updated. Please help review again. For unittest, can you give me
some advices on how to write an unittest for Chromium? Is there any
documentation about it? Especially it involves browser process and need multi
processes.

Regards
James Su

[willchan no longer on Chromium, 2009-07-23 03:31:17]

On 2009/07/23 03:12:57, james.su wrote:
> Hi,
>   CL has been updated. Please help review again. For unittest, can you give me
> some advices on how to write an unittest for Chromium? Is there any
> documentation about it? Especially it involves browser process and need multi
> processes.
> 
> Regards
> James Su

My recommendation to you is to look at a uitest.cc file.  This uses the
automation framework to communicate to a browser process via IPC, instructing it
what to do.  You can probably start up a browser process, and then use
base::LaunchApp() to start up another process as well, which should terminate
after it causes a tab to open up in the first browser process.  There's also a
new in process browser test framework which might work for this as well (and
people are recommending over uitests since it's in process instead of
communicating over IPC), so you might want to take a look at some browsertest.cc
files as well.  You can look at chrome/test/in_process_browser_test.h and
chrome/test/ui/ui_test.h for information on these two frameworks for browser
tests.

[james.su_gmail.com, 2009-07-23 03:41:46]

Thanks for your information. One question, when I add a unittest file, what
build script shall be changed to be able to build the test?

James Su

On 2009/07/23 03:31:17, willchan wrote:
> On 2009/07/23 03:12:57, james.su wrote:
> > Hi,
> >   CL has been updated. Please help review again. For unittest, can you give
me
> > some advices on how to write an unittest for Chromium? Is there any
> > documentation about it? Especially it involves browser process and need
multi
> > processes.
> > 
> > Regards
> > James Su
> 
> My recommendation to you is to look at a uitest.cc file.  This uses the
> automation framework to communicate to a browser process via IPC, instructing
it
> what to do.  You can probably start up a browser process, and then use
> base::LaunchApp() to start up another process as well, which should terminate
> after it causes a tab to open up in the first browser process.  There's also a
> new in process browser test framework which might work for this as well (and
> people are recommending over uitests since it's in process instead of
> communicating over IPC), so you might want to take a look at some
browsertest.cc
> files as well.  You can look at chrome/test/in_process_browser_test.h and
> chrome/test/ui/ui_test.h for information on these two frameworks for browser
> tests.

[willchan no longer on Chromium, 2009-07-23 03:50:21]

Add your test to chrome/chrome.gyp.  You'll see the appropriate section for the
relevant test files there.  gyp stands for Generate Your Project.  It will
generate the relevant scons/Makefile/.vcproj/xcode files for the platform you're
running on.  In order to update these files after you've edited a .gyp file, you
should run `gclient runhooks --force`, which will run gyp on all the .gyp files
to generate the relevant build project files.

On 2009/07/23 03:41:46, james.su wrote:
> Thanks for your information. One question, when I add a unittest file, what
> build script shall be changed to be able to build the test?
> 
> James Su
> 
> On 2009/07/23 03:31:17, willchan wrote:
> > On 2009/07/23 03:12:57, james.su wrote:
> > > Hi,
> > >   CL has been updated. Please help review again. For unittest, can you
give
> me
> > > some advices on how to write an unittest for Chromium? Is there any
> > > documentation about it? Especially it involves browser process and need
> multi
> > > processes.
> > > 
> > > Regards
> > > James Su
> > 
> > My recommendation to you is to look at a uitest.cc file.  This uses the
> > automation framework to communicate to a browser process via IPC,
instructing
> it
> > what to do.  You can probably start up a browser process, and then use
> > base::LaunchApp() to start up another process as well, which should
terminate
> > after it causes a tab to open up in the first browser process.  There's also
a
> > new in process browser test framework which might work for this as well (and
> > people are recommending over uitests since it's in process instead of
> > communicating over IPC), so you might want to take a look at some
> browsertest.cc
> > files as well.  You can look at chrome/test/in_process_browser_test.h and
> > chrome/test/ui/ui_test.h for information on these two frameworks for browser
> > tests.

[james.su_gmail.com, 2009-07-24 12:36:40]

Hi,
  A simple unittest has been implemented based on InProcessBrowserTest. However
it's far from complete. I have no idea on how to test following cases:
1. NotifyOtherProcess() returns false because browser process is being shutting
down.
2. NotifyOtherProcess() kills browser process and returns false.
3. NotifyOtherProcess() returns true because browser process's ProcessSingleton
object is locked.

Please help review the code and give me some advices.
Regards
James Su

[willchan no longer on Chromium, 2009-07-24 21:30:57]

Don't worry about case (1) for now.  I need to think about that some more.
Case (2) is doable.  Post a task to the IO thread that sleeps or otherwise
deadlocks.  Then start up another process via base::LaunchApp() and make sure
the first process dies and then second one works.  You will have to use the ui
test framework instead of the in process one, since you don't want the test
process to get killed.
Don't worry about case (3), that only happens in windows.

http://codereview.chromium.org/155772/diff/2001/1016
File chrome/browser/process_singleton_linux_unittest.cc (right):

http://codereview.chromium.org/155772/diff/2001/1016#newcode85
Line 85: original_tab_count_ = browser()->tab_count();
Why is original_tab_count_ a member variable?

http://codereview.chromium.org/155772/diff/2001/1016#newcode95
Line 95: ASSERT_EQ(original_tab_count_ + 1, browser()->tab_count());
You should also add an assertion that the new tab is the correct url.  You can
access the TabContents from the Browser() object and get the GURL from that.

[james.su_gmail.com, 2009-07-27 03:39:36]

Hi,
  Thanks for your feedback. I'll update the CL asap. Just two questions:
1. How to test if a process is killed or not?
2. If ui test framework is used, how to block the io thread in browser process,
which is different than the test process.

On 2009/07/24 21:30:57, willchan wrote:
> Don't worry about case (1) for now.  I need to think about that some more.
> Case (2) is doable.  Post a task to the IO thread that sleeps or otherwise
> deadlocks.  Then start up another process via base::LaunchApp() and make sure
> the first process dies and then second one works.  You will have to use the ui
> test framework instead of the in process one, since you don't want the test
> process to get killed.
> Don't worry about case (3), that only happens in windows.
> 
> http://codereview.chromium.org/155772/diff/2001/1016
> File chrome/browser/process_singleton_linux_unittest.cc (right):
> 
> http://codereview.chromium.org/155772/diff/2001/1016#newcode85
> Line 85: original_tab_count_ = browser()->tab_count();
> Why is original_tab_count_ a member variable?
> 
> http://codereview.chromium.org/155772/diff/2001/1016#newcode95
> Line 95: ASSERT_EQ(original_tab_count_ + 1, browser()->tab_count());
> You should also add an assertion that the new tab is the correct url.  You can
> access the TabContents from the Browser() object and get the GURL from that.

[james.su_gmail.com, 2009-07-27 11:00:41]

Hi,
  I just changed the test to use UITest. Now it can test both success and
failure case of ProcessSingleton::NotifyOtherProcess().

Regards
James Su

[willchan no longer on Chromium, 2009-07-27 16:09:00]

On 2009/07/27 11:00:41, james.su wrote:
> Hi,
>   I just changed the test to use UITest. Now it can test both success and
> failure case of ProcessSingleton::NotifyOtherProcess().
> 
> Regards
> James Su

I don't see the test file in this patch...can you add it in?

[james.su_gmail.com, 2009-07-28 00:32:16]

Sorry, my fault. The test file is not attached.

Regards
James Su

On 2009/07/27 16:09:00, willchan wrote:
> On 2009/07/27 11:00:41, james.su wrote:
> > Hi,
> >   I just changed the test to use UITest. Now it can test both success and
> > failure case of ProcessSingleton::NotifyOtherProcess().
> > 
> > Regards
> > James Su
> 
> I don't see the test file in this patch...can you add it in?

[willchan no longer on Chromium, 2009-07-28 04:35:49]

http://codereview.chromium.org/155772/diff/2018/2020
File chrome/browser/process_singleton_linux.cc (right):

http://codereview.chromium.org/155772/diff/2018/2020#newcode113
Line 113: // Wait a socket for read for a certain timeout in seconds.
Please document the return value.

http://codereview.chromium.org/155772/diff/2018/2020#newcode608
Line 608: // TODO(james.su@gmail.com): Is 5 seconds enough?
In windows we wait 20 seconds.  It's probably good to be consistent.  Perhaps
you should move the constant into process_singleton.h.

http://codereview.chromium.org/155772/diff/2018/2022
File chrome/browser/process_singleton_linux_uitest.cc (right):

http://codereview.chromium.org/155772/diff/2018/2022#newcode52
Line 52: // are valid. When running this test, ProcessSingleton object is
already
s/, ProcessSingleton/, the ProcessSingleton/

http://codereview.chromium.org/155772/diff/2018/2022#newcode74
Line 74: // Test success case of NotifyOtherProcess().
There's nothing linux specific about the following 2 tests.  Can you move them
into a process_singleton_uitest.cc file, so they'll get run for the other
platforms?  And then make the above one a standard unittest (I don't think you
need a UITest or an in process browser test for it, right?).  It's preferable to
avoid UITest and in process browser test when possible, so we don't have to pay
the expensive browser/process startup cost.  You have your chromium account now,
right?  You should use the trybots to make sure the new tests pass on the other
platforms.

Sorry for jerking you around about using a browser test vs a ui test or a
unittest.  I didn't think everything through from the beginning.  And thanks
again for writing the tests, it'll be great to finally get some test coverage
here.

http://codereview.chromium.org/155772/diff/2018/2022#newcode82
Line 82: ASSERT_EQ(url, GetActiveTabURL().spec());
EXPECT_EQ

http://codereview.chromium.org/155772/diff/2018/2022#newcode95
Line 95: usleep(10000);
Ugh, this makes our tests take a lot longer to run.  I wonder if there's
anything we can do to eliminate the need for this long sleep.

http://codereview.chromium.org/155772/diff/2018/2022#newcode97
Line 97: ASSERT_FALSE(IsBrowserRunning());
EXPECT_EQ

[james.su_gmail.com, 2009-07-28 05:15:57]

Hi,
  Thanks for your feedback. Please see my comments below. And unfortunately, I
still don't have chromium account yet.

Regards
James Su

http://codereview.chromium.org/155772/diff/2018/2020
File chrome/browser/process_singleton_linux.cc (right):

http://codereview.chromium.org/155772/diff/2018/2020#newcode113
Line 113: // Wait a socket for read for a certain timeout in seconds.
On 2009/07/28 04:35:49, willchan wrote:
> Please document the return value.

Ok.

http://codereview.chromium.org/155772/diff/2018/2020#newcode608
Line 608: // TODO(james.su@gmail.com): Is 5 seconds enough?
On 2009/07/28 04:35:49, willchan wrote:
> In windows we wait 20 seconds.  It's probably good to be consistent.  Perhaps
> you should move the constant into process_singleton.h.

Ok

http://codereview.chromium.org/155772/diff/2018/2022
File chrome/browser/process_singleton_linux_uitest.cc (right):

http://codereview.chromium.org/155772/diff/2018/2022#newcode74
Line 74: // Test success case of NotifyOtherProcess().
On 2009/07/28 04:35:49, willchan wrote:
> There's nothing linux specific about the following 2 tests.  Can you move them
> into a process_singleton_uitest.cc file, so they'll get run for the other
> platforms?  And then make the above one a standard unittest (I don't think you
> need a UITest or an in process browser test for it, right?).  It's preferable
to
> avoid UITest and in process browser test when possible, so we don't have to
pay
> the expensive browser/process startup cost.  You have your chromium account
now,
> right?  You should use the trybots to make sure the new tests pass on the
other
> platforms.
> 
> Sorry for jerking you around about using a browser test vs a ui test or a
> unittest.  I didn't think everything through from the beginning.  And thanks
> again for writing the tests, it'll be great to finally get some test coverage
> here.

NotifyOtherProcessSuccess() is platform independent. But I don't know how to do
kill(process(), SIGSTOP) on Win.
ProcessSingleton is useless on Mac, so we only need these tests for Win and
Linux.
And above test requires browser process, because ProcessSingleton::Create
requires it.

http://codereview.chromium.org/155772/diff/2018/2022#newcode82
Line 82: ASSERT_EQ(url, GetActiveTabURL().spec());
On 2009/07/28 04:35:49, willchan wrote:
> EXPECT_EQ

ok

http://codereview.chromium.org/155772/diff/2018/2022#newcode95
Line 95: usleep(10000);
On 2009/07/28 04:35:49, willchan wrote:
> Ugh, this makes our tests take a lot longer to run.  I wonder if there's
> anything we can do to eliminate the need for this long sleep.

10000 microseconds is a very short time indeed. Without this short sleep, the
parent process won't receive the state of child process. I don't know why.

http://codereview.chromium.org/155772/diff/2018/2022#newcode97
Line 97: ASSERT_FALSE(IsBrowserRunning());
On 2009/07/28 04:35:49, willchan wrote:
> EXPECT_EQ

Do you mean EXPECT_FALSE?

[willchan no longer on Chromium, 2009-07-28 05:47:38]

Please upload your latest snapshot, I didn't see a change.

http://codereview.chromium.org/155772/diff/2018/2022
File chrome/browser/process_singleton_linux_uitest.cc (right):

http://codereview.chromium.org/155772/diff/2018/2022#newcode74
Line 74: // Test success case of NotifyOtherProcess().
On 2009/07/28 05:15:57, james.su wrote:
> On 2009/07/28 04:35:49, willchan wrote:
> > There's nothing linux specific about the following 2 tests.  Can you move
them
> > into a process_singleton_uitest.cc file, so they'll get run for the other
> > platforms?  And then make the above one a standard unittest (I don't think
you
> > need a UITest or an in process browser test for it, right?).  It's
preferable
> to
> > avoid UITest and in process browser test when possible, so we don't have to
> pay
> > the expensive browser/process startup cost.  You have your chromium account
> now,
> > right?  You should use the trybots to make sure the new tests pass on the
> other
> > platforms.
> > 
> > Sorry for jerking you around about using a browser test vs a ui test or a
> > unittest.  I didn't think everything through from the beginning.  And thanks
> > again for writing the tests, it'll be great to finally get some test
coverage
> > here.
> 
> NotifyOtherProcessSuccess() is platform independent. But I don't know how to
do
> kill(process(), SIGSTOP) on Win.

I believe we need to use SuspendThread(), but I know very little about Windows. 
I'm fine with not doing this for now then.  You should leave a TODO in the code
to port this to windows since we should implement a PauseProcess() function or
something that uses SuspendThread() to do this.  I still suggest you move these
bottom two tests to process_singleton_uitest.cc since the other platforms will
eventually want to have this test.  You should only add the uitest for linux
though in chrome.gyp.

> ProcessSingleton is useless on Mac, so we only need these tests for Win and
> Linux.
> And above test requires browser process, because ProcessSingleton::Create
> requires it.
> 

It's not useless on Mac.  It'll be helpful for the mac team to know that they
can enable this for Mac when they eventually implement it.  Our typical strategy
is to try to write cross platform tests as much as possible, and add it to the
appropriate test target (ui_tests in this case).  Then we use "sources!" to
subtract the source from the relevant platforms.  You can see this in chrome.gyp
where we disable certain tests in certain platforms.

You're right about needing the browser process, so it's fine to keep this as a
ui_test.  Thanks for correcting me.

http://codereview.chromium.org/155772/diff/2018/2022#newcode95
Line 95: usleep(10000);
On 2009/07/28 05:15:57, james.su wrote:
> On 2009/07/28 04:35:49, willchan wrote:
> > Ugh, this makes our tests take a lot longer to run.  I wonder if there's
> > anything we can do to eliminate the need for this long sleep.
> 
> 10000 microseconds is a very short time indeed. Without this short sleep, the
> parent process won't receive the state of child process. I don't know why.
> 

Er, I'm an idiot, I forgot usleep was in microseconds.  How does this work then?
 Don't you have to wait longer than the timeout?  I thought the timeout was set
to 5 seconds (which you have changed to 20 seconds now, right?), so why is it
enough to only wait 10ms?

http://codereview.chromium.org/155772/diff/2018/2022#newcode97
Line 97: ASSERT_FALSE(IsBrowserRunning());
On 2009/07/28 05:15:57, james.su wrote:
> On 2009/07/28 04:35:49, willchan wrote:
> > EXPECT_EQ
> 
> Do you mean EXPECT_FALSE?

Oops...um...yes :)

[james.su_gmail.com, 2009-07-28 07:42:50]

The latest code has been uploaded. I'd prefer to keep the tests for linux only
for now, until following things are ready:
1. A cross platform PauseProcess()
2. Fix Windows implementation of CommandLine, so that we can customize current
command line by invoking Init().
3. Modify process_singleton_win.cc to use CommandLine to read arguments instead
of ::GetCommandLineW().

Regards
James Su

http://codereview.chromium.org/155772/diff/2018/2022
File chrome/browser/process_singleton_linux_uitest.cc (right):

http://codereview.chromium.org/155772/diff/2018/2022#newcode95
Line 95: usleep(10000);
On 2009/07/28 05:47:38, willchan wrote:
> On 2009/07/28 05:15:57, james.su wrote:
> > On 2009/07/28 04:35:49, willchan wrote:
> > > Ugh, this makes our tests take a lot longer to run.  I wonder if there's
> > > anything we can do to eliminate the need for this long sleep.
> > 
> > 10000 microseconds is a very short time indeed. Without this short sleep,
the
> > parent process won't receive the state of child process. I don't know why.
> > 
> 
> Er, I'm an idiot, I forgot usleep was in microseconds.  How does this work
then?
>  Don't you have to wait longer than the timeout?  I thought the timeout was
set
> to 5 seconds (which you have changed to 20 seconds now, right?), so why is it
> enough to only wait 10ms?

This delay has nothing to do with the timeout for socket communication. When
above NotifyOtherProcess() returns, the browser process should already been
killed, however I found that without this short delay, this test will fail in
UITest::QuitBrowser(), because it's not able to retrieve the status of browser
process correctly.

[willchan no longer on Chromium, 2009-07-28 17:57:21]

Ok, given the cross platform issues you've stated, I'm fine keeping this linux
specific for now.

http://codereview.chromium.org/155772/diff/1034/1035
File chrome/browser/process_singleton.h (right):

http://codereview.chromium.org/155772/diff/1034/1035#newcode106
Line 106: static const int kTimeoutInSeconds = 20;
This is just a declaration.  According to the C++ standard, you need a
corresponding definition.  You should place one in _win.cc, _mac.cc, and
_linux.cc.  I didn't see you add process_singleton_win.cc to the changelist, did
you update it to use this constant?

[james.su_gmail.com, 2009-07-29 00:25:35]

Please see my comment below.

Regards
James Su

http://codereview.chromium.org/155772/diff/1034/1035
File chrome/browser/process_singleton.h (right):

http://codereview.chromium.org/155772/diff/1034/1035#newcode106
Line 106: static const int kTimeoutInSeconds = 20;
On 2009/07/28 17:57:22, willchan wrote:
> This is just a declaration.  According to the C++ standard, you need a
> corresponding definition.  You should place one in _win.cc, _mac.cc, and
> _linux.cc.  I didn't see you add process_singleton_win.cc to the changelist,
did
> you update it to use this constant?

Though it's just a declaration according to standard, it works just like a
constant if nobody tries to get its address. If you think it's necessary to add
definitions in each implementation files, then I'd prefer to move it back to
implementation files as private constants, because in any case, we can't share
the value among all platforms. Unless macro is used instead.

And please forgive my careless, I just forgot to add process_singleton_win.cc
:-) Anyway, this file won't be changed if you think it's ok to revert to
original implementation.

[james.su_gmail.com, 2009-07-29 00:58:21]

CL updated, please help review again.

Regards
James Su

[willchan no longer on Chromium, 2009-07-29 01:08:35]

LGTM, just have a small nitpick about constant placement.

http://codereview.chromium.org/155772/diff/1042/2034
File chrome/browser/process_singleton_linux.cc (right):

http://codereview.chromium.org/155772/diff/1042/2034#newcode259
Line 259: const int ProcessSingleton::kTimeoutInSeconds;
Can you move it to the top of the file, above the anonymous namespace?  It feels
more natural to me to see constants at the top.

http://codereview.chromium.org/155772/diff/1042/2037
File chrome/browser/process_singleton_win.cc (right):

http://codereview.chromium.org/155772/diff/1042/2037#newcode36
Line 36: const int ProcessSingleton::kTimeoutInSeconds;
Can you move this above the anonymous namespace?

[james.su_gmail.com, 2009-07-29 01:14:11]

Done. Thanks for your review.

http://codereview.chromium.org/155772/diff/1042/2034
File chrome/browser/process_singleton_linux.cc (right):

http://codereview.chromium.org/155772/diff/1042/2034#newcode259
Line 259: const int ProcessSingleton::kTimeoutInSeconds;
On 2009/07/29 01:08:35, willchan wrote:
> Can you move it to the top of the file, above the anonymous namespace?  It
feels
> more natural to me to see constants at the top.

Done.

http://codereview.chromium.org/155772/diff/1042/2037
File chrome/browser/process_singleton_win.cc (right):

http://codereview.chromium.org/155772/diff/1042/2037#newcode36
Line 36: const int ProcessSingleton::kTimeoutInSeconds;
On 2009/07/29 01:08:35, willchan wrote:
> Can you move this above the anonymous namespace?

Done.

[willchan no longer on Chromium, 2009-07-29 19:21:29]

I landed this but then had to revert it due to the ui test failure in valgrind
here:
http://chrome-buildbot.corp.google.com:8010/builders/Linux%20UI%203%20of%203%...
Can you please look into this?  My suspicion is that the process hasn't
completely stopped before the connect(), since the connect() is succeeding. 
This might be a race condition exposed by valgrind.

[james.su_gmail.com, 2009-07-30 02:27:10]

On 2009/07/29 19:21:29, willchan wrote:
> I landed this but then had to revert it due to the ui test failure in valgrind
> here:
>
http://chrome-buildbot.corp.google.com:8010/builders/Linux%20UI%203%20of%203%...
> Can you please look into this?  My suspicion is that the process hasn't
> completely stopped before the connect(), since the connect() is succeeding. 
> This might be a race condition exposed by valgrind.

I'll look into it asap.

[willchan no longer on Chromium, 2009-07-31 04:19:47]

I'm closing this code review since suzhe's opened up another one.