Closed Bug 1127189 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: IPC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: ting, Assigned: ting)

References

Details

Attachments

(2 files, 7 obsolete files)

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.
Attached patch patch v1 (obsolete) — Splinter Review
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=702ad1c144c4
Assignee: nobody → tchou
Status: NEW → ASSIGNED
Not only remote-browser-shown, remote-browser-pending also has the same problem.
Summary: Set observers accordingly in ParticularProcessPriorityManager → Remove observers of remote-browser-* once the event is handled to avoid redundant processing
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #8556277 - Attachment is obsolete: true
Attachment #8556369 - Flags: review?(khuey)
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.
Attachment #8556369 - Flags: review?(khuey)
Blocks: AppStartup
No longer depends on: AppStartup
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.
(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
Try is green. Kyle, sorry to bother, but since you're away, whom should I ask for reviewing?
Flags: needinfo?(khuey)
Attachment #8556998 - Attachment description: patch v3 → part1: avoid redundant observers v3
Attachment #8558336 - Flags: feedback?(kchen)
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.
Attachment #8558336 - Flags: feedback?(kchen) → feedback+
See Also: → 960641
Attachment #8558466 - Flags: review?(fabrice)
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!
Attachment #8558466 - Flags: review?(fabrice) → review?(kchen)
I'll, be back on Monday, so ...
Flags: needinfo?(khuey)
Comment on attachment 8556998 [details] [diff] [review]
part1: avoid redundant observers v3

I'll just wait then.
Attachment #8556998 - Flags: review?(khuey)
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.
Attachment #8558466 - Flags: review?(kchen) → feedback+
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.
Attachment #8558466 - Attachment is obsolete: true
Attachment #8560370 - Flags: review?(kchen)
Attachment #8560370 - Flags: review?(kchen) → review+
(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 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.
Attachment #8556998 - Flags: review?(khuey) → review+
Rebased and added "r=khuey" to commit message.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) (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.
Attachment #8556998 - Attachment is obsolete: true
Rebased & added "r=kanru" to commit message.
Attachment #8560370 - Attachment is obsolete: true
Try results are in comment 6 and 17.
Keywords: checkin-needed
Blocks: 1110624
No longer blocks: AppStartup
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6401084&repo=mozilla-inbound
Flags: needinfo?(tchou)
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
Attachment #8561223 - Attachment is obsolete: true
Flags: needinfo?(tchou)
Duplicate of this bug: 1096782
Try looks good. Didn't ask for another review since it is a straightforward fix.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/20f6c0ee944d
https://hg.mozilla.org/mozilla-central/rev/7e57011701b9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.