Closed Bug 838616 Opened 7 years ago Closed 7 years ago

Set the preallocated process's priority sooner after we decide to transform it into an app

Categories

(Core :: IPC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
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

Attachments

(2 files)

After we commit to using the preallocated process to host a real app, that process must not die.  So we therefore have to re-prioritize the preallocated process and then check that it's alive /before/ we commit to it.

Patches in a moment.
Previously, it was possible to return a dead preallocated process.  This
patch eliminates or at least significantly reduces the likelihood of
this race.
Attachment #710707 - Flags: review?(jones.chris.g)
Attachment #710706 - Flags: review?(jones.chris.g) → review+
Attachment #710707 - Flags: review?(jones.chris.g) → review+
Comment on attachment 710706 [details] [diff] [review]
Part 1: Boost preallocated process's priority sooner after we decide to transform it into an app process.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a

User impact if declined: It's possible that apps will be launched and then killed immediately, thus causing us to fail our acceptance tests.

Testing completed: Locally

Risk to taking this patch (and alternatives if risky): We could simply not take this change and accept the increased chance of not being able to load apps.  I have no way of telling how often we hit this race in practice.  I think these patches are unlikely to cause more problems than they solve, though.

String or UUID changes made by this patch: None.
Attachment #710706 - Flags: approval-mozilla-b2g18?
Attachment #710707 - Flags: approval-mozilla-b2g18?
https://hg.mozilla.org/mozilla-central/rev/b7b4cfecd9dc
https://hg.mozilla.org/mozilla-central/rev/4fb612f6279f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Comment on attachment 710706 [details] [diff] [review]
Part 1: Boost preallocated process's priority sooner after we decide to transform it into an app process.

unknown risk, let's take it if/when we need it.
Attachment #710706 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18-
Attachment #710707 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18-
Rejecting this patch for b2g18 puts my critical blockers at risk; see bug 838615 comment 15.  I hope you can accept this without much debate, because that also puts my blockers at risk.
Attachment #710706 - Flags: approval-mozilla-b2g18- → approval-mozilla-b2g18?
Attachment #710707 - Flags: approval-mozilla-b2g18- → approval-mozilla-b2g18?
FWIW I can now say with confidence that these patches are necessary to fix bug 836638.  I need to tweak them a tad, but I can do that in a separate bug, since this has already landed on m-c.

Please approve these; they block a blocker.
Approving since they block a blocker.
Blocks: 836638
blocking-b2g: --- → tef+
Comment on attachment 710706 [details] [diff] [review]
Part 1: Boost preallocated process's priority sooner after we decide to transform it into an app process.

If these land on mozilla-b2g18 before Wednesday's branching then the status-b2g18-v1.0.1 flag should match the status-b2g18 flag in being fixed.
Attachment #710706 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Attachment #710707 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Assignee: nobody → justin.lebar+bug
Depends on: 840277
Backed out for bustage.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/f80dde8f2352

https://tbpl.mozilla.org/php/getParsedLog.php?id=19669784&tree=Mozilla-B2g18

e:/builds/moz2_slave/m-b18-w32-00000000000000000000/build/dom/ipc/ContentParent.cpp(247) : error C2171: '!' : illegal on operands of type 'void'

e:/builds/moz2_slave/m-b18-w32-00000000000000000000/build/dom/ipc/ContentParent.cpp(247) : error C2451: conditional expression of type 'void' is illegal

        Expressions of type void cannot be converted to other types

e:/builds/moz2_slave/m-b18-w32-00000000000000000000/build/dom/ipc/ContentParent.cpp(535) : error C2556: 'bool mozilla::dom::ContentParent::TransformPreallocatedIntoApp(const nsAString_internal &,mozilla::dom::PContentParent::ChildPrivileges)' : overloaded function differs only by return type from 'void mozilla::dom::ContentParent::TransformPreallocatedIntoApp(const nsAString_internal &,mozilla::dom::PContentParent::ChildPrivileges)'

        e:\builds\moz2_slave\m-b18-w32-00000000000000000000\build\dom\ipc\ContentParent.h(188) : see declaration of 'mozilla::dom::ContentParent::TransformPreallocatedIntoApp'

e:/builds/moz2_slave/m-b18-w32-00000000000000000000/build/dom/ipc/ContentParent.cpp(535) : error C2371: 'mozilla::dom::ContentParent::TransformPreallocatedIntoApp' : redefinition; different basic types

        e:\builds\moz2_slave\m-b18-w32-00000000000000000000\build\dom\ipc\ContentParent.h(188) : see declaration of 'mozilla::dom::ContentParent::TransformPreallocatedIntoApp'
Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.
The issue here was that somehow in the merge TransformPreallocatedIntoApp didn't become a bool function in ContentParent.h.  I can fix this and re-land.
This should do the trick.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/24531174c225
https://hg.mozilla.org/releases/mozilla-b2g18/rev/1a13d7ec3f9d

I did /not/ push to b2g-v1.0.1.  This still needs to be pushed there.
Why is this tef+ if v1.0.0 is marked wontfix?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> Why is this tef+ if v1.0.0 is marked wontfix?

Nevermind, I see in dev-b2g that it's how things are being handled at this point. Time to change some queries...
You need to log in before you can comment on or make changes to this bug.