840277 - Before we use the pre-allocated process, check that it's actually alive

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

We tried to do this in bug 838616, but didn't get it quite right.  I have a simple patch to fix this.

This needs to block, because it blocks bug 836638.  But the good news is, this patch plus my WIP patch for bug 836654 actually seems to fix bug 836638.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 712635 [details] [diff] [review]
Patch, v1

If we checked error returns from SetProcessPriority() on out, we wouldn't need this extra check.  Worth filing a followup on but I think this change is less risky.

>diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp

>+    bool crashed = false;
>+    base::DidProcessCrash(&crashed, mSubprocess->GetChildProcessHandle());
>+    if (crashed) {

This is a confusing use of this helper; it returns true if the process crashed and the outparam is set to true if the process *exited*.  We only care about the latter here.

So, s/crashed/exited/ here.

Comment 3

[Justin Lebar (not reading bugmail)]

Thanks; I should have read the docs more closely!

Now that I have, I also notice that it's illegal to call this function on Windows if we're not sure that the process has exited.  So I need to put all this in #ifndef XP_WIN.

Comment 4

[Alex Keybl [:akeybl]]

Blocks a blocker, blocking.

Comment 5

[Justin Lebar (not reading bugmail)]

This hasn't landed yet because I'm tracking down an apparent unrelated regression on trunk that's preventing me from testing these patches when rebased to tip.

Comment 6

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1cb87cc6b6a8

Comment 7

[Ed Morley [:emorley]]

Push & followups backed out for mochitest-2 and mochitest-other failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a74f08ec0e46

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=f48618e815d1

Comment 8

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1738e1adece

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d1738e1adece

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ef2edab2f168
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/e7479f012e25

Comment 11

[Cody Roesch [:croesch]]

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Comment 12

[Justin Lebar (not reading bugmail)]

(In reply to croesch from comment #11)
> Can you please provide steps to verify this fix - as we will blackbox test
> from the UI?

You can't.

Comment 13

[amiller]

No need to create a TC in Moztrap for this issue.