1751961 - The `tabs.captureVisibleTab` returns wrong image if `iframe` is on the page AND user has `devicePixelRatio` above 1 (monitor scaling)

Status: VERIFIED FIXED

(Core :: Graphics, defect, P3)

Description

[juraj.masiar]

Attachment: what extension see.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:97.0) Gecko/20100101 Firefox/97.0

Steps to reproduce:

1. set DPI scaling in your OS to more than 100%, for example 125%
2. place Firefox window to this monitor
3. open any extension page with "tabs" permission, for example "uBlock origin" dashboard page. Or install "Group Speed Dial" and open new tab page.
4. open console and execute this code:

(async () => {
  const tab = await browser.tabs.create({url: 'https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe'});
  await new Promise(r => setTimeout(r, 5_000));
  const img = await browser.tabs.captureVisibleTab(tab.windowId);
  await browser.tabs.remove(tab.id);
  const imageNode = new Image();
  imageNode.src = img;
  document.body.textContent = '';
  document.body.appendChild(imageNode);
})()

5. after few seconds you will see a screenshot

Actual results:

The screenshot is bad!
The part of the page that has iframe is shifted.

Expected results:

See uploaded image below to see how it should look.

This is a regression, probably related to fission.

Comment 1

[juraj.masiar]

Attachment: real page.png

Comment 2

[juraj.masiar]

Attachment: what take screenshot see.png

Taking screenshot with the build-in tool results in the same broken screenshot.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'WebExtensions::Untriaged' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 4

[Alex Cornestean]

Hello,

I reproduced the issue as per the mentioned STR on the latest Nightly (98.0a1/20220126190617) and Beta (97.0b8/20220125201015) under Windows 10 x64. For further details, see the attached screenshot.

Also tested on the latest Release (96.0.2/20220119190439), however the issue does not occur on this version.

Performed a bisection spanning Firefox 96 to 97 and found the following regressor:

2022-01-27T09:57:21.213000: DEBUG : Found commit message:
Bug 1732358 - Part 5: Add the fission rollout slug to the GRADUATION_SET, r=mythmon
Depends on D133008
Differential Revision: https://phabricator.services.mozilla.com/D133659
2022-01-27T09:57:21.213000: DEBUG : Did not find a branch, checking all integration branches
2022-01-27T09:57:21.215000: INFO : The bisection is done.
2022-01-27T09:57:21.216000: INFO : Stopped

The corresponding pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=383986e2f5cb835a55c47bbd6e15d81035ca0784&tochange=d6e8528f0a936df88369c71f4e390e31a4d621a1

Comment 5

[Alex Cornestean]

Attachment: 2022-01-27_09h44_28.png

Comment 6

[juraj.masiar]

Thanks Alex, enabling fission.autostart in current release Firefox 96 will also trigger this bug.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1732358

Comment 8

[Tomislav Jovanovic :zombie]

I wanna look into this.

Comment 9

[Tomislav Jovanovic :zombie]

Extensions code just calls the WGP.drawSnapshot method for the whole window, so the regression is in the case when (cross-origin) subframes are in separate processes (under Fission), obviously when passing down the scale.

I would ni? Matt Woodrow, but he's not at Mozilla anymore.

Comment 10

[juraj.masiar]

Hey Tom, could you tell me if the Fission is gonna be enabled for everyone in 97?

This bug affects one of my addons so I have a workaround ready that will negate the scale in the iframe using CSS - which helps, but it will work only if fission is enabled (which I don't think I can detect from addon).

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Fission was enabled by default for all users as of 95.

Comment 12

[juraj.masiar]

Thanks Ryan, but that doesn't match the fact that this bug is not present in Firefox 96 (see comment 4 and comment 6).
Also bug 1732358 talks about Firefox 97.

I thought fission.autostart config value is the "Fission", but it's still false in 96.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

It was rolled out via remote pref flip previously for 95/96. 97 is just when it was set by default in-tree. If we wanted a better regression range, setting the fission.autostart pref to true while running mozregression would narrow it down to a better culprit.

Comment 14

[Alex Cornestean]

Hello,

I re-did the mozregression, this time with fission.autostart manually enabled on the checked builds as suggested by Ryan. Checked builds from the beginning of 2020 until the present day.

Mozregression found the following:
2022-02-03T13:51:37.626000: DEBUG : Found commit message:
Backed out changeset a1a730897df8 (bug 1723956) for causing build bustages. CLOSED TREE

2022-02-03T13:51:37.626000: DEBUG : Did not find a branch, checking all integration branches
2022-02-03T13:51:37.628000: INFO : The bisection is done.
2022-02-03T13:51:37.630000: INFO : Stopped

This is the corresponding pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ccd03e2f466b4623583fa4ad711a7a56a493d96f&tochange=9493a6e7425f88b0eb198ca0de6f026669e484cf

However, from the pushlog, the culprit might be bug 1720152 .

In case of error, let me know and I’ll attempt another bisection. Thank you !

Comment 15

[Jim Mathies [:jimm]]

Tim, any thoughts here? Fission and iframe related issue with capture. Looks like some work Matt did regressed it.

Comment 16

[Timothy Nikkel (:tnikkel)]

I've dipped my toe into this code before for another bug and I can definitely see that code not correctly handling a different devicePixelRatio but I don't know the code well enough to pinpoint the problem without spending more time understanding the code/debugging it.

Comment 17

[Sam Foster [:sfoster] (he/him)]

:jimm, this looks to be the cause of a couple of the outstanding Screenshots bugs we're trying to burn down. Would you be able to triage and see if someone can look into this?

Comment 19

[Emilio Cobos Álvarez (:emilio)]

The nsSubDocumentFrame.cpp code added in D120050 looks at least suspect.

Comment 20

[Bob Hood [:bhood]]

Emilio, any chance you'd have the time to do some debugging and nail this down a bit more?

Comment 21

[Emilio Cobos Álvarez (:emilio)]

FWIW I confirmed that backing out the code mentioned in comment 19 fixes the bug, but obvs reintroduces the print issue. What I'm not sure is what's the best way to deal with the print issue. I think this is a case where the print preview document actually incorrectly has a different DPI from the parent document, and fixing that is the right fix.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1751961 - Remove some dead code. r=tnikkel

Drive-by but found while I was looking at this code.

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1751961 - Simplify viewport computation for CrossProcessPaint. r=tnikkel

This doesn't change behavior but seems cleaner.

Depends on D142906

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1751961 - Disable snapping on cross-process paint for printing. r=tnikkel

This fixes the relevant test on Linux and matches the setup
nsDeviceContext does for printing.

Depends on D142907

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1751961 - Account for cross-process paint scale in nsSubDocumentFrame::Paint. r=tnikkel

This is the real fix.

Depends on D142908

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9fa6a362d31
Disable snapping on cross-process paint for printing. r=tnikkel

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba28fbaa025c
Remove some dead code. r=tnikkel

Comment 28

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/f9fa6a362d31
https://hg.mozilla.org/mozilla-central/rev/ba28fbaa025c

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1753f68774b
Account for cross-process paint scale in nsSubDocumentFrame::Paint. r=tnikkel

Comment 30

[Norisz Fay [:noriszfay]]

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

Comment 31

[Alex Cornestean]

Hello,

Verified the fix on the latest Nightly (101.0a1/20220413215253) under Windows 10 x64 following the original STR from Comment 0.

The part of the page with the iframe is no longer shifted, resulting in a proper screenshot and confirming the fix. For further details, see the attached screenshot.

Comment 32

[Alex Cornestean]

Attachment: 2022-04-14_10h39_09.png

Comment 35

[Mathew Hodson]

I looked at the test case from bug 1750026 and verified taking a zoomed screenshot of the building diagram is working at https://www.theguardian.com/society/2022/jan/12/planners-set-to-approve-52-storey-london-tower-with-only-one-staircase

Tested with Firefox 101.0a1 build ID 20220413113606

Comment 37

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 38

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9270816 [details]
Bug 1751961 - Account for cross-process paint scale in nsSubDocumentFrame::Paint. r=tnikkel

Beta/Release Uplift Approval Request

• User impact if declined: Screenshots of iframes are wrongly scaled.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Comment 0 or any of the dupes
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Pretty self-contained patches.
• String changes made/needed: none

Comment 39

[Dianna Smith [:diannaS]]

Comment on attachment 9270813 [details]
Bug 1751961 - Remove some dead code. r=tnikkel

Approved for 100.0b8

Comment 40

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ce4e42aaeb18
https://hg.mozilla.org/releases/mozilla-beta/rev/16dae0c1a60a
https://hg.mozilla.org/releases/mozilla-beta/rev/711f2255576a

Comment 41

[Alex Cornestean]

Hello,

Verified the fix on the latest Beta (100.0b8/20220419190903) under Windows 10 x64 following the original STR from Comment 0.

The part of the page with the iframe is no longer shifted, resulting in a proper screenshot and confirming the fix. See the attached screenshot for further details.

Comment 42

[Alex Cornestean]

Attachment: 2022-04-20_09h23_57.png