Closed
Bug 1475139
Opened 6 years ago
Closed 6 years ago
Support rendering OOP iframes with new drawSnapshot API in parent process
Categories
(Core :: Web Painting, enhancement, P2)
Core
Web Painting
Tracking
()
People
(Reporter: mattwoodrow, Assigned: rhunt)
References
(Blocks 2 open bugs)
Details
Attachments
(13 files, 3 obsolete files)
(deleted),
text/x-phabricator-request
|
froydnj
:
review+
|
Details |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
nika
:
review+
|
Details |
(deleted),
text/x-phabricator-request
|
jrmuizel
:
review+
|
Details |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
mattwoodrow
:
review+
|
Details |
(deleted),
text/x-phabricator-request
|
mattwoodrow
:
review+
|
Details |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
mattwoodrow
:
review+
|
Details |
(deleted),
text/x-phabricator-request
|
nika
:
review+
|
Details |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/x-phabricator-request
|
Details |
Currently drawWindow only supports drawing OOP content when called from the parent process (and even then, only when you're requesting exactly what's already in the compositor).
Firefox screenshots currently uses drawWindow from the content process, and expects to get iframe content included in the result. Once we enable Fission, these iframes will be OOP, and nothing will be drawn for them.
The simplest thing to do would be to have the drawing code use synchronous IPC to request an image from child processes when it wants to draw them, but I'm working on the assumption that we can't do this (parent can't block on the child).
It's possible for content in the current process to both underlay and overlay content from the subprocess, so we can't finalize drawing from our process until we have the image from the child.
Some possible solutions:
1) Collect a list of subdocuments that are visible in the requested drawWindow area, and send asynchronous requests to them for them to draw the required content. Once all of those are completed, then draw the main process content, inserting subprocess images as needed.
The big downside here is that the content in the main process could change while we're waiting, and now the set of subprocess images that we have no longer matches what we need. We could accept this (and leave missing ones blank), or try again?
I particularly think this is a bit weird from the author's perspective, where drawWindow draws content as at some undefined state in the future, with no defined ordering relative to future changes they make.
2) Use our existing Layers infrastructure to render the page into multiple layers, so that we get separate buffers for content behind an iframe and content infront of one. Then we can request the iframe content, and do the final composition once we have all the pieces.
This still has the issue that content can change between the drawWindow call and drawing, but at least it's restricted to just the iframes, and I think that's unsolvable when they're OOP.
The hard bit here is that all our Layers infrastructure equates retaining (having explicit buffers for each layer, to be composited at a later time) with needing to track changes between paints (invalidation). All of this code assumes that we only ever have a single retained Layer tree for a document/tab, so we'd need to untangle the concepts. A bit scary.
3) Use something similar to OMTP, where we do all drawing into a recording destination, which will hold references to any resources we need, and let us execute the actual drawing at a later time. If we do this, inserting placeholders for OOP content, then we can execute the recording once we've finished acquiring all the OOP content.
We don't currently have a concept of placeholders in our recording code, but it seems simpler to add than untangling layers.
I'm leaning towards 3 at the moment as it seems the simplest, and the closest to the expected synchronous behaviour.
All of the above approaches will need a new asynchronous API to replace drawWindow.
Comment 2•6 years ago
|
||
I agree that 3 seems like a reasonable approach.
Assignee | ||
Comment 3•6 years ago
|
||
One additional point here. I believe we don't want to allow content processes to request the contents of rendering OOP iframes, as that would allow a compromised content process to view the contents of a site of a different origin.
That leads me to think we'll need to update the API more than just making it async.
One solution would be to disallow using drawWindow in any way in content processes and instead allow the parent process to use drawWindow on a child frame. The parent process would then request recordings of content from each child process, with placeholders for remote content, and then resolve the recordings.
This would make drawWindow work similarly to printing in e10s, minus OOP iframe support. Currently unsure whether we can share code here or not.
Assignee | ||
Updated•6 years ago
|
Blocks: graphics-fission
Assignee | ||
Comment 4•6 years ago
|
||
Got a prototype of this working. It's a new API called 'drawSnapshot' (name might change).
Making this a new API seems the most straightforward option as the new method is async (cannot block on other processes rendering). If the method is async, it also makes more sense to return a Promise<ImageBitmap> instead of drawing directly to a canvas. The method will also only be allowed to be used in the content process, as a compromised content process shouldn't be able to request painting from other processes.
Right now the API is exposed on 'Window' and 'FrameLoader', but it will need to find a better home.
The basic approach is to create a 'CrossProcessPaint' object which will request a paint from the desired tab (or window, if a chrome document), and then recursively request paints for further tabs as needed. Tabs remote rendering via draw target recordings. Currently they're just sent over IPDL and not through shmems or files. Once the recordings are all gathered, they are merged together into one image.
Merging and expressing dependencies between recordings is accomplished using an extension to Moz2D.
The remaining tasks are error handling, specifying the background color and DPI, setting a timeout for unresponsive content processes, and finding a good place to expose this API.
Assignee | ||
Comment 5•6 years ago
|
||
Assignee | ||
Comment 6•6 years ago
|
||
I'd like to use a struct over IPDL which is move only. This patch adds a modifier to
usings statements to indicate the C++ type should always be moved, similar to refcounted.
Assignee | ||
Comment 7•6 years ago
|
||
I'd like to use a nsTHashtable acting as a set in a struct that will be passed over IPDL, but
couldn't find any ParamTraits implementation for it. It'd be nice to make the implementation
generic, but I couldn't find an easy way to do it.
Assignee | ||
Comment 8•6 years ago
|
||
This commit adds a method to get the ContentParentId for a specified TabId. This will be used
to get the TabParent from a specified TabId.
Assignee | ||
Comment 9•6 years ago
|
||
This was giving me some font assertion crashes, and changing this as a hunch fixed it.
Assignee | ||
Comment 10•6 years ago
|
||
I'd like to be able to take the memory directly from a DrawRecorderMemory and store it
in a nsTArray. nsTArray doesn't have an API to adopt a raw char array, so this commit
changes MemStream to use a nsTArray instead.
Assignee | ||
Comment 11•6 years ago
|
||
This commit adds an API to DrawTarget to draw a surface that will be provided
at the time a recording is replayed. The surface is referenced using a user
interpreted ID.
This will be used for drawing a OOP iframe, and the ID will be the TabId.
Assignee | ||
Comment 12•6 years ago
|
||
mContainer is only ever read, and never written to. This commit removes it.
Assignee | ||
Comment 13•6 years ago
|
||
Assignee | ||
Comment 14•6 years ago
|
||
This commit adds a method to create an ImageBitmap from a SourceSurface, for use
by the new drawSnapshot API.
Assignee | ||
Comment 15•6 years ago
|
||
This commit adds a CrossProcessPaint class which can be used to paint a
cross process document tree. This API is async, as we cannot block on child
processes, and initially geared towards servicing a JS API and not internal
consumers. The API can only be used in the chrome process for security
reasons.
The class is implemented as a recursive resolver, requesting a root paint,
gathering dependent frames to be painted, then requesting paints from those
tabs. Once all paints have been completed, the dependency tree is rasterized
in a bottom up fashion.
Future improvements can be made here. Currently, the rasterization is
performed on the main thread which could cause jank. We also transmit
recordings directly over IPDl, and no effort is made to minimize the
recordings from child layer trees.
Assignee | ||
Comment 16•6 years ago
|
||
This commit initially exposes the drawSnapshot API off of <browser>. This
is done by adding a WebIDL binding to FrameLoader and wrapping it in
browser.xml.
Assignee | ||
Comment 17•6 years ago
|
||
Okay, I've got the patches to a good state now. Most of these patches are plumbing to support the IPDL/IPC code in part 11.
Currently the API is exposed to chrome JS off of <browser>. I'm not sure if that's the best place for all current consumers of drawWindow, but it's a start.
There are some other future improvements that should be made.
1. Rasterize all fragments off the main thread in the parent process
2. Support omitting size in drawSnapshot and filling it in with the size of the frame. This is important for the remote frame case, where we might not know how big it is easily.
3. Avoid OOM errors somehow, maybe using FD or shmem streaming
But again, this is a start.
Once this infrastructure is landed we can begin to use it in some places internally as well, such as thumbnails.
Assignee | ||
Updated•6 years ago
|
Summary: Support rendering OOP iframes from drawWindow in a content process → Support rendering OOP iframes with new drawSnapshot API in parent process
Comment 18•6 years ago
|
||
Comment on attachment 9011810 [details]
Bug 1475139 part 1 - Add move assignment operator to nsTHashtable. r?froydnj
Nathan Froyd [:froydnj] has approved the revision.
Attachment #9011810 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Attachment #9004395 -
Attachment is obsolete: true
Assignee | ||
Comment 19•6 years ago
|
||
Here's the patch I used to test this API, it adds a shortcut to take a screenshot of the current browser.
Assignee | ||
Comment 20•6 years ago
|
||
Here are the important differences between this API and drawWindow.
1. drawWindow is exposed off of Window, can only be used in the same process as the Window, and cannot render remote frames
2. drawWindow is synchronous
3. drawWindow renders directly to a canvas, respecting the canvas scale and blend modes
4. The rect passed to drawWindow is relative to the page origin, which can cause issues with position: fixed, position: sticky elements, as they will be still be positioned at the scroll offset
5. drawWindow accepts flags controlling the flushing behavior, caret visibility, and other things
1. drawSnapshot is (currently) exposed off of <browser>, can only be used in the parent process, and can render remote frames
2. drawSnapshot is asynchronous
3. drawSnapshot returns an ImageBitmap which can be drawn onto a canvas
4. The rect passed to drawSnapshot is relative to the current scroll offset
5. drawSnapshot doesn't accept flags, but could if they are needed
Comment 21•6 years ago
|
||
Comment on attachment 9011814 [details]
Bug 1475139 part 5 - Only use external fonts with DrawEventRecorderMemory if we have a callback. r?jrmuizel
Jeff Muizelaar [:jrmuizel] has approved the revision.
Attachment #9011814 -
Flags: review+
Reporter | ||
Comment 22•6 years ago
|
||
Comment on attachment 9011817 [details]
Bug 1475139 part 8 - Remove unused mContainer from RenderFrameParent. r?mattwoodrow
Matt Woodrow (:mattwoodrow) has approved the revision.
Attachment #9011817 -
Flags: review+
Comment 23•6 years ago
|
||
Comment on attachment 9011821 [details]
Bug 1475139 part 12 - Expose drawSnapshot API to <browser>. r?nika
:Nika Layzell has approved the revision.
Attachment #9011821 -
Flags: review+
Comment 24•6 years ago
|
||
Comment on attachment 9011813 [details]
Bug 1475139 part 4 - Add map to get ContentParentId for TabId in ContentProcessManager. r?nika
:Nika Layzell has approved the revision.
Attachment #9011813 -
Flags: review+
Reporter | ||
Comment 25•6 years ago
|
||
Comment on attachment 9011818 [details]
Bug 1475139 part 9 - Use DrawDependentSurface in nsDisplayRemote when painting to a temp layer manager with a recording. r?mattwoodrow
Matt Woodrow (:mattwoodrow) has approved the revision.
Attachment #9011818 -
Flags: review+
Reporter | ||
Comment 26•6 years ago
|
||
Comment on attachment 9011820 [details]
Bug 1475139 part 11 - Add CrossProcessPaint implementation. r?mattwoodrow
Matt Woodrow (:mattwoodrow) has approved the revision.
Attachment #9011820 -
Flags: review+
Updated•6 years ago
|
Attachment #9011815 -
Attachment is obsolete: true
Assignee | ||
Comment 27•6 years ago
|
||
Assignee | ||
Comment 28•6 years ago
|
||
Updated•6 years ago
|
Attachment #9013776 -
Attachment is obsolete: true
Comment 29•6 years ago
|
||
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab436fa8add9
part 1 - Add move assignment operator to nsTHashtable. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/766131a8c0a9
part 2 - Add 'moveonly' qualifier to IPDL using statements. r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba9a0af62c4b
part 3 - Add serialization support for nsTHashtable with nsUint64HashKey. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b5334e2a284
part 4 - Add map to get ContentParentId for TabId in ContentProcessManager. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/af900f874404
part 5 - Only use external fonts with DrawEventRecorderMemory if we have a callback. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/c048009640a9
part 6 - Add move assignment operator to ByteBuf. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed92daddd126
part 7 - Add DrawDependentSurface API to DrawTarget. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bad6cdec7da
part 8 - Remove unused mContainer from RenderFrameParent. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/128141924622
part 9 - Use DrawDependentSurface in nsDisplayRemote when painting to a temp layer manager with a recording. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/3942a3b983a6
part 10 - Add method to create ImageBitmap from SourceSurface. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/d393cf125c13
part 11 - Add CrossProcessPaint implementation. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/4de4f6aaef10
part 12 - Expose drawSnapshot API to <browser>. r=nika
Comment 30•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ab436fa8add9
https://hg.mozilla.org/mozilla-central/rev/766131a8c0a9
https://hg.mozilla.org/mozilla-central/rev/ba9a0af62c4b
https://hg.mozilla.org/mozilla-central/rev/0b5334e2a284
https://hg.mozilla.org/mozilla-central/rev/af900f874404
https://hg.mozilla.org/mozilla-central/rev/c048009640a9
https://hg.mozilla.org/mozilla-central/rev/ed92daddd126
https://hg.mozilla.org/mozilla-central/rev/0bad6cdec7da
https://hg.mozilla.org/mozilla-central/rev/128141924622
https://hg.mozilla.org/mozilla-central/rev/3942a3b983a6
https://hg.mozilla.org/mozilla-central/rev/d393cf125c13
https://hg.mozilla.org/mozilla-central/rev/4de4f6aaef10
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Comment 31•6 years ago
|
||
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/150a7a5613b7
Fixup bug 1475139 for MSVC. r=me
Comment 32•6 years ago
|
||
bugherder |
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Blocks: graphics-fission
Comment 33•5 years ago
|
||
Retroactively moving fixed bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to an appropriate Fission Milestone.
This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:
0ee3c76a-bc79-4eb2-8d12-05dc0b68e732
Fission Milestone: --- → M4
You need to log in
before you can comment on or make changes to this bug.
Description
•