1994148 - browsingContext.captureScreenshot should return an error instead of a screenshot with a reduced size

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, defect, P3)

Description

[Holger Benl [:hbenl]]

Currently, browsingContext.captureScreenshot caps the screenshot size if the requested size is too large. But the BiDi spec says that it should return an unsupported operation error if it can't capture a screenshot (which in my opinion includes this case where the browser can only capture a screenshot with a reduced size).

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I think that we should better update step 13 of the remote end steps and use the trying mechanism. If render the document to a canvas fails we probably want to raise a unable to capture screen error as well.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

It should indeed be step 3 because that's the place where the canvas is created. So unsupported operation error is correct.

Comment 3

[Sameem [:sameembaba]]

Hi, I’ve got my local setup ready! would it be okay if I work on this issue?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Sameem, you are welcome to work on this bug. Let me know in case you have questions. Thanks!

Comment 5

[Sameem [:sameembaba]]

Thanks! I'm getting started now.

I've located the resize logic in remote/shared/Capture.sys.mjs inside capture.canvas. My plan is to remove the code that caps the dimensions and replace it with an error check.

Comment 6

[Sameem [:sameembaba]]

Attachment: Bug 1994148 - Throw UnsupportedOperationError when screenshot is too large. r=whimboo

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/1167cbc450fe
https://hg.mozilla.org/integration/autoland/rev/a33079b82412
Throw UnsupportedOperationError when screenshot is too large. r=whimboo

Comment 8

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/57564 for changes under testing/web-platform/tests

Comment 9

[Cosmin Sabou [:CosminS]]

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

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thank you so much Sameem!

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As I just saw, in Firefox we increased the maximum size for screenshots to 65535x65535 pixels to match Chrome's behavior. The changes landed end of January this year via bug 1911583. That's the reason why the most recent testing didn't throw an error with 32767x32767 sizes.

Comment 13

[Sameem [:sameembaba]]

We just have to change the test dimension to fix it! When we landed the patch i don't think it had any problem with the test we wrote. So, what is the plan?

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hm, I wonder why the test def test_huge_full_screenshot in testing/web-platform/mozilla/tests/webdriver/classic/take_full_screenshot/screenshot.py still passes. I would expect that it fails now.

Sameem, I'm currently away until next week. So if you could file a new bug and take a look why the error is still thrown for the above test it would be great. My colleagues in the #webdriver channel can help you in case of questions. Thanks!

Comment 15

[Sameem [:sameembaba]]

Yeah, they should all fail the new test i have created as they have the previous boundaries(32768px)! I will check what is wrong here

Comment 16

[Sameem [:sameembaba]]

I did some test and i see when i instrumented capture.canvas. Canvas allocation succeeds with 32768×32768 (DPR=1). The failure occurs inside drawSnapshot, which throws NS_ERROR_LOSS_OF_SIGNIFICANT_DATA which we catch and throw an UnsupportedOperationError. That is why the the test still passes

Comment 17

[Sameem [:sameembaba]]

Hey there, i got busy last week! So, I didnt have time to work on it! The above message i sent about the different error being is this how it supposed behavior or do we have to change it?

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Interesting. First, could you check if it also fails with drawWindow() (the other code block)? Just force readback to be true for this purpose. Maybe they only increased it for that code path. If yes we would have to think about how to better handle it.

Comment 19

[Sameem [:sameembaba]]

I tested both paths:

• drawWindow() fails in the Canvas 2D layer with InvalidStateError: Canvas exceeds max size, due to the gfx.canvas.max-area limit.
• drawSnapshot() fails deeper in the snapshot/compositor pipeline with NS_ERROR_LOSS_OF_SIGNIFICANT_DATA.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Sameem [:sameembaba] from comment #19)

I tested both paths:

• drawWindow() fails in the Canvas 2D layer with InvalidStateError: Canvas exceeds max size, due to the gfx.canvas.max-area limit.
• drawSnapshot() fails deeper in the snapshot/compositor pipeline with NS_ERROR_LOSS_OF_SIGNIFICANT_DATA.

Hi Lee, given that you wrote the patch on bug 1911583 maybe you can help us? Maybe it was not intended to use larger canvas sizes for screenshots, especially when both the width and height larger than 32767? Or it is a miss and we should add proper support for it? Thanks!

Comment 21

[Lee Salzman [:lsalzman]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+1] from comment #20)

(In reply to Sameem [:sameembaba] from comment #19)

I tested both paths:

• drawWindow() fails in the Canvas 2D layer with InvalidStateError: Canvas exceeds max size, due to the gfx.canvas.max-area limit.
• drawSnapshot() fails deeper in the snapshot/compositor pipeline with NS_ERROR_LOSS_OF_SIGNIFICANT_DATA.

Hi Lee, given that you wrote the patch on bug 1911583 maybe you can help us? Maybe it was not intended to use larger canvas sizes for screenshots, especially when both the width and height larger than 32767? Or it is a miss and we should add proper support for it? Thanks!

To be compatible with our Skia backends, the total size still needs to fit in a signed 32-bit integer to not overflow. For compat, we needed to be able to support sizes such as 64000x10, which is a very small area objectively, but has a dimension over the previous 32K limit. We, however, would not support a 64000x64000 canvas, because that would violate the constraint that the size can fit in signed 32 bits without overflow.

Essentially, I kept the area constraint to the same effective value as would be imposed by a 32K x 32K limit (as I am not sure yet we want to allow canvases that can consume more memory internally without any headroom), but I allow individual dimensions to reach up to 64K. The gfx.canvas.max-area pref implements this constraint.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thank you Lee!

Sameem, with the above information I may propose that we re-add a check that the size of the screenshot doesn't exceed the gfx.canvas.max-area value. That way we would be flexible with any future change and as well could raise a specific error beside any other platform error which could happen as well. Would you like to file and work on such a bug?

Comment 23

[Sameem [:sameembaba]]

Yeah! I would like to work on it

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Great. I filed bug 2020302 for it.