928995 - Let PreallocatedProcessManager manage Nuwa process.

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Most of logic of nuwa is implemented in ContentParent, we should move some of them into PreallocateProcessManager to let PreallocateProcessManager control nuwa process.

Comment 1

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Proposed Patch

Moving Nuwa process logic to PreallocatedProcessManager, use PreallocatedProcessManager to control the process.

Comment 2

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Proposed Patch

Comment 3

[Cervantes Yu [:cyu] [:cervantes]]

Comment on attachment 823989 [details] [diff] [review]
Proposed Patch

Review of attachment 823989 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for taking so long. I took a quick glance, and generally, the patch looks good. Some suggestions:

- I don't think we need PreallocatedProcessManager::ScheduleDelayedNuwaFork(). This should be encapsulated in AllocAfterDelay().
- We don't need to #include Nuwa.h in ContentParent or PreallocatedProcessManager.

Comment 4

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Proposed Patch

Thanks, Cervantes! This is updated version.

Comment 5

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Comment on attachment 828529 [details] [diff] [review]
Proposed Patch

Hi Ben, the propurse of this patch is to move Nuwa logic to preallocated process manager, since Nuwa is a preallocated process, and this would make 'dom.ipc.processPrelaunch.enabled' preference able to control Nuwa, which is necessary in Nuwa test case. Would you review this patch?  Thanks!

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 828529 [details] [diff] [review]
Proposed Patch

Review of attachment 828529 [details] [diff] [review]:
-----------------------------------------------------------------

It's not clear from the patch if anything has really changed in the code that was moved from ContentParent to PreallocatedProcessManager... Has anything changed? Or was it simply moved?

Comment 7

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

It is just moving code. The only change is to allow us to enable or disable Nuwa process by controlling value of 'dom.ipc.processPrelaunch.enabled'.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 828529 [details] [diff] [review]
Proposed Patch

Review of attachment 828529 [details] [diff] [review]:
-----------------------------------------------------------------

Ok!

Comment 9

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Thanks, Ben!

Comment 10

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Try looks good.

https://tbpl.mozilla.org/?tree=Try&rev=8bef2aa6d013

Comment 11

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch v.check-in r=bent

Comment 12

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

https://hg.mozilla.org/integration/b2g-inbound/rev/5cbb679dda59

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5cbb679dda59