1636508 - Make captureTab work with Fission iframes, expand API to cover drawWindow() usecases

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P2)

Description

[Andrew Overholt [:overholt]]

Attachment: test

Screenshots of PDFs like https://www.w3.org/WAI/ER/tests/xhtml/testfiles/resources/pdf/dummy.pdf work but those of PDFs in cross-origin iframes (like the attached) have blank areas where the PDF content is.

Comment 1

[Andrew Overholt [:overholt]]

Attachment: test.html

Simple test case

Comment 2

[Chris Peterson [:cpeterson]]

Ian, this screenshot bug blocks enabling Fission. We hope to start testing Fission in Nightly in Q3, so it would be good to get this bug fixed in Q2.

With Fission enabled, Documents and nsDocShells for related frames, such as subframes and parent documents, may not be available within the current process and the corresponding nsIDocShellTreeItem methods will return null.

Fission documentation about replacing nsIDocShellTree Item:
https://wiki.mozilla.org/Project_Fission/DocShell_Tree_Replace

:farre's presentation with examples of replacing nsIDocShellTreeItem with BrowsingContext, WindowContext, SyncedContexts, and BrowsingContextGroup:
https://docs.google.com/presentation/d/1K4j6ngty64TZjJNS5qH-MBoOm3TI2dJedVsbH8jUhKE/edit#slide=id.g6e35225e5d_1_264

Comment 3

[Ian Bicking (:ianbicking)]

Screenshots does not directly use nsIDocShellTreeItem, it only uses WebExtension APIs, DOM APIs, and probably relevant to this issue it uses drawWindow

While I'm not familiar with most of the terms here, I feel pretty confident that it can't be fixed within Screenshots. Of course someone with more of an understanding of drawWindow and how it interacts with these Fission changes might have ideas. If this is a blocker then this probably requires someone closer to Firefox to identify the underlying issue.

Comment 4

[Chris Peterson [:cpeterson]]

Thanks for the clarification. I'll send this bug back to Fission bug triage for analysis.

Comment 5

[Chris Peterson [:cpeterson]]

@ Zombie: this is a WebExtensions bug. The captureTab and captureVisibleTab APIs [1] should call the drawSnapshot API [2], which will capture remote iframes correctly from the parent process:

@ Matt: Nika suggested I needinfo you to ask if you have any tips for using the drawSnapshot API.

This problem would affect any cross-origin iframe, not just PDFs.

[1]
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/tabs/captureVisibleTab
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/tabs/captureTab

[2] https://searchfox.org/mozilla-central/rev/df4c90d4b8c92c99f76334acfe4813c573c12661/dom/chrome-webidl/WindowGlobalActors.webidl#80-97

Moving to WebExtensions component

This bug is a Fission Nightly blocker (milestone M6c).

Comment 6

[Matt Woodrow (:mattwoodrow)]

I think there's 3 separate things that need to be done here, so we may want multiple bugs.

• captureTab needs to be converted to use WindowGlobalParent.drawSnapshot (from the parent process), since this API supports capturing all contents of a given Window, including OOP <iframes> within it.

• We need new extension API (either as more options to captureTab, or a new method) to allow more control over what is captured. captureTab only captures the visible area of a tab, but screenshots needs the ability to capture a specific region, and the entire document.

• The screenshots addon needs to be converted to use the capturing extension APIs.

The second of those is probably the highest priority, since we can't start on converting screenshots until the API has all the needed functionality.

Comment 7

[Tomislav Jovanovic :zombie]

Thanks Matt, that sounds about right, though I'm not clear if drawSnapshot actually supports the ability to capture the whole document, and not just a rect relative to the current viewport?

Additionally, we'll need to deprecate the use of the drawWindow API officially.

Comment 8

[Matt Woodrow (:mattwoodrow)]

(In reply to Tomislav Jovanovic :zombie from comment #7)

Thanks Matt, that sounds about right, though I'm not clear if drawSnapshot actually supports the ability to capture the whole document, and not just a rect relative to the current viewport?

Additionally, we'll need to deprecate the use of the drawWindow API officially.

We should improve the documentation for drawSnapshot!

It captures a rect relative to the document, not the viewport, and thus handles capturing the entire document (and marionette is using it that way).

We can extend it to have a viewport-relative mode if needed.

Comment 9

[Rachel Tublitz [:rachel]]

S1 or S2 bugs need an assignee - could you find someone for this bug?

Comment 10

[Shane Caraveo (:mixedpuppy)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #6)

I think there's 3 separate things that need to be done here, so we may want multiple bugs.

• captureTab needs to be converted to use WindowGlobalParent.drawSnapshot (from the parent process), since this API supports capturing all contents of a given Window, including OOP <iframes> within it.

• We need new extension API (either as more options to captureTab, or a new method) to allow more control over what is captured. captureTab only captures the visible area of a tab, but screenshots needs the ability to capture a specific region, and the entire document.

• The screenshots addon needs to be converted to use the capturing extension APIs.

The second of those is probably the highest priority, since we can't start on converting screenshots until the API has all the needed functionality.

I'm not really clear about something here. Screenshots is using tabs.captureVisibleTab and thus to my knowledge works with whatever we currently support via that api. The comment above indicates that we need to add new functionality to the api. While that is fine if screenshots needs new functionality, I'm not clear why that part is required to fixing the api to work in fission.

(In reply to Tomislav Jovanovic :zombie from comment #7)

Additionally, we'll need to deprecate the use of the drawWindow API officially.

drawWindow is a web api, is there a bug in that component to deprecate and document the replacement?

Comment 11

[Matt Woodrow (:mattwoodrow)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #10)

I'm not really clear about something here. Screenshots is using tabs.captureVisibleTab and thus to my knowledge works with whatever we currently support via that api. The comment above indicates that we need to add new functionality to the api. While that is fine if screenshots needs new functionality, I'm not clear why that part is required to fixing the api to work in fission.

Screenshots is only using tabs.captureVisibleTab for a subset of use cases, and uses drawWindow for others - https://searchfox.org/mozilla-central/rev/fac90408bcf52ca88a3dcd2ef30a379b68ab24e2/browser/extensions/screenshots/selector/shooter.js#80

To support fission properly we need screenshots to exclusively use tabs.capture*** (and extend the APIs as needed to make that possible), as well as fixing the implementation of those APIs to use WindowGlobalParent.

Comment 12

[Tomislav Jovanovic :zombie]

Will probably split this up into two parts:

1. making the method implementation Fission compatible, and
2. extending the API to cover additional use-cases needed for Screenshots, which drawWindow() supported.

Additionally, considering adding telemetry for drawWindow(), to help us with the individual reach out to developers.

Comment 14

[Tomislav Jovanovic :zombie]

Attachment: Bug 1636508 - Make tabs.captureTab compatible with Fission

Also fix WindowGlobalParent.drawSnapshot() to render the currently visible
viewport when called with a null rect, and clarify the webidl comment.

Comment 15

[Tomislav Jovanovic :zombie]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12e979da5b3871d35110de945db5325853ec02ec

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e882756f90f
Make tabs.captureTab compatible with Fission r=mattwoodrow,robwu,geckoview-reviewers,agi

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/6e882756f90f

Comment 18

[Manuel Reimer]

So if the "only make compatible" change fixes this bug, where is the follow-up bug about covering "drawWindow()" usecases?

Comment 19

[Tomislav Jovanovic :zombie]

Reopening, should have marked this as leave-open.

Comment 20

[Shane Caraveo (:mixedpuppy)]

The test fails locally for me on osx. A fix is:

add_task(function setup() {
  Services.prefs.setCharPref("layout.css.devPixelsPerPx", String("1"));
  registerCleanupFunction(() => {
    Services.prefs.clearUserPref("layout.css.devPixelsPerPx");
  });
});

Comment 21

[Tomislav Jovanovic :zombie]

That's strange, it's supposed to use the devicePixelRatio from the chrome window:
https://searchfox.org/mozilla-central/rev/0c682c4f01/toolkit/components/extensions/parent/ext-tabs-base.js#126

Can you please post the test failure message?

Also, can you check what tabs.captureTab returns on OSX in Nightly vs Beta, and what's the value of window.devicePixelRatio from the browser console?

Comment 22

[u482975]

I'm currently using drawWindow() in a WebExtension where it's used in a synchronous fashion to capture part of the page whilst having a GUI overlayed on the page. The synchronous nature allows it to hide the overlay, capture the page, and show the overlay again; without the user noticing.

Is this a use-case that will be covered by this bug?

Comment 23

[Shane Caraveo (:mixedpuppy)]

(In reply to Tomislav Jovanovic :zombie from comment #21)

That's strange, it's supposed to use the devicePixelRatio from the chrome window:
https://searchfox.org/mozilla-central/rev/0c682c4f01/toolkit/components/extensions/parent/ext-tabs-base.js#126

Can you please post the test failure message?

All tests in the file that compare the size of an image to the size of the window, the image size is exactly twice the size, which made me look immediately at the pixel ratio.

Also, can you check what tabs.captureTab returns on OSX in Nightly vs Beta,

I'll try to look at that, the other questions were easy/quick to answer.

and what's the value of window.devicePixelRatio from the browser console?

2 (web console is 2.2222222)

Comment 24

[Tomislav Jovanovic :zombie]

Attachment: self-capture.zip

A test extension to compare captureVisibleTab. Click the browser action button, and check the size of the rendered image.

Comment 25

[Tomislav Jovanovic :zombie]

Attachment: Bug 1636508 - Add options to select captureTab area and scale

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b57de5d3cf4c
Add options to select captureTab area and scale r=robwu,geckoview-reviewers,agi

Comment 27

[Narcis Beleuzu [:NarcisB]]

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

Comment 28

[Tomislav Jovanovic :zombie]

We'll be documenting and announcing this in the next blog post only after we hear back from Screenshots folks if the API good enough, or if it needs further tweaks (bug 1664444).

Comment 29

[Caitlin Neiman [:caitmuenster]]

MDN documentation for tabs.captureTab() (https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/tabs/captureTab) &
tabs.captureVisibleTab() (https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/tabs/captureVisibleTab) have been updated (thanks @zombie!).

Comment 30

[Tomislav Jovanovic :zombie]

Also note that new additions to this API (rect and scale) are actually described on the ImageDetails argument type page:
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/extensionTypes/ImageDetails

Comment 32

[Tomislav Jovanovic :zombie]

(In reply to MartijnJoosstens from comment #22)

I'm currently using drawWindow() in a WebExtension where it's used in a synchronous fashion to capture part of the page whilst having a GUI overlayed on the page. The synchronous nature allows it to hide the overlay, capture the page, and show the overlay again; without the user noticing.

Is this a use-case that will be covered by this bug?

Sorry I missed to answer this. No, this method can't be synchronous, since cross-origin iframes will be in another process with Fission, and that will by definition require async IPC to construct the whole image. You can still continue to use drawWindow (until that is removed), it will just fail to capture cross-origin iframes with Fission enabled.

Comment 33

[Chris Mills [:cmills]]

Thanks for the MDN content :zombie. I've also included a rel note about it on the 82 rel notes page: https://wiki.developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/82#Changes_for_add-on_developers. Does this read OK?

Comment 34

[Tomislav Jovanovic :zombie]

Looks good, thanks Chris.