matt Reporter
	Description • 6 years ago

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.106 Safari/537.36

Steps to reproduce:

Platform: Windows 64bit
Firefox version: 61.02 (64bit)
Geckodriver version: v0.21.0

I used the screenshot element command and attempted to screenshot the html element (the html tag). This works on every other page I have ever tried, but for some reason when I try taking a screenshot of this page, Marionette throws an error.

Website URL that throws the error: http://www.kkr.com/global-perspectives/publications/china-a-visit-to-the-epicenter


Actual results:

1535043722204   webdriver::server       DEBUG   <- 200 OK {"value":null}
1535043722205   webdriver::server       DEBUG   -> GET /session/612fae6b-1097-4b2a-a63d-3270aa942b60/element/bd79972f-a77d-4678-a4e1-e64d61dd65b9/screenshot
1535043722238   webdriver::server       DEBUG   <- 500 Internal Server Error {"value":{"error":"unknown error","message":"[Exception... \"Failure\"  nsresult: \"0x80004005 (NS_ERROR_FAILURE)\"  location: \"JS frame :: chrome://marionette/content/capture.js :: capture.canvas :: line 135\"  data: no]","stacktrace":"capture.canvas@chrome://marionette/content/capture.js:135:3\ncapture.element@chrome://marionette/content/capture.js:46:10\ntakeScreenshot@chrome://marionette/content/listener.js:1562:14\ndispatch/</req<@chrome://marionette/content/listener.js:489:14\ndispatch/<@chrome://marionette/content/listener.js:484:15\n"}}


Expected results:

The page itself was completely in view so this should have worked without a problem.

	Daniel_A[:daschilean] [Please contact - liviu.seplecan@softvision.ro]
	Comment 1 • 6 years ago

User Agent:  Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180807170231                  
I manage to reproduce this issue on Windows 10 x64 with Firefox release version 61.0.2(64-bit).

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 2 • 6 years ago

The problem here is at:
https://dxr.mozilla.org/mozilla-release/source/testing/marionette/capture.js#135

> ctx.scale(scale, scale);

I can remember that I have seen a similar issue in the past, but no bug has been filed for it yet.

Note that this also affects beta and nightly!

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 3 • 6 years ago

The problem here is that the page has a height of `32771.58203125` pixels, which exceeds the max value of a 16-bit integer. When I manually reduce the height to `32767` it works, and with `32768` it fails.

Here some Javascript code which can be run in scratchpad to reproduce the problem:

```
var canvas = window.document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
canvas.width = 100;
canvas.height = 32768;

var ctx = canvas.getContext("2d");
ctx.scale(1, 1);
```

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 4 • 6 years ago

Ok, so it looks like that Cairo hard-codes the maximum size of a canvas to 32767:

https://dxr.mozilla.org/mozilla-central/rev/9c13dbdf4cc9baf98881b4e2374363587fb017b7/gfx/2d/DrawTargetCairo.h#194

That means if width*scale or height*scale is larger than this value, we have to set 32767 as limit and ignore any other content.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 5 • 6 years ago

Actually not sure what those numbers are what `GetMaxSurfaceSize()` returns. When I limit both the width and height to 32767, it even fails. The maximum I'm able to use is the following:

> canvas.width = 3812;
> canvas.height = 32767;

So maybe `GetMaxSurfaceSize()` is not enough, and there is another limit we reach here.

Given that we might never have a canvas of this size I would go ahead and limit width and height to 32767 first. If failures are still getting reported we could evaluate more.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 6 • 6 years ago

The Skia GFX backend limits the dimension of canvases to a maximum
of 32767 for the width and height.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 7 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35b397c6e8c80cd4eb0d76aa9cc347a98ed76831

	Andreas Tolfsen ❲:ato❳
	Comment 8 • 6 years ago

Comment on attachment 9004525 [details] [diff] [review]
[marionette] Limit width and height of created canvas to 32767 pixels

Review of attachment 9004525 [details] [diff] [review]:
-----------------------------------------------------------------

Perhaps we should also log a warning when the dimensions are over
this limit?

::: testing/marionette/capture.js
@@ +113,5 @@
>  
>    if (canvas === null) {
>      canvas = win.document.createElementNS(XHTML_NS, "canvas");
> +    canvas.width = Math.min(width * scale, 32767);
> +    canvas.height = Math.min(height * scale, 32767);

Nit: Put magical number in constant with descriptive name of what
it implies, e.g. MAX_SKIA_DIMENSIONS.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 9 • 6 years ago

The Skia GFX backend limits the dimension of canvases to a maximum
of 32767 for the width and height.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 10 • 6 years ago

The Skia GFX backend limits the dimension of canvases to a maximum
of 32767 for the width and height.

	Andreas Tolfsen ❲:ato❳
	Comment 11 • 6 years ago

Comment on attachment 9004833 [details] [diff] [review]
[marionette] Limit width and height of created canvas to 32767 pixels

Review of attachment 9004833 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/marionette/capture.js
@@ +129,4 @@
>    if (canvas === null) {
>      canvas = win.document.createElementNS(XHTML_NS, "canvas");
> +    canvas.width = width;
> +    canvas.height = height;

width and height for <canvas> are no longer scaled up, shouldn’t
these values be set before checking the MAX_SKIA_DIMENSIONS?

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 12 • 6 years ago

(In reply to Andreas Tolfsen ﹝:ato﹞ from comment #11)
> >      canvas = win.document.createElementNS(XHTML_NS, "canvas");
> > +    canvas.width = width;
> > +    canvas.height = height;
> 
> width and height for <canvas> are no longer scaled up, shouldn’t
> these values be set before checking the MAX_SKIA_DIMENSIONS?

Ups, this is true. Will have this fixed in the next update.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 13 • 6 years ago

The Skia GFX backend limits the dimension of canvases to a maximum
of 32767 for the width and height.

	Andreas Tolfsen ❲:ato❳
	Comment 14 • 6 years ago

Comment on attachment 9004849 [details] [diff] [review]
[marionette] Limit width and height of created canvas to 32767 pixels

Review of attachment 9004849 [details] [diff] [review]:
-----------------------------------------------------------------

r+wc

::: testing/marionette/capture.js
@@ +115,5 @@
>    const scale = win.devicePixelRatio;
>  
>    if (canvas === null) {
> +    let canvas_width = width * scale;
> +    let canvas_height = height * scale;

I think this will fail the linter because of the camel casing.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 15 • 6 years ago

The Skia GFX backend limits the dimension of canvases to a maximum
of 32767 for the width and height.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 16 • 6 years ago

Comment on attachment 9004852 [details] [diff] [review]
[marionette] Limit width and height of created canvas to 32767 pixels

Fixed the linter issue, and taking over the r+.

	Pulsebot
	Comment 17 • 6 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd6fb6ab62f4
[marionette] Limit width and height of created canvas to 32767 pixels. r=ato

	Cosmin Sabou [:CosminS]
	Comment 18 • 6 years ago
bugherder

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