Closed Bug 1636508 Opened 4 years ago Closed 4 years ago

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

Categories

(WebExtensions :: General, defect, P2)

defect

Tracking

(Fission Milestone:M6b, firefox-esr68 disabled, firefox76 disabled, firefox77 disabled, firefox78 disabled, firefox82 fixed)

RESOLVED FIXED
82 Branch
Fission Milestone M6b
Tracking Status
firefox-esr68 --- disabled
firefox76 --- disabled
firefox77 --- disabled
firefox78 --- disabled
firefox82 --- fixed

People

(Reporter: overholt, Assigned: zombie)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 1 obsolete file)

Attached file test (obsolete) —

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.

Attachment #9146813 - Attachment filename: file_1636508.txt → file_1636508.html
Attached file test.html

Simple test case

Attachment #9146813 - Attachment is obsolete: true
Summary: [Fission] screenshots of PDFs at Fastmail end up blank → [Fission] screenshots of PDFs in cross-origin iframes end up blank

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

Fission Milestone: --- → M6b
Flags: needinfo?(ianb)

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.

Flags: needinfo?(ianb) → needinfo?(cpeterson)

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

Severity: -- → S3
Fission Milestone: M6b → ?
Flags: needinfo?(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).

Severity: S3 → --
Fission Milestone: ? → M6c
Component: Screenshots → Untriaged
Flags: needinfo?(tomica)
Flags: needinfo?(matt.woodrow)
Product: Firefox → WebExtensions
Summary: [Fission] screenshots of PDFs in cross-origin iframes end up blank → WebExtension captureTab/captureVisibleTab APIs do not work with Fission remote iframes

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.

Flags: needinfo?(matt.woodrow)

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.

Flags: needinfo?(tomica) → needinfo?(matt.woodrow)

(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.

Flags: needinfo?(matt.woodrow)
Severity: -- → S2
Priority: -- → P2
Component: Untriaged → General

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

Flags: needinfo?(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?

Flags: needinfo?(mixedpuppy) → needinfo?(matt.woodrow)

(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.

Flags: needinfo?(matt.woodrow)
Fission Milestone: M6c → M6b

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.

Assignee: nobody → tomica
Summary: WebExtension captureTab/captureVisibleTab APIs do not work with Fission remote iframes → Make captureTab work with Fission iframes, expand API to cover drawWindow() usecases

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

Pushed by tjovanovic@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6e882756f90f
Make tabs.captureTab compatible with Fission r=mattwoodrow,robwu,geckoview-reviewers,agi
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch

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

Reopening, should have marked this as leave-open.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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");
  });
});
Flags: needinfo?(tomica)

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?

Flags: needinfo?(tomica) → needinfo?(mixedpuppy)

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?

(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)

Flags: needinfo?(mixedpuppy) → needinfo?(tomica)
Attached file self-capture.zip

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

Flags: needinfo?(tomica)
Attachment #9173609 - Attachment description: Bug 1636508 - Add options to select captureTab area and scale Also fix test for HiDPI monitors, and refactor it to remove duplicate code for captureVisibleTab. → Bug 1636508 - Add options to select captureTab area and scale
Whiteboard: patch awaiting review
Pushed by tjovanovic@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b57de5d3cf4c
Add options to select captureTab area and scale r=robwu,geckoview-reviewers,agi
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Blocks: 1664444

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).

Keywords: dev-doc-needed
Whiteboard: patch awaiting review
Regressed by: 1664522
No longer regressed by: 1664522
Regressions: 1664522

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

(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.

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?

Looks good, thanks Chris.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: