Fail all started requests if the utility process crashes or fails to start.

Fail all started requests if the utility process crashes or fails to start.

Committed: https://crrev.com/d59fe6bfbf19bbc01a2ff9f39d6300339f577a3f
Cr-Commit-Position: refs/heads/master@{#327837}

[Anand Mistry (off Chromium), 2015-04-08 04:00:53]

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

[Anand Mistry (off Chromium), 2015-04-08 04:00:53]

This change depends on https://codereview.chromium.org/1068743002/

[Lei Zhang, 2015-04-29 19:35:48]

On 2015/04/08 04:00:53, Anand Mistry (away till 30th) wrote:
> This change depends on https://codereview.chromium.org/1068743002/

Since that change didn't land. You'll need to rework this CL a bit.

[Anand Mistry (off Chromium), 2015-04-30 04:14:52]

On 2015/04/29 19:35:48, Lei Zhang wrote:
> On 2015/04/08 04:00:53, Anand Mistry (away till 30th) wrote:
> > This change depends on https://codereview.chromium.org/1068743002/
> 
> Since that change didn't land. You'll need to rework this CL a bit.

Done. PTAL.

[Lei Zhang, 2015-04-30 05:31:14]

lgtm

[Lei Zhang, 2015-04-30 05:31:48]

but it looks like the windows trybots are unhappy

[chromium-reviews, 2015-04-30 05:38:54]

Yeah, looks like the process handle gets closed on windows when you're
using base::Process. I need to either duplicate the process handle, or
figure out another way to kill the process. That, or disable the test under
windows, which I'd rather avoid.

On 30 April 2015 at 15:31, <thestig@chromium.org> wrote:

> but it looks like the windows trybots are unhappy
>
> https://codereview.chromium.org/1071453002/
>



-- 
Anand K. Mistry
Software Engineer
*Google* Australia

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Anand Mistry (off Chromium), 2015-04-30 06:41:12]

Fixed test on windows. PTAL.

[Lei Zhang, 2015-04-30 08:08:52]

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

https://codereview.chromium.org/1071453002/diff/60001/chrome/browser/image_de...
chrome/browser/image_decoder_browsertest.cc:78: if
(!::DuplicateHandle(GetCurrentProcess(), handle,
I would:
- wrap |data.handle| in a base::Process object
-- check if it IsValid()
-- call dup = base::Process::Duplicate()
-- then dup.Terminate().

That way the code here can be platform agnostic, and you don't touch the raw
handle much.

[Anand Mistry (off Chromium), 2015-04-30 19:17:34]

On 2015/04/30 08:08:52, Lei Zhang wrote:
>
https://codereview.chromium.org/1071453002/diff/60001/chrome/browser/image_de...
> File chrome/browser/image_decoder_browsertest.cc (right):
> 
>
https://codereview.chromium.org/1071453002/diff/60001/chrome/browser/image_de...
> chrome/browser/image_decoder_browsertest.cc:78: if
> (!::DuplicateHandle(GetCurrentProcess(), handle,
> I would:
> - wrap |data.handle| in a base::Process object
> -- check if it IsValid()
> -- call dup = base::Process::Duplicate()
> -- then dup.Terminate().
> 
> That way the code here can be platform agnostic, and you don't touch the raw
> handle much.

The problem there is that by wrapping the handle in a base::Process, it gets
closed when the object is destructed (on windows). So even if it's dup'd and the
dup is terminated, the original handle is also closed regardless. I need to
avoid wrapping |data.handle| in base::Process to avoid it being closed.

[Lei Zhang, 2015-04-30 20:11:53]

On 2015/04/30 19:17:34, Anand Mistry wrote:
> On 2015/04/30 08:08:52, Lei Zhang wrote:
> >
>
https://codereview.chromium.org/1071453002/diff/60001/chrome/browser/image_de...
> > File chrome/browser/image_decoder_browsertest.cc (right):
> > 
> >
>
https://codereview.chromium.org/1071453002/diff/60001/chrome/browser/image_de...
> > chrome/browser/image_decoder_browsertest.cc:78: if
> > (!::DuplicateHandle(GetCurrentProcess(), handle,
> > I would:
> > - wrap |data.handle| in a base::Process object
> > -- check if it IsValid()
> > -- call dup = base::Process::Duplicate()
> > -- then dup.Terminate().
> > 
> > That way the code here can be platform agnostic, and you don't touch the raw
> > handle much.
> 
> The problem there is that by wrapping the handle in a base::Process, it gets
> closed when the object is destructed (on windows). So even if it's dup'd and
the
> dup is terminated, the original handle is also closed regardless. I need to
> avoid wrapping |data.handle| in base::Process to avoid it being closed.

Oh, sorry, I missed the win::ScopedHandle part. This is not ideal, but it'll
have to do until ChildProcessData cleans up its ProcessHandle usage.

[Lei Zhang, 2015-04-30 20:12:00]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2015-04-30 20:12:00]

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/1071453002/#ps60001
(title: "Maybe fix windows.")

[commit-bot: I haz the power, 2015-04-30 20:13:33]

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

[commit-bot: I haz the power, 2015-04-30 20:21:46]

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

[commit-bot: I haz the power, 2015-04-30 20:21:47]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_asan_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[Lei Zhang, 2015-04-30 20:27:02]

On 2015/04/30 20:21:47, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

Gotta rebase.

[Anand Mistry (off Chromium), 2015-04-30 22:31:48]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2015-04-30 22:31:49]

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/1071453002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-30 22:32:19]

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

[commit-bot: I haz the power, 2015-04-30 23:50:38]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-04-30 23:51:43]

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