Closed
Bug 840277
Opened 12 years ago
Closed 12 years ago
Before we use the pre-allocated process, check that it's actually alive
Categories
(Firefox OS Graveyard :: General, defect)
Firefox OS Graveyard
General
Tracking
(blocking-b2g:tef+, 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)
(deleted),
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•12 years ago
|
Assignee: nobody → justin.lebar+bug
Assignee | ||
Comment 1•12 years ago
|
||
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+
Assignee | ||
Comment 3•12 years ago
|
||
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.
Assignee | ||
Comment 5•12 years ago
|
||
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.
Assignee | ||
Comment 6•12 years ago
|
||
Comment 7•12 years ago
|
||
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
Assignee | ||
Comment 8•12 years ago
|
||
Comment 9•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 10•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/ef2edab2f168
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/e7479f012e25
status-b2g18:
--- → fixed
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → fixed
status-firefox19:
--- → wontfix
status-firefox20:
--- → wontfix
status-firefox21:
--- → fixed
Target Milestone: --- → B2G C4 (2jan on)
Comment 11•12 years ago
|
||
Can you please provide steps to verify this fix - as we will blackbox test from the UI?
Assignee | ||
Comment 12•12 years ago
|
||
(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.
Updated•12 years ago
|
Whiteboard: QARegressExclude
Updated•12 years ago
|
Whiteboard: QARegressExclude → QARegressExclude, [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•