1127189 - Remove observers of remote-browser-* once the event is handled to avoid redundant processing

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Ting-Yu Chou [:ting] (away)]

As Nuwa's priority can't be changed, it is a waste to setup observers for it. Also, the 'remote-browser-shown' observer should be removed once it is received, so we don't have each live content process to receive it while launching an app.

Comment 1

[Ting-Yu Chou [:ting] (away)]

Attachment: patch v1

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=702ad1c144c4

Comment 2

[Ting-Yu Chou [:ting] (away)]

Not only remote-browser-shown, remote-browser-pending also has the same problem.

Comment 3

[Ting-Yu Chou [:ting] (away)]

Attachment: patch v2

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=14c1da7699d0

The patch does two things: 1) remove remote-brower-* observers once the message is handled, 2) do not new a ParticularProcessPriorityManager for Nuwa since its priority is not allowed to be changed.

Comment 4

[Ting-Yu Chou [:ting] (away)]

Comment on attachment 8556369 [details] [diff] [review]
patch v2

Try is not good, the test case removes the iframe from DOM and adds it back, which in that case. I'll check if this means the observers should not be removed.

Comment 5

[Ting-Yu Chou [:ting] (away)]

Unbind iframe from DOM tree will destroy its frame loader. After adding it back, a new frame loader will be created and another browser element API initialization will be done by nsBrowserElement::BrowserShownObserver.

Comment 6

[Ting-Yu Chou [:ting] (away)]

Attachment: part1: avoid redundant observers v3

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

Comment 7

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #6)
> Created attachment 8556998 [details] [diff] [review]
> patch v3
> 
> Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b89bdff0ebc7

I forgot to clear the customized mozharness setting, here's another Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5747959ae3c7

Comment 8

[Ting-Yu Chou [:ting] (away)]

Try is green. Kyle, sorry to bother, but since you're away, whom should I ask for reviewing?

Comment 9

[Ting-Yu Chou [:ting] (away)]

Attachment: part2: get rid of remote-broser-pending

Comment 10

[Kan-Ru Chen [:kanru] (UTC+8)]

Comment on attachment 8558336 [details] [diff] [review]
part2: get rid of remote-broser-pending

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

::: dom/base/nsFrameLoader.cpp
@@ +362,5 @@
>    if (mRemoteFrame) {
>      if (!mRemoteBrowser) {
> +      if (!mBrowserAPIInitialized) {
> +        static_cast<nsBrowserElement*>(static_cast<nsGenericHTMLFrameElement*>(mOwnerContent))->InitBrowserElementAPI();
> +        mBrowserAPIInitialized = true;

You could query the nsIMozBrowserFrame interface.

Comment 11

[Ting-Yu Chou [:ting] (away)]

Attachment: part2: get rid of remote-broser-pending v2

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

Comment 12

[[:fabrice] Fabrice Desré]

Comment on attachment 8558466 [details] [diff] [review]
part2: get rid of remote-broser-pending v2

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

I'll let this one to Kanru!

Comment 13

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

I'll, be back on Monday, so ...

Comment 14

[Ting-Yu Chou [:ting] (away)]

Comment on attachment 8556998 [details] [diff] [review]
part1: avoid redundant observers v3

I'll just wait then.

Comment 15

[Kan-Ru Chen [:kanru] (UTC+8)]

Comment on attachment 8558466 [details] [diff] [review]
part2: get rid of remote-broser-pending v2

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

::: dom/base/nsFrameLoader.cpp
@@ +2733,5 @@
> +  nsCOMPtr<nsIMozBrowserFrame> browserFrame = do_QueryInterface(mOwnerContent);
> +  if (browserFrame) {
> +    static_cast<nsBrowserElement*>(static_cast<nsGenericHTMLFrameElement*>(
> +      mOwnerContent))->InitBrowserElementAPI();
> +    mBrowserAPIInitialized = true;

You could add a InitializeBrowserAPI() method to the nsIMozBrowserFrame interface to avoid this ugly casting.

Comment 16

[Ting-Yu Chou [:ting] (away)]

Attachment: part2: get rid of nsBrowserElement observers v3

Diff to v2:
- Added InitializeBrowserAPI() to nsIMozBrowserFrame.
- Removed the remote-browser-pending in nsFrameLoader::ReallyStartLoadingInternal(). It was added by bug 960641 since at that time ReallyStartLoadingInternal() can return before remote-browser-shown, but it is no longer the case.
- Removed mBrowserAPIInitialized flag since remote-browser-shown/inprocess-browser-shown can only be done once for a nsFrameLoader.

Comment 17

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #16)
> Created attachment 8560370 [details] [diff] [review]
> part2: get rid of nsBrowserElement observers v3

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=176a476a9051

Comment 18

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

Comment on attachment 8556998 [details] [diff] [review]
part1: avoid redundant observers v3

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

::: dom/ipc/ProcessPriorityManager.cpp
@@ +502,5 @@
>    nsRefPtr<ParticularProcessPriorityManager> pppm =
>      GetParticularProcessPriorityManager(aContentParent);
> +  if (pppm) {
> +    pppm->SetPriorityNow(aPriority, aBackgroundLRU);
> +  }

I'm slightly concerned that there will be other places that need a null check, but I guess if that happens those will be easy to fix.

@@ +833,5 @@
> +
> +  nsCOMPtr<nsIObserverService> os = services::GetObserverService();
> +  if (os) {
> +    os->RemoveObserver(this, "remote-browser-shown");
> +  }

If we're confident this always gets called, we could change it from a weak observer to a strong observer, to avoid the overhead of QIing through nsIWeakReference and whatnot.

Comment 19

[Ting-Yu Chou [:ting] (away)]

Attachment: part1: avoid redundant observers v4

Rebased and added "r=khuey" to commit message.

(In reply to Kyle Huey [:khuey] ([e-mail]) (Away 1/22-2/8) from comment #18)
> Comment on attachment 8556998 [details] [diff] [review]
> part1: avoid redundant observers v3
> 
> Review of attachment 8556998 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/ProcessPriorityManager.cpp
> @@ +502,5 @@
> >    nsRefPtr<ParticularProcessPriorityManager> pppm =
> >      GetParticularProcessPriorityManager(aContentParent);
> > +  if (pppm) {
> > +    pppm->SetPriorityNow(aPriority, aBackgroundLRU);
> > +  }
> 
> I'm slightly concerned that there will be other places that need a null
> check, but I guess if that happens those will be easy to fix.

Just double checked the places calling GetParticularProcessPriorityManager(), only this one needs a null check.

> 
> @@ +833,5 @@
> > +
> > +  nsCOMPtr<nsIObserverService> os = services::GetObserverService();
> > +  if (os) {
> > +    os->RemoveObserver(this, "remote-browser-shown");
> > +  }
> 
> If we're confident this always gets called, we could change it from a weak
> observer to a strong observer, to avoid the overhead of QIing through
> nsIWeakReference and whatnot.

Thanks for tellming me this, I didn't know there's an overhead. Since I can't really sure 'remote-browser-shown' will always be notified (for example, when the content process gets killed by OOM killer before it is shown), I'd like to keep it as a weak observer.

Comment 20

[Ting-Yu Chou [:ting] (away)]

Attachment: part2: get rid of nsBrowserElement observers v4

Rebased & added "r=kanru" to commit message.

Comment 21

[Ting-Yu Chou [:ting] (away)]

Try results are in comment 6 and 17.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d32ab2da8a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f31942e18b5

Comment 23

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6401084&repo=mozilla-inbound

Comment 24

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/cypress/rev/bde0d9575b8d
https://hg.mozilla.org/projects/cypress/rev/869e9a0cfbc9

Comment 25

[Ting-Yu Chou [:ting] (away)]

Attachment: part1: avoid redundant observers v5

My bad, I should try debug builds.

Removed the MOZ_ASSERT when receive ipc:content-shutdown, and double checked the code reference to mParticularManagers.

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

Comment 27

[Ting-Yu Chou [:ting] (away)]

Try looks good. Didn't ask for another review since it is a straightforward fix.

Comment 28

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/20f6c0ee944d
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e57011701b9

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/20f6c0ee944d
https://hg.mozilla.org/mozilla-central/rev/7e57011701b9