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)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Fission Milestone M4
Tracking Status
firefox63 --- wontfix
firefox64 --- fixed

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.
Ryan agree to drive this.
Assignee: nobody → rhunt
I agree that 3 seems like a reasonable approach.
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.
Attached patch draw-snapshot-v1.patch (obsolete) (deleted) — Splinter Review
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.
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.
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.
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.
This was giving me some font assertion crashes, and changing this as a hunch fixed it.
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.
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.
mContainer is only ever read, and never written to. This commit removes it.
This commit adds a method to create an ImageBitmap from a SourceSurface, for use by the new drawSnapshot API.
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.
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.
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.
Summary: Support rendering OOP iframes from drawWindow in a content process → Support rendering OOP iframes with new drawSnapshot API in parent process
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+
Attachment #9004395 - Attachment is obsolete: true
Attached patch snapshot-example.patch (deleted) — Splinter Review
Here's the patch I used to test this API, it adds a shortcut to take a screenshot of the current browser.
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 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+
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 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 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+
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+
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+
Attachment #9011815 - Attachment is obsolete: true
Attachment #9013776 - Attachment is obsolete: true
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
Blocks: rendering-fission
No longer blocks: graphics-fission

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.

Attachment

General

Created:
Updated:
Size: