Fix ImageDecoderBrowserTest.StartAndKillProcess flakyness and re-enable.

Fix ImageDecoderBrowserTest.StartAndKillProcess flakyness and re-enable.

BUG=486194
Committed: https://crrev.com/89f2e8c68ef88714dcacecb42e6e673a153e44c7
Cr-Commit-Position: refs/heads/master@{#329345}

[Anand Mistry (off Chromium), 2015-05-11 07:11:17]

The CQ bit was checked by amistry@chromium.org to run a CQ dry run

[Anand Mistry (off Chromium), 2015-05-11 07:11:59]

amistry@chromium.org changed reviewers:
+ thestig@chromium.org

[commit-bot: I haz the power, 2015-05-11 07:15:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1137993002/1

[commit-bot: I haz the power, 2015-05-11 08:18:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-11 08:18:52]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2015-05-11 23:09:19]

https://codereview.chromium.org/1137993002/diff/1/chrome/browser/image_decode...
File chrome/browser/image_decoder_browsertest.cc (right):

https://codereview.chromium.org/1137993002/diff/1/chrome/browser/image_decode...
chrome/browser/image_decoder_browsertest.cc:111: // Don't wait for the process
after sending the kill signal. According to
By 'kill signal', did you mean SIGTERM?

https://codereview.chromium.org/1137993002/diff/1/chrome/browser/image_decode...
chrome/browser/image_decoder_browsertest.cc:115: // host detegate's
|OnProcessCrashed|.
nit: typo

https://codereview.chromium.org/1137993002/diff/1/chrome/browser/image_decode...
chrome/browser/image_decoder_browsertest.cc:116:
EXPECT_TRUE(base::Process(handle).Terminate(1, false));
Maybe we should post this to the IO thread, so the kill() calls don't race
against each other.

[Anand Mistry (off Chromium), 2015-05-12 00:05:32]

https://codereview.chromium.org/1137993002/diff/1/chrome/browser/image_decode...
File chrome/browser/image_decoder_browsertest.cc (right):

https://codereview.chromium.org/1137993002/diff/1/chrome/browser/image_decode...
chrome/browser/image_decoder_browsertest.cc:111: // Don't wait for the process
after sending the kill signal. According to
On 2015/05/11 23:09:19, Lei Zhang wrote:
> By 'kill signal', did you mean SIGTERM?

Yes.

https://codereview.chromium.org/1137993002/diff/1/chrome/browser/image_decode...
chrome/browser/image_decoder_browsertest.cc:115: // host detegate's
|OnProcessCrashed|.
On 2015/05/11 23:09:19, Lei Zhang wrote:
> nit: typo

Done.

https://codereview.chromium.org/1137993002/diff/1/chrome/browser/image_decode...
chrome/browser/image_decoder_browsertest.cc:116:
EXPECT_TRUE(base::Process(handle).Terminate(1, false));
On 2015/05/11 23:09:19, Lei Zhang wrote:
> Maybe we should post this to the IO thread, so the kill() calls don't race
> against each other.

I don't think it's possible for the two to race. If this functions runs, it
means the UI message loop is still looping, which means the request's response
functions haven't been called (and quit the loop), which means there are still
requests in the image decoder's request map, which means the image decoder won't
try to kill the decoder process. Phew, that was a mouthful.

[Lei Zhang, 2015-05-12 02:23:25]

lgtm

https://codereview.chromium.org/1137993002/diff/1/chrome/browser/image_decode...
File chrome/browser/image_decoder_browsertest.cc (right):

https://codereview.chromium.org/1137993002/diff/1/chrome/browser/image_decode...
chrome/browser/image_decoder_browsertest.cc:116:
EXPECT_TRUE(base::Process(handle).Terminate(1, false));
On 2015/05/12 00:05:31, Anand Mistry wrote:
> On 2015/05/11 23:09:19, Lei Zhang wrote:
> > Maybe we should post this to the IO thread, so the kill() calls don't race
> > against each other.
> 
> I don't think it's possible for the two to race. If this functions runs, it
> means the UI message loop is still looping, which means the request's response
> functions haven't been called (and quit the loop), which means there are still
> requests in the image decoder's request map, which means the image decoder
won't
> try to kill the decoder process. Phew, that was a mouthful.

I was thinking if we had left this as true, then the code runs waitpid() on the
UI thread, but at the same time, the IO thread is calling
GetChildTerminationStatus() which may also involve calling kill() and waitpid().
But ya, I guess we don't really need to wait, so n/m.

https://codereview.chromium.org/1137993002/diff/60001/chrome/browser/image_de...
File chrome/browser/image_decoder_browsertest.cc (right):

https://codereview.chromium.org/1137993002/diff/60001/chrome/browser/image_de...
chrome/browser/image_decoder_browsertest.cc:115: // invoke the process host
delegate's |OnProcessCrashed|.
nit: OnProcessCrashes()

[Anand Mistry (off Chromium), 2015-05-12 03:12:21]

https://codereview.chromium.org/1137993002/diff/60001/chrome/browser/image_de...
File chrome/browser/image_decoder_browsertest.cc (right):

https://codereview.chromium.org/1137993002/diff/60001/chrome/browser/image_de...
chrome/browser/image_decoder_browsertest.cc:115: // invoke the process host
delegate's |OnProcessCrashed|.
On 2015/05/12 02:23:24, Lei Zhang wrote:
> nit: OnProcessCrashes()

Done. And I double-checked, it is OnProcessCrashed, not Crashes.

[Anand Mistry (off Chromium), 2015-05-12 03:12:28]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2015-05-12 03:12:29]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1137993002/#ps80001
(title: "Comment change.")

[commit-bot: I haz the power, 2015-05-12 03:13:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1137993002/80001

[commit-bot: I haz the power, 2015-05-12 03:55:51]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-12 03:56:36]

Patchset 5 (id:??) landed as
https://crrev.com/89f2e8c68ef88714dcacecb42e6e673a153e44c7
Cr-Commit-Position: refs/heads/master@{#329345}