Closed Bug 1500257 Opened 6 years ago Closed 6 years ago

Standup some form of OOP iframes for graphics and layout testing

Categories

(Core :: Graphics, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla67
Fission Milestone M1
Tracking Status
firefox67 --- fixed

People

(Reporter: rhunt, Assigned: rhunt)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [gfx-noted])

Attachments

(11 files)

(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
Attached patch iframe-fission-gfx.patch (deleted) — — Splinter Review
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)
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)
Attached patch iframe-fission-gfx-2019.patch (deleted) — — Splinter Review

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)

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)

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

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

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

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

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

Depends on D17445

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

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

Depends on D17448

Blocks: 1523072
Fission Milestone: --- → M1

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)

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

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)

(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)
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)

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

Flags: needinfo?(rhunt)
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

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)

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

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
Blocks: oop-frames
No longer blocks: graphics-fission
Summary: Standup some form of remote iframes for graphics and layout testing → Standup some form of OOP iframes for graphics and layout testing

A whitelist entry for using eval() with system privileges in chrometask.js, introduced by this patch, is going to be removed and replaced by a pref flip in the respective test file, see Bug 1547713.
This is the preferred way for exceptionally using eval() in test contexts. The permanent whitelist is supposed to be empty at some point.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: