Closed Bug 2020302 Opened 5 months ago Closed 3 months ago

Over on bug 1994148 we removed the explicit checks for the maximum width and height values and allowing the platform to raise an error in case the screenshot area is too big. But given that there are lots of possible errors it would be good to raise an explicit error when we are outside of the allowed bounds for the dimension.

Given that on bug 1911583 the maximum width and height have been increased to 64k it's still not allowed to capture a screenshot that is larger than 32k*32k in size. For details see bug 1994148 comment 21. As such lets use gfx.canvas.max-area to check against the maximum allowed area and if the value is outside we can raise a clearer error while as well making our behavior dynamic to any future changes to the pref's value.

Sameem is interested to work on this bug, so I'm already assigning him. Thank you!

So, the plan here is to remove the try catch block and do it like we were doing it before checking the max dimensions and area. right?

Yes, that would be better to have explicit error messages, which are not send by the underlying platform, but already in our code. Note that there is as well gfx.canvas.max-size which we can use for it.

Best would be to have a lazy preference getter defined for both of these preferences and the check you can perform as usual.

Priority: -- → P3

Whiteboard: [lang=js][lang=py] → [lang=js][lang=py][webdriver:backlog]

Hey there! I commented in the PR asking a question about your review. Just checking in if you saw that message or not.

There is an r+ patch which didn't land and no activity in this bug for 2 weeks.
:sameembaba, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Hey, this patch is already approved. The reviewer was away this past week, which caused the delay. I’ve pinged them and will move it forward as soon as they’re back.

Status: NEW → ASSIGNED

Flags: needinfo?(hskupin)

Flags: needinfo?(ssssameembaba)

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

1. The Rounding Error: First, the 1px overflow (65536px exceeds...) was real. Android's default viewport scaling was taking the 65535px width and rounding the final layout up to 65536px. Adding a <meta name="viewport"> tag fixed this math calculation.

Adding the meta tag for the screenshot tests would indeed be a good idea. It helps with having a consistent scaling, and reduces side-effects like that. We already did that for other tests but looks like we missed to update it here.

2. The Hardware Crash: However, once the layout was correctly locked under the limit, the test failed with Failed to decode response from marionette. I discovered this is a hard crash. The Android emulator's GPU/WebRender physically cannot allocate a horizontal texture that massive (65535px wide). It throws an Out-Of-Memory exception and completely kills the Firefox process, severing the Marionette connection. (Interestingly, the height axis passes perfectly fine due to vertical tiling).

Hm, when does it happen? I assume it needs the above fix to get reproduced? If so please update the patch in Phabricator so that I can re-push to try to see it within the logs. I think that we would have to ask some GeckoView folks about that. It's strange that this would trigger a OOM.

@whimboo, What should we do about this? I think we can skip this test for android platform

We could mark it as such when it turns out that there is a bug for GeckoView that needs to be fixed. But first lets examine first what this could be exactly.

Flags: needinfo?(hskupin)

I have updated the patch to include the meta tag! You can check and let me know what is the next step.

It works all fine for me locally. So I pushed a try build. More details in Phabricator.

Status: ASSIGNED → RESOLVED

Closed: 3 months ago

Resolution: --- → FIXED

Target Milestone: --- → 152 Branch

Thank you Sameem for the enhancement! Now lets continue with the visual viewport one which isn't done yet.

Thanks for the guidance!

Also, I commented about if you could vouch for me for level 1 access. If you want me to do some more contribution that is also okay. Let me know what you decide.

Summary: Explictly check for the maximum screenshot dimensions → Explicitly check for the maximum screenshot dimensions

Whiteboard: [lang=js][lang=py][webdriver:m20][webdriver:external] → [lang=js][lang=py][webdriver:m20][webdriver:external][webdriver:relnote]

Hi Henrik, Thanks for the update! I don't have a specific bug in mind at the moment. If there's a bug or task that you think would be a good fit for a next contribution, I'd be happy to take a look at it.