Closed Bug 840277 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set

Tracking

(blocking-b2g:tef+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

RESOLVED FIXED
B2G C4 (2jan on)
blocking-b2g tef+
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

(Whiteboard: QARegressExclude, [qa-])

Attachments

(1 file)

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.
Assignee: nobody → justin.lebar+bug
Blocks: 836638
Attached patch Patch, v1Splinter Review
Attachment #712635 - Flags: review?(jones.chris.g)
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.
Attachment #712635 - Flags: review?(jones.chris.g) → review+
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.
Blocks a blocker, blocking.
blocking-b2g: tef? → tef+
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.
https://hg.mozilla.org/mozilla-central/rev/d1738e1adece
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Can you please provide steps to verify this fix - as we will blackbox test from the UI?
(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.
Whiteboard: QARegressExclude
No need to create a TC in Moztrap for this issue.
Flags: in-moztrap-
Whiteboard: QARegressExclude → QARegressExclude, [qa-]
You need to log in before you can comment on or make changes to this bug.