Standup some form of OOP iframes for graphics and layout testing

RESOLVED FIXED in Firefox 67

Status

()

defect
P3
normal
RESOLVED FIXED
6 months ago
2 days ago

People

(Reporter: rhunt, Assigned: rhunt)

Tracking

(Blocks 3 bugs)

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Fission Milestone:M1, firefox67 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(11 attachments)

62.55 KB, patch
Details | Diff | Splinter Review
39.34 KB, patch
Details | Diff | Splinter 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
(Assignee)

Description

6 months ago
I've written a (very hacky) patch that stands up remote iframes. Currently it just does graphics initialization and rendering and is very crashy. It doesn't use any of the nested content process machinery as that's not going to be used for fission.

In the short term, I plan on using this patch to experiment and fix things that will be broken with remote iframes.

In the long term, it'd be nice to get something like this (preffed off or whatever) in mozilla-central so more people can work with it.

The patch currently renders this test page without crashing [1]. Don't interact with the iframe or it'll crash. The iframe has a 'fission' attribute that opts-in to the special behavior. You will have to access the test case through file:// or http:// because my blog is served on S3 and doesn't use https yet like github pages does.

[1] https://eqrion.github.io/web-tests/embed/cross-origin-blog.html
Ryan, is this the most recent version of the patch? I was going to have a go at updating the patch for the big clang-format of the source, but then it turned out that there have been less trivial changes to the codebase, such as RenderFrameParent being no more.
Flags: needinfo?(rhunt)
(Assignee)

Comment 2

4 months ago
It’s the most recent complete patch, but it’s out of date for those changes. I haven’t updated the patch since then as the approach will need some reworking to actually be able to be landed.

I have a good idea of what needs to be done and plan on working on it when I get back from PTO next week and have scroll anchoring in a good spot. Feel free to steal this if it blocks you though.
Flags: needinfo?(rhunt)
(Assignee)

Comment 3

3 months ago

Here is the newly rebased and updated version. It's actually cleaner now because of the PWindowGlobal and RenderFrame changes. It did make the rebasing, non-trivial though.

:rhunt, any updates on the changes you wanted to apply after my changes in https://github.com/mystor/mozilla-central/tree/oopif?

It'd be awesome if we could get this initial version landed soon :-)

Flags: needinfo?(rhunt)
(Assignee)

Comment 5

3 months ago

I've taken that patch, fixed up some of the RenderFrame changes to be landable, split the patches, and fixed some failures on try. It should be ready for review now.

So now the current patches have some history. I originally wrote them a while ago, then rebased them, then Nika cleaned them up a bunch and added actors, and now I've tried to get them ready for review.

Here's the last try run I've done [1]. The one failure should be fixed in the latest patches, I've done a new try run here [2].

I can confirm that the patches work to load and render a remote iframe with the pref and fission attribute set on the iframe. There's still much work to be done, this is just a start.

Kats was the last one to review my RenderFrame related stuff, but is on parental leave. Andrew, is there any chance you could take a look?

Boris, I was also recommended by Nika to have you look at the bulk of it. If you don't have time though, we can find another reviewer.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=53b8411a4e07e79931c24bcb04509a07433f6c61
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=73b385cab8406597a1ac6304c3a1fe2c2052855c

Flags: needinfo?(rhunt)
(Assignee)

Comment 7

3 months ago

This commit adds a PRemoteFrame actor which is managed by PBrowser. It will
be created in a child process nsFrameLoader when loading a remote subframe.

This actor will mostly just bounce messages from a nsFrameLoader in the child
process to the actor in the parent process which will redirect the messages
to the TabParent of the remote subframe.

The piece in the parent actor to create the proxied PBrowser actors is
deferred to the next commit.

Depends on D17441

(Assignee)

Comment 8

3 months ago

This commit causes a new TabParent/TabChild to be created for a remote
subframe. The logic here is partially bogus and will need to be iterated
on.

Depends on D17442

(Assignee)

Comment 9

3 months ago

This commit hooks up nsFrameLoader in the child process to use the
PRemoteFrame protocol to support remote iframes. This is only activated
when a special pref is set, and the iframe has a marker attribute on it.

For example:
<iframe fission/>

In the future, we should unify nsFrameLoader to operate on a common
interface between the parent process top-level browser, and child
process subframe case. This commit just adds a new member that can
be used instead of mRemoteBrowser, when appropriate. IsRemoteFrame()
will return true for both cases.

Depends on D17443

(Assignee)

Comment 10

3 months ago

This commit hooks up the pieces of the PRemoteFrame protocol that
will proxy initialization, sizing, and display messages. The messages
chosen are just enough to start the frame and get an initial rendering.

Depends on D17444

(Assignee)

Comment 11

3 months ago

The documentation for these pieces are a bit out of date.

Depends on D17445

(Assignee)

Comment 12

3 months ago

A TabParent for a remote subframe will have the same owner content as the top-level
remote browser. This means that 'TabParent::GetFrameLoader()' will return the
frame loader of the top-level remote browser.

This is fine for getting the layer manager and compositor for connecting layer trees,
but the frame loader is also used to acquire a TabParent for its process ID. This
is incorrect in the remote subframe case, and will lead to the compositor rejecting
layer transactions for the remote subframe because it will only accept them from
the top-level remote browser's process.

This commit switches RenderFrame to just hold on to TabParent, and acquire the
nsFrameLoader as necessary.

Another change is to RenderFrame::SetOwnerContent. Previously this method would
take the new owner content and check an assertion. I don't see much value in the
assertion, so I've removed it. Additionally, now that we acquire the owner content,
and therefore the layer manager, from TabParent, we need to ensure that
RenderFrame::SetOwnerContent is ran after the TabParent has had it's owner content
updated. So the callsite has been moved into TabParent. This resolved a test failure
with frame loader swapping.

Depends on D17446

(Assignee)

Comment 13

3 months ago

This commit removes the dependency on RenderFrame from nsDisplayRemote so that
it can work in child processes with remote subframes. Instead nsDisplayRemote
now works with an nsFrameLoader, which will return the LayerId from either
the RenderFrame (for top-level remote browsers), or from RemoteFrameChild
(for remote subframes).

Depends on D17447

Blocks: 1523072

Updated

3 months ago
Fission Milestone: --- → M1
Duplicate of this bug: 1467229

Is it correct to conclude that this patch set conflicts with the recent IPC code generation changes? (I tried to apply the first couple of patches to the current trunk.)

Flags: needinfo?(rhunt)
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 17

2 months ago

It looks like there's some conflicts, yeah. Rebasing now.

Flags: needinfo?(rhunt)
Attachment #9038709 - Attachment description: Bug 1500257 part 1 - Fix unified build bustage. r?bzbarsky → Bug 1500257 part 1 - Fix unified build bustage. r?qdot
Attachment #9038710 - Attachment description: Bug 1500257 part 2 - Add PRemoteFrame stub implementation. r?bzbarsky → Bug 1500257 part 2 - Add PRemoteFrame stub implementation. r?qdot
Attachment #9038711 - Attachment description: Bug 1500257 part 3 - Create remote browser in parent process when initializing RemoteFrameParent. r?bzbarsky → Bug 1500257 part 3 - Create remote browser in parent process when initializing RemoteFrameParent. r?qdot
Attachment #9038712 - Attachment description: Bug 1500257 part 4 - Modify nsFrameLoader to create PRemoteFrame when enabled via pref and attribute. r?bzbarsky → Bug 1500257 part 4 - Modify nsFrameLoader to create PRemoteFrame when enabled via pref and attribute. r?qdot
Attachment #9038713 - Attachment description: Bug 1500257 part 5 - Implement messages for loading and displaying remote subframes on PRemoteFrame. r?bzbarsky → Bug 1500257 part 5 - Implement messages for loading and displaying remote subframes on PRemoteFrame. r?qdot
Attachment #9038717 - Attachment description: Bug 1500257 part 9 - Add basic tests for out-of-process iframes. r?bzbarsky → Bug 1500257 part 9 - Add basic tests for out-of-process iframes. r?qdot
(Assignee)

Comment 18

2 months ago

I've rebased and re-uploaded the patches.

Flags: needinfo?(bzbarsky)

(In reply to Ryan Hunt [:rhunt] from comment #18)

I've rebased and re-uploaded the patches.

Thanks.

Now the patches apply (except part 1, which is now obsolete), but the methods related to RemoteFrameParent/Child cause errors. The errors are around virtual/override, which changed recently.

What base revision do I need for the patches to build? (I fail to find a way to see this information on Phabricator.)

Flags: needinfo?(rhunt)
(Assignee)

Comment 20

2 months ago

(In reply to Henri Sivonen (:hsivonen) from comment #19)

(In reply to Ryan Hunt [:rhunt] from comment #18)

I've rebased and re-uploaded the patches.

Thanks.

Now the patches apply (except part 1, which is now obsolete), but the
methods related to RemoteFrameParent/Child cause errors. The errors are
around virtual/override, which changed recently.

What base revision do I need for the patches to build? (I fail to find a way
to see this information on Phabricator.)

Here's my latest try run [1]. You should be able to import those and/or get the base commit information.

I've resolved the first test failure that came up after rebasing (related to tighter eval restrictions and ChromeUtils.import changes), and discovered a new issue.

Webrender's picture caching is causing a test failure in 'test_force_oop_iframe.html'. To resolve this temporarily, I'm disabling the test when webrender is active.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bb1d2e1b1ec5d82277f9bb772380ca05a69e00a

Flags: needinfo?(rhunt)

Comment 21

2 months ago
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1615df089ad3
part 1 - Fix unified build bustage. r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/e72d9d31a8bc
part 2 - Add PRemoteFrame stub implementation. r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/002762a021c4
part 3 - Create remote browser in parent process when initializing RemoteFrameParent. r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/e33b9de7283e
part 4 - Modify nsFrameLoader to create PRemoteFrame when enabled via pref and attribute. r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/b68b732411e2
part 5 - Implement messages for loading and displaying remote subframes on PRemoteFrame. r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba62cc27c32f
part 6 - Update documentation for RenderFrame and nsDisplayRemote. r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c85fb68f2ed
part 7 - Modify RenderFrame to hold onto TabParent instead of nsFrameLoader. r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e1b5cd23412
part 8 - Remove dependency on RenderFrame from nsDisplayRemote. r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/335ddf6a213a
part 9 - Add basic tests for out-of-process iframes. r=qdot

Backed out 9 changesets (bug 1500257) for build bustage at /dom/TabParent.h on a CLOSED TREE

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

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&revision=335ddf6a213a17764262041f46a51605fc931276&selectedJob=227374516

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

Log snippet:

[task 2019-02-09T06:53:38.202Z] 06:53:38 INFO - netwerk/protocol/http/Unified_cpp_protocol_http0.i_o
[task 2019-02-09T06:53:38.202Z] 06:53:38 INFO - make[5]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/netwerk/protocol/http'
[task 2019-02-09T06:53:40.033Z] 06:53:40 INFO - make[5]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/netwerk/protocol/ftp'
[task 2019-02-09T06:53:40.037Z] 06:53:40 INFO - /builds/worker/workspace/build/src/clang/bin/clang++ -m32 -o Unified_cpp_netwerk_protocol_ftp0.i_o -c -I/builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -DNDEBUG=1 -DTRIMMED=1 -DOS_POSIX=1 -DOS_LINUX=1 -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/netwerk/protocol/ftp -I/builds/worker/workspace/build/src/obj-firefox/netwerk/protocol/ftp -I/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -I/builds/worker/workspace/build/src/ipc/chromium/src -I/builds/worker/workspace/build/src/ipc/glue -I/builds/worker/workspace/build/src/netwerk/base -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -Qunused-arguments -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++1z-compat -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=return-std-move -Wno-error=atomic-alignment -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -Wno-return-type-c-linkage -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fcrash-diagnostics-dir=/builds/worker/artifacts -march=pentium-m -msse -msse2 -mfpmath=sse -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O3 -fno-omit-frame-pointer -funwind-tables -Werror -Wno-error=shadow -fprofile-instr-generate -MD -MP -MF .deps/Unified_cpp_netwerk_protocol_ftp0.i_o.pp /builds/worker/workspace/build/src/obj-firefox/netwerk/protocol/ftp/Unified_cpp_netwerk_protocol_ftp0.cpp
[task 2019-02-09T06:53:40.037Z] 06:53:40 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/netwerk/protocol/ftp/Unified_cpp_netwerk_protocol_ftp0.cpp:11:
[task 2019-02-09T06:53:40.038Z] 06:53:40 INFO - In file included from /builds/worker/workspace/build/src/netwerk/protocol/ftp/FTPChannelParent.cpp:11:
[task 2019-02-09T06:53:40.039Z] 06:53:40 ERROR - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/TabParent.h:319:31: error: 'AllocPRemoteFrameParent' marked 'override' but does not override any member functions
[task 2019-02-09T06:53:40.039Z] 06:53:40 INFO - virtual PRemoteFrameParent* AllocPRemoteFrameParent(
[task 2019-02-09T06:53:40.040Z] 06:53:40 INFO - ^
[task 2019-02-09T06:53:40.041Z] 06:53:40 ERROR - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/TabParent.h:322:16: error: 'DeallocPRemoteFrameParent' marked 'override' but does not override any member functions
[task 2019-02-09T06:53:40.041Z] 06:53:40 INFO - virtual bool DeallocPRemoteFrameParent(PRemoteFrameParent* aActor) override;
[task 2019-02-09T06:53:40.042Z] 06:53:40 INFO - ^
[task 2019-02-09T06:53:40.042Z] 06:53:40 INFO - 2 errors generated.
[task 2019-02-09T06:53:40.042Z] 06:53:40 INFO - /builds/worker/workspace/build/src/config/rules.mk:812: recipe for target 'Unified_cpp_netwerk_protocol_ftp0.i_o' failed
[task 2019-02-09T06:53:40.043Z] 06:53:40 ERROR - make[5]: *** [Unified_cpp_netwerk_protocol_ftp0.i_o] Error 1
[task 2019-02-09T06:53:40.043Z] 06:53:40 INFO - make[5]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/netwerk/protocol/ftp'
[task 2019-02-09T06:53:40.043Z] 06:53:40 INFO - /builds/worker/workspace/build/src/config/recurse.mk:74: recipe for target 'netwerk/protocol/ftp/target' failed
[task 2019-02-09T06:53:40.043Z] 06:53:40 ERROR - make[4]: *** [netwerk/protocol/ftp/target] Error 2
[task 2019-02-09T06:53:40.044Z] 06:53:40 INFO - make[4]: *** Waiting for unfinished jobs....
[task 2019-02-09T06:53:40.047Z] 06:53:40 INFO - make[5]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/netwerk/protocol/http'
[task 2019-02-09T06:53:40.047Z] 06:53:40 INFO - netwerk/protocol/http/Unified_cpp_protocol_http1.i_o

Flags: needinfo?(rhunt)
(Assignee)

Comment 23

2 months ago

Ah, I assumed that I had rebased and fixed the issues that Henri had reported. I guess I hadn't.

Flags: needinfo?(rhunt)

Comment 24

2 months ago
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3abe61f5dc5c
part 1 - Fix unified build bustage. r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/12d7dba34733
part 2 - Add PRemoteFrame stub implementation. r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/f228f043e6cf
part 3 - Create remote browser in parent process when initializing RemoteFrameParent. r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/697a9159e0d2
part 4 - Modify nsFrameLoader to create PRemoteFrame when enabled via pref and attribute. r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/c19bc81c4f43
part 5 - Implement messages for loading and displaying remote subframes on PRemoteFrame. r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/caafb04b7dd4
part 6 - Update documentation for RenderFrame and nsDisplayRemote. r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/62ad54fb95eb
part 7 - Modify RenderFrame to hold onto TabParent instead of nsFrameLoader. r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/a62c02951031
part 8 - Remove dependency on RenderFrame from nsDisplayRemote. r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/6097503e6300
part 9 - Add basic tests for out-of-process iframes. r=qdot
(Assignee)

Comment 27

2 months ago

I can't seem to reproduce these failures locally. Here's a speculative patch that seems to fix some of the intermittent failures [1]. The 'test_browserElement_oop_FirstPaint.html' failure still seems to remain.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b4679ef2089fb2d27596c2e5d1777ad5130b181

Flags: needinfo?(rhunt)
(Assignee)

Comment 28

2 months ago

I figured out I just needed to disable e10s. The failure is due to us not looking for the layer manager in the document of the owner content when the owner content is not attached.

With this, the test failures should be resolved. Here's the latest try run to verify [1].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb3d83fa7b4a18b614bc47082084025dc1555d0b

Comment 29

2 months ago
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08617d9d95cc
part 1 - Fix unified build bustage. r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea9fcc2f47f3
part 2 - Add PRemoteFrame stub implementation. r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5d1d5f2c850
part 3 - Create remote browser in parent process when initializing RemoteFrameParent. r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cfed0a28a2c
part 4 - Modify nsFrameLoader to create PRemoteFrame when enabled via pref and attribute. r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e296f9ed857
part 5 - Implement messages for loading and displaying remote subframes on PRemoteFrame. r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa01a3a883af
part 6 - Update documentation for RenderFrame and nsDisplayRemote. r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d0e20cf0c2e
part 7 - Modify RenderFrame to hold onto TabParent instead of nsFrameLoader. r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/473bed49a2fd
part 8 - Remove dependency on RenderFrame from nsDisplayRemote. r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/409364e06a94
part 9 - Add basic tests for out-of-process iframes. r=qdot

Updated

26 days ago
Blocks: oop-frames
(Assignee)

Updated

5 days ago
No longer blocks: graphics-fission
(Assignee)

Updated

2 days ago
Summary: Standup some form of remote iframes for graphics and layout testing → Standup some form of OOP iframes for graphics and layout testing
You need to log in before you can comment on or make changes to this bug.