1678483 - Fix DevTools' "Take a screenshot of the entire page" button for Fission

Status: RESOLVED FIXED

(DevTools :: General, defect, P3)

Description

[Chris Peterson [:cpeterson]]

Fix DevTools' "Take a screenshot of the entire page" button for Fission

With Fission enabled, clicking DevTools' "Take a screenshot of the entire page" button currently does nothing.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

This can be done by using the new currentWindowGlobal.drawSnapshot() API as introduced in bug 1499845.

Here a link in how I implemented it for the remote protocol:
https://searchfox.org/mozilla-central/rev/a0ccd492719b1ad2106f6456549be62a76f45acb/remote/domains/parent/Page.jsm#274

Comment 2

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Thanks for the pointer Henrik!

Comment 4

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Since there are concern about drawSnapshot being slower than canvas.drawWindow, I created a DAMP test and run it with the current implementation and with a drawSnapshot-based one: https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=006d2f00d93e0768bb7ae19b86ed5a74e7b81e5f&newProject=try&newRevision=9f46bb0148dbd3513c55a941fe8eb6107959f5eb&framework=12

It looks like it introduce a 10% performance regression.

I feels like it's not that bad, but we might discuss it as a team before switching to drawSnapshot.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #4)

It looks like it introduce a 10% performance regression.

I feels like it's not that bad, but we might discuss it as a team before switching to drawSnapshot.

CC'ing Matt who might be interested in real numbers. Also see bug 1600269 for the actual performance related bug. Might be good to also add some details over there.

Comment 6

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

In order to take screenshots from DevTools client, we have a ScreenshotActor, which calls the devtools/server/actors/utils/capture-screenshot.js file, which calls canvas.drawWindow.

This is possible because the ScreenshotActor lives in a content process and has access to the window object directly.
As we want to switch to windowGlobal.drawSnapshot, we need to take the screenshot from the parent process.

This gives us 2 solutions:

1. Keep the ScreenshotActor in content process, and create a jsWindowActor to communicate to the parent process.
2. Move the ScreenshotActor in parent process, so we have direct access to drawSnapshot.

The issue with 2 is that in order to take the screenshot, we need a few information from the window to compute the DOMRect we need to pass to drawSnapshot:

• when doing a fullpage screenshot, we scroll the page to the top before taking the screenshot, and setting it back to the original position after (devtools/server/actors/utils/capture-screenshot.js#70-72,145)
• when doing a fullpage screenshot, we need to retrieve the fullpage width and height of the page (devtools/server/actors/utils/capture-screenshot.js#73-74 )
• for fullpage screenshot still, we need to get the size of scrollbars (devtools/server/actors/utils/capture-screenshot.js#91-96 )
• for a screenshot of a given node (or from a selector), we need to retrieve the rect of said node (devtools/server/actors/utils/capture-screenshot.js#76-81 )

So unless I'm missing something obvious, we would need to retrieve those information from the client/triggering actions before asking for the screenshot, and then eventually do some post screenshot actions. if we take the fullscreenshot example this would look like:

• The user click on the screenshot button
• DevTools client reaches an actor in content process on the server to retrieve the page and scrollbar dimensions and scroll to the top
• DevTools client can then ask the screenshot actor in parent process to draw the page with the provided rect
• Finally DevTools client reaches an actor in content process again to scroll back to the original scroll position

If we compare with the JsWindowActor solution (1):

• The user click on the screenshot button
• DevTools client reaches the screenshot actor in content process on the server
• Screenshot actors computes page and scrollbar dimensions and scroll page to the top
• then send a message to the parent jswindowactor to draw the snapshot with the provided dimensions
• finally, parent jswindowactor send back the dataurl representing the image to the screenshot actor, which can pipe it down to the client

I feel like the flow for the jsWindowActor solution is much straightforward, but we can decide as a team which solution we should implement

• Finally DevTools client reaches an actor in content process again to scroll back to the original scroll position

Comment 7

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Nika, would you know if there's any security concern using a JSWindowActor to reach the parent process from a content process?

Comment 8

[Nika Layzell [:nika] (ni? for response)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #7)

Nika, would you know if there's any security concern using a JSWindowActor to reach the parent process from a content process?

I would prefer not to expose an untrusted API which directly provides the ability for a content process to request e.g. a screenshot, as a compromised content process would be able to send one of these requests and access information it shouldn't have access to. To avoid the possibility of this being exploited, we should not pipe the screenshot data through a content process.

I also believe the pattern of having the entry point from DevTools being in the content process will lead to extra overhead, as messages between the client and the content process are already being relayed through the parent process, so we'd just be sending the message over extra bounces.

It sounds like the reason you need many of these flags is in order to take a screenshot of the entirety of the page (or similar) - perhaps this is something which should be supported by the WindowGlobalParent.drawSnapshot API directly, rather than needing to implement it using DOM manipulation. I think :mattwoodrow might be a better person to ask about the feasibility of that, though, as I am not familiar with the screenshotting code.

Comment 9

[Matt Woodrow (:mattwoodrow)]

I very much agree on the security side. WindowGlobalParent.drawSnapshot is a parent-process API, since the result can contain content from multiple origins, and we shouldn't ever share that with a fission content process.

It appears that scrolling the page back to 0 to take a full page screenshot was added in bug 961832. That looks like a bug in drawWindow, and one that I believe has since been fixed.

Marionette is doing full page screenshots without needing to scroll, so I think this should work, and we can fix it if not.

Comment 10

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Thanks you both for your answers, I'll proceed with the client -> parent process solution then.

I think drawSnapshot suffers from the fixed header issue for full page screenshot, so I filed Bug 1688813.
Eventually, it would be nice if we could migrate the devtools screenshot argument we have directly into drawSnapshot of course, but I don't know if how long it could take (we'd need 2 options, fullpage, that would take the fullpage screenshot, removing the scrollbars, and node/contentDomReference which would take the screenshot of the given element).

Comment 11

[Neha Kochar [:neha]]

When we fix this, we expect to be able to re-enable browser_jsterm_screenshot_command_file.js that was marked as a Fission failure in Bug 1686720.

Comment 12

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Attachment: Bug 1678483 - [devtools] Make capture-screenshot fission compatible. r=jdescottes.

This is by using the WindowGlobal#drawSnapshot function that, unlike Canvas#drawWindow,
handles remote frame.
Since drawSnapshot is only available on the parent process, we re-purpose the
current ScreenshotActor to a parent process one, that only calls drawSnapshot
with a passed rect on a given browsing context, and return a data url of the
screenshot.
A new target-scoped actor, ScreenshotContentActor, is added so we can retrieve
the information of the window needed to take the screenshot (the relative area
to the browsing context, the dpr), but also run pre and post screenshot tasks,
like scrolling to the top and then resettting the scroll position once the screenshot
is taken.
On the client, we add an helper function that handles all the client/server communication
and actually save the screenshot. All previous consumers of the screenshot front
use that function, that also handles backward compatibility, when connection to a server
where the screenshot actor is still living in the content process.
Some tests that were failing on Fission now pass, so we can remove their fail-if
annotation.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa4b2b19290e
[devtools] Make capture-screenshot fission compatible. r=jdescottes,devtools-backward-compat-reviewers.

Comment 14

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/fa4b2b19290e