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

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Justin Lebar (not reading bugmail)]

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.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Part 1: Boost preallocated process's priority sooner after we decide to transform it into an app process.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Part 2: Eliminate a process-priority race condition around preallocated process creation.

Previously, it was possible to return a dead preallocated process.  This
patch eliminates or at least significantly reduces the likelihood of
this race.

Comment 3

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4fb612f6279f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7b4cfecd9dc

Comment 4

[Justin Lebar (not reading bugmail)]

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.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b7b4cfecd9dc
https://hg.mozilla.org/mozilla-central/rev/4fb612f6279f

Comment 6

[Alex Keybl [:akeybl]]

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.

Comment 7

[Justin Lebar (not reading bugmail)]

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.

Comment 8

[Justin Lebar (not reading bugmail)]

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.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Approving since they block a blocker.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/50887bcad0f2
https://hg.mozilla.org/releases/mozilla-b2g18/rev/4b8996fb2ad7

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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'

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.

Comment 14

[Justin Lebar (not reading bugmail)]

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.

Comment 15

[Justin Lebar (not reading bugmail)]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Why is this tef+ if v1.0.0 is marked wontfix?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

(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...

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/87a16b416962
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/cef9fbc01fb3