1155547 - Wait Nuwa forking a process if preallocated process isn't present

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

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

Attachment: proposed patch

+++ This bug was initially created as a clone of Bug #1053634 +++

I discussed with Kanru. As the change is really large to make loading an frame asynchronous, and the complexity doesn't seen converge now, the risk of taking patches of 1053634 to 2.2 has increased. It have been already quite hard to just rebase the patches onto 2.2, and it could incur unexpected regression.

I've implemented short version that synchronously waits for nuwa forking a process when a process request comes after nuwa ready. I think we would need more time and complex work to make loading a frame become a real asynchronous task.

Comment 1

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

Attachment: Patch

with 8 lines of context.

Comment 2

[Ken Chang[:kenkai]]

According to comment 93 of bug 1053634, this bug is the simpler fix of a 2.2+ bug

Comment 3

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

Attachment: Patch

Frameloader can try several times before it actually get the process, adding a waiting list to make sure only one process per manifest url.

Comment 4

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

Attachment: Patch

Comment 5

[Mike Lee [:mlee]]

Hi Patrick,

I noticed your needinfo request to me but don't see a question indicating what info do you need from me. Please clarify.

Thanks,
Mike

Comment 6

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

(In reply to Mike Lee [:mlee] from comment #5)
> I noticed your needinfo request to me but don't see a question indicating
> what info do you need from me. Please clarify.

I just like to know if you have any thought about this as a quick fix of bug 1053634, since we had some email contact about that bug. :)

Comment 7

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

(In reply to Patrick Wang (Chih-Kai Wang) (:kk1fff) from comment #6)
> I just like to know if you have any thought about this as a quick fix of bug
> 1053634, since we had some email contact about that bug. :)
                    ^ I meant.. had some conversations on mail

Comment 8

[Michael Vines [:m1] [:evilmachines]]

Low(er) risk fix for v2.2 sounds great to me.  How will this compare to the previous approach in terms of memory and performance?   

Once this lands will be plugin-container process be used for anything in b2g?

Comment 9

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

(In reply to Michael Vines [:m1] [:evilmachines] from comment #8)
> Low(er) risk fix for v2.2 sounds great to me.  How will this compare to the
> previous approach in terms of memory and performance?   

The patch simply require all process being forked from Nuwa. If this landed, I'd see original one as an architectural refactor for making loading frame asynchronous.

Both original one and this don't wait for Nuwa ready. As bug 1053634 comment 52 states, index db becomes busy when an x-heavy workload pushed, and Nuwa needs several minutes to becoming ready, waiting for Nuwa doesn't seem feasible. Thus, before Nuwa ready, we just fork from B2G. But after Nuwa ready, both approaches make sure that all processes are forked from Nuwa. The difference is that the previous one do it asynchronously while this one just enter nested loop and wait. I think the memory consumption should be close, and the loading time increases only when no preallocated process present, which is within 5 seconds after a process launched.

Comment 10

[Mike Lee [:mlee]]

(In reply to Patrick Wang (Chih-Kai Wang) (:kk1fff) from comment #7)
> (In reply to Patrick Wang (Chih-Kai Wang) (:kk1fff) from comment #6)
> > I just like to know if you have any thought about this as a quick fix of bug
> > 1053634, since we had some email contact about that bug. :)
>                     ^ I meant.. had some conversations on mail

I agree with mvines comment 8 re: this lower risk patch.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8594717 [details] [diff] [review]
Patch

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

I'm afraid of this change, but I'm willing to try it.

::: dom/ipc/PreallocatedProcessManager.h
@@ +85,5 @@
>    static void MaybeForgetSpare(ContentParent* aContent);
>    static bool IsNuwaReady();
>    static void OnNuwaReady();
>    static bool PreallocatedProcessReady();
> +  static already_AddRefed<ContentParent> WaitNewProcess(const nsAString& aManifestURL);

This is nit-picking, but I would prefer to call this BlockForNewProcess.

Regardless, it should definitely have a "For" in there.

Comment 12

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

Attachment: Patch

Thanks, Kyle!

Fixing nit.

Comment 13

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d53d91a3fae7

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e91d718aa500 for xpcshell orange:

https://treeherder.mozilla.org/logviewer.html#?job_id=9371513&repo=mozilla-inbound

Comment 15

[Dylan Oliver [:doliver]]

Patrick, can you provide a status update here on the outlook for getting this patch re-landed?

Comment 16

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

I am going to move a patch that disables nuwa on xpcshell to here. B2G on try seems broken yesterday, thus I can't run the test cases. I'll try today.

Comment 17

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

Looks like disabling Nuwa on xpcshell doesn't help. oauth test timed out. I would like to see if Cervantes could help take a look..

Comment 18

[Michael Vines [:m1] [:evilmachines]]

Hey Patrick, can you please attach a v2.2 patch for this bug?  We'll pull this in to our build in parallel to the xpchell test debug.

Comment 19

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

Attachment: Patch on 2.2

Comment 20

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

I'll continue working on this.

Comment 21

[Greg Grisco]

We applied the patch but it is causing major instability.  cafbot will attach minidump and log info in a few minutes.  Please do not merge the change as-is, thanks!

Comment 22

[cafbot (PoC: ggrisco)]

Attachment: EXTRA file attachment -

Comment 23

[cafbot (PoC: ggrisco)]

Attachment: decoded minidump -

Comment 24

[cafbot (PoC: ggrisco)]

Attachment: EXTRA file attachment -

Comment 25

[cafbot (PoC: ggrisco)]

Attachment: decoded minidump -

Comment 26

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

Greg, is there any STR for the above crashes? Thanks.

Comment 27

[Greg Grisco]

(In reply to Cervantes Yu from comment #26)
> Greg, is there any STR for the above crashes? Thanks.

We found this in stability test, so no clear steps are available, although it is happening quite often.  We've seen it around 40 times in a relatively small number of test hours.

Comment 28

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

I also reproduced the crash. It turns out that TabParent::mFrameElement is destroyed and then is later used in referenced in TabParent::IsVisible() or TabParent::GetFrameLoader(). I guess the nested loop used in this patch reenters some code that is not reentrant. Need to investigate more.

Comment 29

[Jean Gong :jgong]

(In reply to Cervantes Yu from comment #28)
> I also reproduced the crash. It turns out that TabParent::mFrameElement is
> destroyed and then is later used in referenced in TabParent::IsVisible() or
> TabParent::GetFrameLoader(). I guess the nested loop used in this patch
> reenters some code that is not reentrant. Need to investigate more.
Cervantes, What is your status? Are you still investigating or are you working on a fix?

Comment 30

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

I am still trying to fix the crash or prove that the approach of using a nested loop is a dead end. I plan to have a conclusion by the end of this week.

Comment 31

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

It turns out that the assertion placed in nsFrameLoader::TryRemoteBrowser() is violated. 2 TabParents are created for the same nsFrameLoader. So the 2nd TabParent can reference the dangling pointer to the nsGenericHTMLFrameElement if the frame element is destroyed.

Comment 32

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

Attachment: Use a nested loop to wait until Nuwa completes forking the process.

Update to the previous patch: make it safe to reenter at places that may enter the nested loop.

A hard lesson for using the nested loop is that we must assume that the method invocation chain can repeat itself with the use of a nested loop. The crash happens because the nested loop is called inside a constructor-like method: ContentParent::CreateBrowserOrApp(). When the constructor-like method repeats itself, a frame loader creates 2 TabParent instances. When the frame element is destroyed, one of the 2 TabParent instances isn't unaware about it and holds a dangling weak reference. Later the TabParent instance crashes when dereferencing it.

The crash is fixed by making the call that might enter a nested loop explicit as ContentParent::BlockForNewProcess(). nsFrameLoader::TryRemoteBrowser() and ContentParent::RecvCreateChildProcess(). The frame loader also checks whether the remote browser is created deep inside the nested loop and skip the creation of another one if the nested loop already created it.

Comment 33

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

Try push log: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b168e51a9f31

Comment 34

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

Attachment: Patch on 2.2

Greg, would you initiate a round of stability test with this patch to see if there are still other crashes or quirks? Thanks.

Comment 35

[Greg Grisco]

>
> Greg, would you initiate a round of stability test with this patch to see if
> there are still other crashes or quirks? Thanks.

Hi Cervantes, we won't be able to bring this in now since we are trying to stabilize for major milestone.  Once you have stabilized this on master and our milestone has passed, we can bring it in.  Thanks.

Comment 36

[Dylan Oliver [:doliver]]

Kyle, have you had a chance to start looking at this review yet?

Comment 37

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8606234 [details] [diff] [review]
Use a nested loop to wait until Nuwa completes forking the process.

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

I was going to hold off on r+ing this until it got some testing, but whatever.  I still don't think we should check this in to branches without extensive testing.

Comment 38

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

From the push log, some tests seem to be permfail. I think there are still other issues with the use of nested loop. We need to consider another alternative.

Comment 39

[Josh Cheng [:josh]]

Hi Cervantes,
Do you have any update about the failed test for push?
Thanks!

Comment 40

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

(In reply to Josh Cheng [:josh] from comment #39)
> Hi Cervantes,
> Do you have any update about the failed test for push?
> Thanks!

I give up the approach in the original patch because the approach is too risky. I am working on another approach to fix this bug.

Comment 41

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

My plan B is moving the Nuwa operations to the background thread. When the frameloader requests a process but there is no spare one, the request will be processed using the background thread while the frameloader gets blocked (hopefully a short time). This is supposed to be much safer than using a nested loop.

Kyle, do you see any issue with this approach?

Comment 42

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

That sounds like a good approach to try.

This should be dropped from the 2.2 scope though.  There's nowhere near enough time to be confident in the stability of a change like that for 2.2.

Comment 43

[Ken Chang[:kenkai]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #42)
> That sounds like a good approach to try.
> 
> This should be dropped from the 2.2 scope though.  There's nowhere near
> enough time to be confident in the stability of a change like that for 2.2.

Agree with Kyle. And according to whiteboard, this is a bug with caf priority 3 for partner. It seems not a
very critical issue. Can we discuss with partner to remove it from CAF Blocker list?

Comment 44

[Dylan Oliver [:doliver]]

Confirmed that we will move this off the 2.2 list. Bumping to 3.0? to retain visibility.

Comment 45

[Dylan Oliver [:doliver]]

Removing dependency on CAF 2.2, per agreement with partner.

Comment 46

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

Attachment: Patch (WIP)

This is the rollup patch for the current WIP.

TODO:
* All fork requests are sync now. The sync fork request takes 60 to 90 ms on flame. We should only block when we need one ContentParent and there is no prellocated process. The request to fork a preallocated process should run async.

Comment 47

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

Attachment: Part 1: fix unified build bustage.

Comment 48

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

Attachment: Part 2: PNuwa protocol implementation

Comment 49

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

Try push log: https://treeherder.mozilla.org/#/jobs?repo=try&revision=02e9d66dd413

Comment 50

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

Attachment: Part 2: PNuwa protocol implementation

Changes: fix PreallocatedProcessManagerImpl::GetSpareProcess() and missing #include in NuwaChild.cpp.

Comment 51

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Am I expecting a new patch here?  Or is this the latest version?

Comment 52

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

This is the latest version, but I just found a conflict in updating m-c. I will rebase and fix a build break on Windows.

Comment 53

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

Attachment: Part 2: PNuwa protocol implementation

Fix build breaks on Windows and Linux.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be14c1ef2408

Comment 54

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8628126 [details] [diff] [review]
Part 2: PNuwa protocol implementation

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

r=me, looks good with comments addressed.

::: dom/ipc/ContentChild.cpp
@@ +532,5 @@
>      if (IsNuwaProcess()) {
>          return;
>      }
>  
> +

nit: extra /n

::: dom/ipc/ContentParent.h
@@ +938,5 @@
>  #endif
>  
>      PProcessHangMonitorParent* mHangMonitorActor;
> +
> +    // NuwaParent and ContentParent holds strong references to each other. The

hold

::: dom/ipc/NuwaChild.cpp
@@ +34,5 @@
> +#ifdef MOZ_NUWA_PROCESS
> +
> +namespace {
> +
> +// FIXME indentation: 2

I think you fixed this.

::: dom/ipc/NuwaParent.cpp
@@ +46,5 @@
> +  , mClonedActor(nullptr)
> +  , mWorkerThread(do_GetCurrentThread())
> +  , mNewProcessPid(0)
> +{
> +  MOZ_ASSERT(!NS_IsMainThread());

AssertIsOnBackgroundThread();

@@ +103,5 @@
> +  MOZ_ASSERT(runnable);
> +  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(mWorkerThread->Dispatch(runnable,
> +                                                       NS_DISPATCH_NORMAL)));
> +
> +  lock.Wait();

wait in a loop

@@ +114,5 @@
> +  // We can't call ActorConstructed() right after Alloc() in the above runnable.
> +  // To be safe we dispatch a runnable to the current thread to do it.
> +  runnable = NS_NewRunnableFunction([actor] () -> void
> +  {
> +    // On the main thread.

Add a MOZ_ASSERT(NS_IsMainThread()) in this one.

@@ +117,5 @@
> +  {
> +    // On the main thread.
> +    nsCOMPtr<nsIRunnable> nested = NS_NewRunnableFunction([actor] () -> void
> +    {
> +      // Call NuwaParent::ActorConstructed() on the worker thread.

And an AssertIsOnBackgroundThread() in this one.

@@ +150,5 @@
> +    contentParent->SetNuwaParent(nullptr);
> +    // Need to clear the ref to ContentParent on the main thread.
> +    self->mContentParent = nullptr;
> +
> +  });

nit: extra \n

@@ +166,5 @@
> +  }
> +
> +  nsCOMPtr<nsIRunnable> runnable =
> +    NS_NewNonOwningRunnableMethod(mContentParent.get(),
> +                                  &ContentParent::OnNuwaReady);

Add a comment about how NonOwning is safe because destroying the mContentParent ref would have to go to the main thread anyways.

@@ +236,5 @@
> +    return false;
> +  }
> +
> +  MonitorAutoLock lock(mMonitor);
> +  mBlocked =  true;

nit: only one space here

@@ +237,5 @@
> +  }
> +
> +  MonitorAutoLock lock(mMonitor);
> +  mBlocked =  true;
> +  lock.Wait();

wait on the condvar in a loop

::: dom/ipc/NuwaParent.h
@@ +17,5 @@
> +
> +class ContentParent;
> +
> +class NuwaParent: public mozilla::dom::PNuwaParent
> +{

nit: "NuwaParent :"

Comment 55

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=266541c6c167

Comment 56

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

Attachment: Part 1: fix unified build bustage. r=khuey

Rebased to latest and added/removed #include's accordingly.

Comment 57

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

Attachment: Part 2: PNuwa protocol implementation. r=khuey

Rebased and addressed review comments.

Comment 58

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

Kicked off another round of tests on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58aaebff90db

Comment 59

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

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/2bdcd74e503d782614c12a9fb013ad80e1784de2
changeset:  2bdcd74e503d782614c12a9fb013ad80e1784de2
user:       Cervantes Yu <[e-mail]>
date:       Fri Jul 31 15:25:14 2015 +0800
description:
Bug 1155547, Part 1: Fix unified build breakage in adding new sources under dom/ipc/. r=khuey

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/89d7e4e93bec246c56efe4225f5e816763e4ba5d
changeset:  89d7e4e93bec246c56efe4225f5e816763e4ba5d
user:       Cervantes Yu <[e-mail]>
date:       Fri Jul 31 15:25:27 2015 +0800
description:
Bug 1155547, Part 2: Create PNuwa protocol (managed by PBackground) for forking content processes. r=khuey

This allows us to send a sync fork request to the Nuwa process when we need one but there is no
spare process available. After an app is launched, the request to fork a spare process is still
handled asynchronously.

Comment 60

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2bdcd74e503d
https://hg.mozilla.org/mozilla-central/rev/89d7e4e93bec