Closed
Bug 1127189
Opened 6 years ago
Closed 6 years ago
Remove observers of remote-browser-* once the event is handled to avoid redundant processing
Categories
(Core :: IPC, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: ting, Assigned: ting)
References
Details
Attachments
(2 files, 7 obsolete files)
12.86 KB,
patch
|
Details | Diff | Splinter Review | |
4.94 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•6 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=702ad1c144c4
Assignee: nobody → tchou
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•6 years ago
|
||
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
Assignee | ||
Comment 3•6 years ago
|
||
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)
Assignee | ||
Comment 4•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Blocks: AppStartup
No longer depends on: AppStartup
Assignee | ||
Comment 5•6 years ago
|
||
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.
Assignee | ||
Comment 6•6 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b89bdff0ebc7
Attachment #8556369 -
Attachment is obsolete: true
Assignee | ||
Comment 7•6 years ago
|
||
(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
Assignee | ||
Comment 8•6 years ago
|
||
Try is green. Kyle, sorry to bother, but since you're away, whom should I ask for reviewing?
Flags: needinfo?(khuey)
Assignee | ||
Updated•6 years ago
|
Attachment #8556998 -
Attachment description: patch v3 → part1: avoid redundant observers v3
Assignee | ||
Comment 9•6 years ago
|
||
Attachment #8558336 -
Flags: feedback?(kchen)
Comment 10•6 years ago
|
||
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+
Assignee | ||
Comment 11•6 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb8088fa537a
Attachment #8558336 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8558466 -
Flags: review?(fabrice)
Comment 12•6 years ago
|
||
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)
Assignee | ||
Comment 14•6 years ago
|
||
Comment on attachment 8556998 [details] [diff] [review] part1: avoid redundant observers v3 I'll just wait then.
Attachment #8556998 -
Flags: review?(khuey)
Comment 15•6 years ago
|
||
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+
Assignee | ||
Comment 16•6 years ago
|
||
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)
Updated•6 years ago
|
Attachment #8560370 -
Flags: review?(kchen) → review+
Assignee | ||
Comment 17•6 years ago
|
||
(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+
Assignee | ||
Comment 19•6 years ago
|
||
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
Assignee | ||
Comment 20•6 years ago
|
||
Rebased & added "r=kanru" to commit message.
Attachment #8560370 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Comment 22•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d32ab2da8a6 https://hg.mozilla.org/integration/mozilla-inbound/rev/2f31942e18b5
Keywords: checkin-needed
Comment 23•6 years ago
|
||
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)
Comment 24•6 years ago
|
||
https://hg.mozilla.org/projects/cypress/rev/bde0d9575b8d https://hg.mozilla.org/projects/cypress/rev/869e9a0cfbc9
Assignee | ||
Comment 25•6 years ago
|
||
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)
Assignee | ||
Comment 27•6 years ago
|
||
Try looks good. Didn't ask for another review since it is a straightforward fix.
Keywords: checkin-needed
Comment 28•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/20f6c0ee944d https://hg.mozilla.org/integration/mozilla-inbound/rev/7e57011701b9
Keywords: checkin-needed
Comment 29•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/20f6c0ee944d https://hg.mozilla.org/mozilla-central/rev/7e57011701b9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•