Wait for pending requests to finish in ImageDecoder.

Wait for pending requests to finish in ImageDecoder.

Otherwise in slow/busy computing environments, image decoding may never
succeed / fail too frequently.

BUG=484915
Committed: https://crrev.com/3d8ebe3a1a270361f51b6777dd328ef288115437
Cr-Commit-Position: refs/heads/master@{#328916}

[Lei Zhang, 2015-05-06 01:00:04]

Patchset #1 (id:1) has been deleted

[Lei Zhang, 2015-05-06 01:06:27]

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

[Lei Zhang, 2015-05-06 01:41:04]

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

[commit-bot: I haz the power, 2015-05-06 01:41:28]

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

[Anand Mistry (off Chromium), 2015-05-06 02:46:09]

lgtm

[commit-bot: I haz the power, 2015-05-06 02:53:39]

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

[commit-bot: I haz the power, 2015-05-06 02:53:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Lei Zhang, 2015-05-06 09:23:06]

This needs another look since the CL has changed a bit. StartAndKillProcess is
still failing on Windows so I'll have to look at that tomorrow when my Windows
box is awake. I'm worried StartAndKillProcess might have been broken and relied
on the timeout as a crutch.

[Lei Zhang, 2015-05-07 00:24:14]

Ok, time for another look.

https://codereview.chromium.org/1129653002/diff/100001/chrome/browser/image_d...
File chrome/browser/image_decoder_browsertest.cc (left):

https://codereview.chromium.org/1129653002/diff/100001/chrome/browser/image_d...
chrome/browser/image_decoder_browsertest.cc:85:
EXPECT_TRUE(base::Process(handle).Terminate(0, true));
0 is "exit normally" on Windows -> OnProcessCrashed() does not get called.

[Lei Zhang, 2015-05-07 04:16:47]

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

[Lei Zhang, 2015-05-07 04:16:47]

The patchset sent to the CQ was uploaded after l-g-t-m from amistry@chromium.org
Link to the patchset: https://codereview.chromium.org/1129653002/#ps120001
(title: "fix mac")

[commit-bot: I haz the power, 2015-05-07 04:17:41]

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

[commit-bot: I haz the power, 2015-05-07 05:31:16]

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

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

Dry run: This issue passed the CQ dry run.

[Anand Mistry (off Chromium), 2015-05-07 05:35:23]

https://codereview.chromium.org/1129653002/diff/100001/chrome/browser/image_d...
File chrome/browser/image_decoder_browsertest.cc (right):

https://codereview.chromium.org/1129653002/diff/100001/chrome/browser/image_d...
chrome/browser/image_decoder_browsertest.cc:145: EXPECT_EQ(0, kill(handle,
SIGCONT));
I'm not 100% sure, but I suspect there's a race here similar to the
StartAndKillProcess case. Consider:
1. Start decoding an image, which starts a utility process and sends the
request.
2. Process creation posts the observer callback to the UI thread.
3. Decoding finishes and result is received.
4. UI thread runs observer.
5. ImageDecoder timeout fires and kills the utility process since there are no
outstanding requests.
6. This function is run and kill(SIGCONT) returns -1 with errno = ESRCH because
the process doesn't exist.

https://codereview.chromium.org/1129653002/diff/100001/chrome/browser/image_d...
chrome/browser/image_decoder_browsertest.cc:209: // checks for both possible
valid outcomes.
I should have commented this when I wrote this test, but the goal was to test
that requests aren't left dangling when the utility process crashes (and hence
would time out the test). Not to test that the request fails, which as you
mention, is racy. I think it should be added to this comment so that "future
folk" don't get confused.

[Lei Zhang, 2015-05-07 22:31:05]

https://codereview.chromium.org/1129653002/diff/100001/chrome/browser/image_d...
File chrome/browser/image_decoder_browsertest.cc (right):

https://codereview.chromium.org/1129653002/diff/100001/chrome/browser/image_d...
chrome/browser/image_decoder_browsertest.cc:145: EXPECT_EQ(0, kill(handle,
SIGCONT));
On 2015/05/07 05:35:22, Anand Mistry wrote:
> I'm not 100% sure, but I suspect there's a race here similar to the
> StartAndKillProcess case. Consider:
> 1. Start decoding an image, which starts a utility process and sends the
> request.
> 2. Process creation posts the observer callback to the UI thread.
> 3. Decoding finishes and result is received.
> 4. UI thread runs observer.
> 5. ImageDecoder timeout fires and kills the utility process since there are no
> outstanding requests.
> 6. This function is run and kill(SIGCONT) returns -1 with errno = ESRCH
because
> the process doesn't exist.

How do you feel if I just removed this test? It no longer tests what we
originally wanted it to test, and trying to fix the raciness is more work than
what I'd like to do here.

[Lei Zhang, 2015-05-07 22:36:23]

https://codereview.chromium.org/1129653002/diff/100001/chrome/browser/image_d...
File chrome/browser/image_decoder_browsertest.cc (right):

https://codereview.chromium.org/1129653002/diff/100001/chrome/browser/image_d...
chrome/browser/image_decoder_browsertest.cc:209: // checks for both possible
valid outcomes.
On 2015/05/07 05:35:22, Anand Mistry wrote:
> I should have commented this when I wrote this test, but the goal was to test
> that requests aren't left dangling when the utility process crashes (and hence
> would time out the test). Not to test that the request fails, which as you
> mention, is racy. I think it should be added to this comment so that "future
> folk" don't get confused.

Done.

[Anand Mistry (off Chromium), 2015-05-08 00:02:11]

LGTM with test removal.

https://codereview.chromium.org/1129653002/diff/100001/chrome/browser/image_d...
File chrome/browser/image_decoder_browsertest.cc (right):

https://codereview.chromium.org/1129653002/diff/100001/chrome/browser/image_d...
chrome/browser/image_decoder_browsertest.cc:145: EXPECT_EQ(0, kill(handle,
SIGCONT));
On 2015/05/07 22:31:05, Lei Zhang wrote:
> On 2015/05/07 05:35:22, Anand Mistry wrote:
> > I'm not 100% sure, but I suspect there's a race here similar to the
> > StartAndKillProcess case. Consider:
> > 1. Start decoding an image, which starts a utility process and sends the
> > request.
> > 2. Process creation posts the observer callback to the UI thread.
> > 3. Decoding finishes and result is received.
> > 4. UI thread runs observer.
> > 5. ImageDecoder timeout fires and kills the utility process since there are
no
> > outstanding requests.
> > 6. This function is run and kill(SIGCONT) returns -1 with errno = ESRCH
> because
> > the process doesn't exist.
> 
> How do you feel if I just removed this test? It no longer tests what we
> originally wanted it to test, and trying to fix the raciness is more work than
> what I'd like to do here.

That sounds fine.

[Lei Zhang, 2015-05-08 02:35:01]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2015-05-08 02:35:01]

The patchset sent to the CQ was uploaded after l-g-t-m from amistry@chromium.org
Link to the patchset: https://codereview.chromium.org/1129653002/#ps160001
(title: "remove freeze test")

[commit-bot: I haz the power, 2015-05-08 02:35:21]

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

[commit-bot: I haz the power, 2015-05-08 03:25:44]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-05-08 03:26:48]

Patchset 8 (id:??) landed as
https://crrev.com/3d8ebe3a1a270361f51b6777dd328ef288115437
Cr-Commit-Position: refs/heads/master@{#328916}

[Nico, 2015-05-10 21:19:52]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-05-10 21:19:53]

The test became very flaky after this CL on OS X 10.9:
http://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%29...