Closed Bug 1525720 Opened 9 months ago Closed 5 months ago

Fission: Enable/disable rendering of OOP iframes when switching tabs

Categories

(Core :: Graphics, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla69
Fission Milestone M3
Tracking Status
firefox69 --- fixed

People

(Reporter: rhunt, Assigned: rhunt)

References

(Blocks 2 open bugs)

Details

Attachments

(18 files, 1 obsolete file)

34.34 KB, image/svg+xml
Details
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

When we switch to a tab with out of process iframes we should request layers/wr display lists from the root frame and all visible remote sub frames. Some changes to async tab switcher will also probably be needed.

Summary: Support tab switching with out of process iframes → Fission: Support tab switching with OOP iframes
Fission Milestone: --- → M2
Assignee: nobody → rhunt
Status: NEW → ASSIGNED
Priority: P3 → P2

I'm on PTO the next few days, so here's a WIP.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c72611bcf77302ce83745d29147cce3f6697c6e

There's some architectural work that has been delaying this. The core problem is that the frontend has direct access to the TabParent, via nsITabParent. It uses this for multiple things, but most importantly for this bug, it's used async tab switching. This works when we use PBrowser for 'a whole tab' but not for 'maximal contiguous group of local connected browsers'.

So I've renamed the interface to nsIRemoteTab, and added a new class BrowserController (temporary sad name) which implements this. This new class will be owned by nsFrameLoader and own the root of a PBrowser tree. It can then perform operations that need to affect 'a whole tab', like tab switching.

Additionally I've added a new interface, RemoteBrowser, which will be used to simplify nsFrameLoader, as it currently has two different code paths for PBrowser and PBrowserBridge. It's mostly a future refactoring goal.

The next steps for these patches are:

  1. Instantiate BrowserController, BrowserBridgeController in nsFrameLoader when creating PBrowser or PBrowserBridge. We might only hold a reference to these as 'RemoteBrowser' or in addition to the protocol actors.
  2. Return the BrowserController instead of the PBrowser when getting a nsIRemoteTab for a nsFrameLoader
  3. Modify the nsIRemoteTab methods on BrowserController to operate on the whole PBrowser tree as appropriate

With those three things, tab switching should just work. All OOP-IFs will be activated and deactivated when a tab is switched on and off.

After this, the next steps are to use this infrastructure for bug 1518917 and bug 1519546. The general plan is laid out in bug 1519546 comment 17. I'd like to add a hook in TabChild/BrowserChild for when we've submitted a layer tree or webrender display list. From this hook, we can tell which iframes are visible or not, their associated scale factors, and clipping. We can then transmit this to the parent process, which can apply it to the descendant remote browsers.

It was a complete pain (since a ton of stuff had conflicted in the last couple of weeks) but I rebased some of the patches. See bug 1534395 comment 5.

Blocks: rendering-fission
No longer blocks: graphics-fission
Blocks: 1519546
Duplicate of this bug: 1518911
No longer blocks: 1519546

Ryan has working patches but they need some work. Moving over to M3.

Fission Milestone: M2 → M3
Summary: Fission: Support tab switching with OOP iframes → Fission: Enable/disable rendering of OOP iframes when switching tabs
Attached image fission-ipc.svg

Alright, this turned into a lot more than I started out to do.

Tab switching (mainly) works by using the renderLayers and docShellIsActive properties on nsIRemoteTab. renderLayers will enable/disable rendering to the compositor for a remote browser. docShellIsActive corresponds with actually enabling/disabling the DOM of a remote browser.

The problem today is that nsIRemoteTab is implemented by BrowserParent, so these properties only affect the root BrowserParent, and not the BrowserParent's for OOP-iframes.

My current patchset fixes this by adding a new class (BrowserHost) to implement nsIRemoteTab that applies these properties to the whole PBrowser tree.

In the process of doing this, I also added a new internal interface called RemoteBrowser which abstracts away the differences of the chrome process and content process case for nsFrameLoader. This simplifies the implementation of nsFrameLoader quite a bit, but there are still special cases for the specific IPDL actors remaining.

Attached is an updated IPC diagram for the situation after these patches.

As always, naming of things is hard and I would appreciate suggestions for better names than BrowserHost or BrowserBridgeHost.

This reduces the amount of code that assumes that BrowserParent implements nsIRemoteTab.

Depends on D31429

This appears unused and adds unneeded surface area for these API's to support.

Depends on D31430

This API is only intended to be used in the chrome process and this commit
makes this explicit to simplify a later refactoring.

Depends on D31431

This code currently works for remote subframes assuming that nsIRemoteTab is implemented
by BrowserParent, but will break when nsIRemoteTab is only for a top-level BrowserParent.

What this code really wants is a content process ID to retarget the channel to. This
commit switches the interfaces to pass this around instead of nsIRemoteTab.

Depends on D31434

This makes it symmetrical to how BrowserParent is created by ContentParent.

Depends on D31435

This prepares nsFrameLoader for replacing mBrowserParent and mBrowserBridgeChild
with a common interface by making special case code use a getter method instead
of direct access.

Depends on D31436

RemoteBrowser is a common interface between the chrome/content process cases for nsFrameLoader,
that allows us to abstract IPC details away. BrowserHost is a concrete implementation for
the chrome process, while BrowserBridgeHost implements the content process case.

Depends on D31437

This commit implements the RemoteBrowser interface for BrowserHost and BrowserBridgeHost.

For BrowserHost, most methods delegate to the root BrowserParent. In the future, we
should move these over to BrowserHost. For BrowserBridgeHost, most methods are taken
from BrowserBridgeParent.

Depends on D31438

This commit adds a link from BrowserParent to it's owning BrowserHost
if it is the root BrowserParent.

Depends on D31439

This commit replaces the direct use of the IPDL actors in nsFrameLoader with
the RemoteBrowser interface. Some special use cases are adapted to still use
the IPDL actors. In the future, we should burn these use cases down.

Depends on D31441

This commit implements nsIRemoteTab in BrowserHost by delegating to nsIRemoteTab. In a
future commit, these methods will be implemented by BrowserHost.

Depends on D31442

This commit removes nsIRemoteTab as a parent class from BrowserParent,
so that BrowserHost is the only concrete implementation of nsIRemoteTab.

Some static_cast's are updated to cast to BrowserHost, and other places
have to be updated to pass a BrowserHost instead of a BrowserParent.

WindowGlobalParent had a getter to return it's managing BrowserParent
as a nsIRemoteTab. I couldn't find a use of this in-tree, so I've just
opt-ed to remove it. If there's a use-case, we can add something back
in.

Depends on D31443

This commit moves the actual implementation of nsIRemoteTab from BrowserParent
to BrowserHost, without any functional changes.

Depends on D31444

This commit finally updates some nsIRemoteTab methods to apply
to the whole tree of BrowserParent's.

Depends on D31445

BrowserParent is cycle collected and supported weak references, so this commit adds support
for these things to BrowserHost.

Depends on D31447

It's possible for front-end references to nsIRemoteTab to outlive the IPDL actor. When
this happens, we should ignore methods and property accesses.

The one special case is that some code expects to be able to
access the TabId after the browser has been destroyed. For this
we can just cache the ID.

Depends on D31448

This commit was added to fix an intermittent issue where the BrowserParent
would not be able to find the original window root while being destroyed
to unregister itself. I couldn't figure out how that would be related
to these commits, so I decided to make this process more robust.

Depends on D31449

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9260bbdf12a5fae42dbc8b35df542fab6bd54b81

There's a crasher that's Windows only. Looking into it now. The other platforms are green.

Attachment #9065396 - Attachment description: Bug 1525720, part 16 - Ignore nsIRemoteTab methods after we have destroyed the browser. r?nika → Bug 1525720, part 17 - Ignore nsIRemoteTab methods after we have destroyed the browser. r?nika
Attachment #9065397 - Attachment description: Bug 1525720, part 17 - Hold current window root for registering nsIRemoteTab. r?smaug → Bug 1525720, part 18 - Hold current window root for registering nsIRemoteTab. r?smaug
Attachment #9065397 - Attachment is obsolete: true
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00d83f1d02e0
part 1 - Allow calling BrowserParent::InitRendering multiple times, and remove RenderFrame dependency from nsFrameLoader. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f1c1b609ec1
part 2 - Move UITabResolutionChanged method to nsIRemoteTab interface from BrowserParent. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a255864f75d
part 3 - Remove method to createRemoteFrameLoader from nsIMozBrowserFrame interface. r=farre
https://hg.mozilla.org/integration/mozilla-inbound/rev/b255e0a84e12
part 4 - Assert in DrawSnapshot if not the chrome process. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/757b4f595cc4
part 5 - Redirect nsIHttpChannel using content process ID instead of nsIRemoteTab. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/07678a2fa9e7
part 6 - Move BrowserBridgeChild creation to ContentChild. r=farre
https://hg.mozilla.org/integration/mozilla-inbound/rev/32eeee79d628
part 7 - Use getter methods to access mBrowserParent and mBrowserBridgeChild. r=farre
https://hg.mozilla.org/integration/mozilla-inbound/rev/eadeacbe4483
part 8 - Add RemoteBrowser interface and BrowserHost/BrowserBridgeHost implementations. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/697774dd8984
part 9 - Fill out implementations of BrowserHost and BrowserBridgeHost. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3b2ac8d5ca4
part 10 - Link BrowserParent and BrowserHost on creation. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bcf9f586c55
part 11 - Use RemoteBrowser interface instead of IPDL actors in nsFrameLoader. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee10a8254481
part 12 - Make BrowserHost implement nsIRemoteTab by delegating to nsIRemoteTab. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/810b73719871
part 13 - Stop inheriting nsIRemoteTab interface in BrowserParent. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/d25963c72ff7
part 14 - Move final bits of nsIRemoteTab implementation to BrowserHost. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/99f971a02d87
part 15 - Apply appropriate nsIRemoteTab methods to all BrowserParents. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/e65cb2d4c5a5
part 16 - Cycle collect RemoteBrowser and support weak references in BrowserHost. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b79caa460a0
part 17 - Ignore nsIRemoteTab methods after we have destroyed the browser. r=nika
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f2e86c2d691
Try to fix build bustage. r=me on a CLOSED TREE
Backout by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f287bb6c1894
Backed out 18 changesets for mass failures on Windows platform e.g ProcessPriorityManager.cpp on a CLOSED TREE.

Backed out 18 changesets (bug 1525720) for mass failures on Windows platform e.g ProcessPriorityManager.cpp on a CLOSED TREE.

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/f287bb6c18942c75aca41d54987006951270628d

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&group_state=expanded&revision=9b79caa460a01a7bdf9c27ede487de0ec642ae0b&selectedJob=247831441

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=247831441&repo=mozilla-inbound&lineNumber=1471

Log snippet:

21:23:32 INFO - GECKO(5992) | AddressSanitizer can not provide additional info.
21:23:32 INFO - GECKO(5992) | SUMMARY: AddressSanitizer: access-violation z:\build\build\src\dom\ipc\ProcessPriorityManager.cpp:624 in `anonymous namespace'::ParticularProcessPriorityManager::Observe
21:23:32 INFO - GECKO(5992) | ==7896==ABORTING
21:23:32 INFO - GECKO(5992) | Exiting due to channel error.
21:23:32 INFO - GECKO(5992) | Exiting due to channel error.
21:26:26 INFO - runtests.py | Waiting for browser...
21:26:26 INFO - TEST-INFO | Main app process: exit 1
21:26:26 INFO - Buffered messages finished
21:26:26 ERROR - TEST-UNEXPECTED-FAIL | automation.py | application terminated with exit code 1
21:26:26 INFO - runtests.py | Application ran for: 0:03:00.346000
21:26:26 INFO - zombiecheck | Reading PID log: c:\users\task_1558552994\appdata\local\temp\tmpfved9fpidlog
21:26:26 INFO - ==> process 7896 launched child process 3992 ("Z:\task_1558552994\build\application\firefox\firefox.exe" -contentproc --channel="7896.0.475452804\1372410691" -parentBuildID 20190522200212 -greomni "Z:\task_1558552994\build\application\firefox\omni.ja" -appomni "Z:\task_1558552994\build\application\firefox\browser\omni.ja" -appdir "Z:\task_1558552994\build\application\firefox\browser" - 7896 gpu)
21:26:26 INFO - ==> process 7896 launched child process 4824 ("Z:\task_1558552994\build\application\firefox\firefox.exe" -contentproc --channel="7896.6.965983313\809637581" -childID 1 -isForBrowser -prefsHandle 2792 -prefMapHandle 2788 -prefsLen 1 -prefMapSize 199352 -parentBuildID 20190522200212 -greomni "Z:\task_1558552994\build\application\firefox\omni.ja" -appomni "Z:\task_1558552994\build\application\firefox\browser\omni.ja" -appdir "Z:\task_1558552994\build\application\firefox\browser" - 7896 tab)
21:26:26 INFO - ==> process 7896 launched child process 1840 ("Z:\task_1558552994\build\application\firefox\firefox.exe" -contentproc --channel="7896.13.60588587\767685795" -childID 2 -isForBrowser -prefsHandle 2968 -prefMapHandle 2964 -prefsLen 98 -prefMapSize 199352 -parentBuildID 20190522200212 -greomni "Z:\task_1558552994\build\application\firefox\omni.ja" -appomni "Z:\task_1558552994\build\application\firefox\browser\omni.ja" -appdir "Z:\task_1558552994\build\application\firefox\browser" - 7896 tab)
21:26:26 INFO - ==> process 7896 launched child process 5784 ("Z:\task_1558552994\build\application\firefox\firefox.exe" -contentproc --channel="7896.20.2044118451\1824183375" -childID 3 -isForBrowser -prefsHandle 3248 -prefMapHandle 3252 -prefsLen 159 -prefMapSize 199352 -parentBuildID 20190522200212 -greomni "Z:\task_1558552994\build\application\firefox\omni.ja" -appomni "Z:\task_1558552994\build\application\firefox\browser\omni.ja" -appdir "Z:\task_1558552994\build\application\firefox\browser" - 7896 tab)
21:26:26 INFO - zombiecheck | Checking for orphan process with PID: 3992
21:26:26 INFO - zombiecheck | Checking for orphan process with PID: 4824
21:26:26 INFO - zombiecheck | Checking for orphan process with PID: 5784
21:26:26 INFO - zombiecheck | Checking for orphan process with PID: 1840

Flags: needinfo?(rhunt)

The issue was in ProcessPriorityManager expecting certain properties to work on BrowserHost after destruction. Here's an all platform all tests run with the fix.

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

Flags: needinfo?(rhunt)
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0a4aecf98b0
part 1 - Allow calling BrowserParent::InitRendering multiple times, and remove RenderFrame dependency from nsFrameLoader. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/37fb118dac6c
part 2 - Move UITabResolutionChanged method to nsIRemoteTab interface from BrowserParent. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/707a48ef2338
part 3 - Remove method to createRemoteFrameLoader from nsIMozBrowserFrame interface. r=farre
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ff1726275aa
part 4 - Assert in DrawSnapshot if not the chrome process. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/420f10c25da1
part 5 - Redirect nsIHttpChannel using content process ID instead of nsIRemoteTab. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/21985ab71b5b
part 6 - Move BrowserBridgeChild creation to ContentChild. r=farre
https://hg.mozilla.org/integration/mozilla-inbound/rev/1332810e2604
part 7 - Use getter methods to access mBrowserParent and mBrowserBridgeChild. r=farre
https://hg.mozilla.org/integration/mozilla-inbound/rev/63dea5419ea4
part 8 - Add RemoteBrowser interface and BrowserHost/BrowserBridgeHost implementations. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c53638d28a9
part 9 - Fill out implementations of BrowserHost and BrowserBridgeHost. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d7151b26a9e
part 10 - Link BrowserParent and BrowserHost on creation. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cf9a1ce9e92
part 11 - Use RemoteBrowser interface instead of IPDL actors in nsFrameLoader. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/50475885250c
part 12 - Make BrowserHost implement nsIRemoteTab by delegating to nsIRemoteTab. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/a902acfbc868
part 13 - Stop inheriting nsIRemoteTab interface in BrowserParent. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ceff9307d51
part 14 - Move final bits of nsIRemoteTab implementation to BrowserHost. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/53663cad8890
part 15 - Apply appropriate nsIRemoteTab methods to all BrowserParents. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/1663719e2aa5
part 16 - Cycle collect RemoteBrowser and support weak references in BrowserHost. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4a9b4dd03ca
part 17 - Ignore nsIRemoteTab methods after we have destroyed the browser. r=nika

Our Thunderbird build is busted with
3:46.09 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\ipc/IPCMessageUtils.h(10,10): fatal error: 'base/process_util.h' file not found
3:46.09 #include "base/process_util.h"

That stuff seems to be included from dom/ipc/RemoteBrowser.h line 10.
EDIT: ... and we include nsFrameLoader.h which not includes this file.

Flags: needinfo?(rhunt)
Flags: needinfo?(nika)

Looks line we need to do something like this now:
https://searchfox.org/comm-central/rev/ee05c0d9006626f95dfbb1b6a79bed84e2b90e98/mozilla/dom/ipc/moz.build#145
include('/ipc/chromium/chromium-config.mozbuild')

A variation on this for C-C gets me further, but we run into other trouble now.

Consider it done, bug 1553944.

Flags: needinfo?(rhunt)
Flags: needinfo?(nika)
Regressions: 1554073
Regressions: 1554474
You need to log in before you can comment on or make changes to this bug.