Standup some form of OOP iframes for graphics and layout testing
Categories
(Core :: Graphics, defect, P3)
Tracking
()
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 |
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
Updated•6 years ago
|
Assignee | ||
Comment 3•6 years 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.
Comment 4•6 years ago
|
||
: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 :-)
Assignee | ||
Comment 5•6 years 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
Assignee | ||
Comment 6•6 years ago
|
||
Assignee | ||
Comment 7•6 years 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•6 years 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•6 years 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•6 years 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•6 years ago
|
||
The documentation for these pieces are a bit out of date.
Depends on D17445
Assignee | ||
Comment 12•6 years 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•6 years 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
Assignee | ||
Comment 14•6 years ago
|
||
Depends on D17448
Updated•6 years ago
|
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.)
Updated•6 years ago
|
Assignee | ||
Comment 17•6 years ago
|
||
It looks like there's some conflicts, yeah. Rebasing now.
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 18•6 years ago
|
||
I've rebased and re-uploaded the patches.
Updated•6 years ago
|
(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.)
Assignee | ||
Comment 20•6 years 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
Comment 21•6 years ago
|
||
Comment 22•6 years ago
|
||
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 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
Assignee | ||
Comment 23•6 years ago
|
||
Ah, I assumed that I had rebased and fixed the issues that Henri had reported. I guess I hadn't.
Comment 24•6 years ago
|
||
Comment 25•6 years ago
|
||
Backed out for multiple failures e.g. test_browserElement_oop_FirstPaint.html
Failure logs: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=227457532&repo=mozilla-inbound&lineNumber=3572
and https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=227459383&repo=mozilla-inbound&lineNumber=58952
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/aba99f466d4433966dff01f5bd975eec35fe2a9b
Comment 26•6 years ago
|
||
Other failure logs:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=227457306&repo=mozilla-inbound&lineNumber=1690
and
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=227458213&repo=mozilla-inbound&lineNumber=1580
and
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=227457528&repo=mozilla-inbound&lineNumber=12491
Assignee | ||
Comment 27•6 years 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
Assignee | ||
Comment 28•6 years 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•6 years ago
|
||
Comment 30•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/08617d9d95cc
https://hg.mozilla.org/mozilla-central/rev/ea9fcc2f47f3
https://hg.mozilla.org/mozilla-central/rev/b5d1d5f2c850
https://hg.mozilla.org/mozilla-central/rev/5cfed0a28a2c
https://hg.mozilla.org/mozilla-central/rev/5e296f9ed857
https://hg.mozilla.org/mozilla-central/rev/fa01a3a883af
https://hg.mozilla.org/mozilla-central/rev/0d0e20cf0c2e
https://hg.mozilla.org/mozilla-central/rev/473bed49a2fd
https://hg.mozilla.org/mozilla-central/rev/409364e06a94
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Comment 31•6 years ago
|
||
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.
Description
•