1343217 - Grid Inspector fails on http://gridbyexample.com/examples/

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Julian Descottes [:jdescottes]]

(you need to have the layout panel enabled, go to devtools options on nightly to enable it)

STRs:
- go to http://gridbyexample.com/examples/
- open inspector, go to layout panel
- scroll down to the grid container list
- check any of the grid container

ER: checkbox is checked, grid highlighter is displayed
AR: nothing happens, errors in the browser console:



console.error: 
  Message: [Exception... "Failure"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/highlighters/css-grid.js :: clearCanvas :: line 496"  data: no]
  Stack:
    clearCanvas@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/highlighters/css-grid.js:496:5
_update@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/highlighters/css-grid.js:408:5
update@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/highlighters/auto-refresh.js:218:5
_startRefreshLoop@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/highlighters/auto-refresh.js:247:5
show@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/highlighters/auto-refresh.js:109:5
show@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/highlighters.js:498:12
handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1082:19
onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1769:15
receiveMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:761:7

Comment 1

[Julian Descottes [:jdescottes]]

I logged the dimensions of the created canvas:
- width: 1994
- height: 38633

The max dimension for canvas elements is 32767, over this limit the canvas is simply not rendered.

Matteo: did you experience similar issues with other canvas based highlighters?

Comment 2

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Julian Descottes [:jdescottes] from comment #1)

> I logged the dimensions of the created canvas:
> - width: 1994
> - height: 38633
> 
> The max dimension for canvas elements is 32767, over this limit the canvas
> is simply not rendered.
> 
> Matteo: did you experience similar issues with other canvas based
> highlighters?

No, usually the canvas covered only the viewport's size. We should probably do the same here; unfortunately due the APZ that means that we'll get back a bit of lag – that we solved for the absolutely positioned element (bug 1312103).

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

Obliviously we need to fix this.
As discussed with Patrick, the immediate fix – the one in the patch below – will be just avoid the exception and display at least something. That means, cut the canvas at the maximum size available. Unfortunately, that seems really depends by the platform, more specifically it seems depends by the GPU. In my case, for instance, it would be 4k per side where in most of recent Mac Book would be 16k. Plus, you have to half that size if you're on retina display, so in my case the maximum canvas would be 2048x2048, that is not really much.
That's why we need a proper fix, the one in the patch is really a quick temporary fix.

The original issue depends by the SkiaGL library – the exception is more clear when we try to create a slightly bigger canvas than the one allowed:

Crash Annotation GraphicsCriticalError: |[C0][GFX1-]: Failed to create a SkiaGL DrawTarget, falling back to software
 (t=11.5064) |[C1][GFX1-]: Failed to create a SkiaGL DrawTarget, falling back to software
 (t=11.5766) [GFX1-]: Failed to create a SkiaGL DrawTarget, falling back to software

Since is a Skia issue, we're also not the only one to suffer from this:

https://bugs.chromium.org/p/skia/issues/detail?id=2122

https://github.com/catapult-project/catapult/issues/2416#issuecomment-232327904

So far, here the solutions we could have:

1. Make the grid highlighter positioned fixed and handle the scrolling in the canvas
Cons: we get back a bit of jitters during the scrolling, as before bug 1312103

2. Creating multiple canvas as needed, and draw to them as they were one canvas
Cons:
 - the creation of canvas will potentially happens every zoom or resize of window; or in multiple monitors with different display ratio; or if there are elements in the page that dynamically increase the size of the document. We would need to map all of the canvas' primitive and proxy them.
We could also end up to have not only multiple canvas in height but in both width and height, makes more complex to draw across all of them. In addition, all those operation and canvases will impact the perf as well.

3. Have a way to disable the APZ, as it would be for scrollable elements, for canvas-based highlighters
Cons: we'll get back the "junky" scroll before the APZ. Personally I don't think it's too bad – we'll have that only during the "debugging" on a page, a developer won't expect the same performance when devtools are open compare to regular navigation – but the layout team doesn't seem to keen about it (see Bug 1316318 comment 9), I can understand however if they're afraid to create an API that could be misused.

4. Use the "virtual lists" approach (see again Bug 1316318 comment 9); to do it properly we probably need some new platform APIs, but I will double check this part with the layout team this week, luckily.
Cons: I've to try the implementation and check what any platform API could do for us, but my feeling is that we just mitigate the jitters in scrolling here, since we have to listen to the async scroll anyway to move the canvas when the user scroll outside the rendered area.

If there is any way to make viable the option 3, I would go for it. If it's not, we should probably check the options 4 before fallback to 1 or 2.

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Bug 1343217 - Cut the CssGridHighlighter canvas size to its maximum size;

- Added temporary fix to cut the canvas at its maximum size allowed

Review commit: https://reviewboard.mozilla.org/r/116598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116598/

Comment 5

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8842823 [details]
Bug 1343217 - Cut the CssGridHighlighter canvas size to its maximum size;

Before going to review I'd like to have a feedback from you guys about this approach.
Especially from Brad, to understand if the way I'm obtaining the maximum size available for a canvas side is fine – empirically it seems – or there is a better way to do so.

Comment 6

[Patrick Brosset <:pbro>]

As discussed today with Matteo, Gabriel and Julian, let's make this a P1, so we at least land the first quick fix. The actual fix is likely to take longer, and will not land in 54.

Comment 7

[Brad Werth [:bradwerth] | Out until March 3]

Comment on attachment 8842823 [details]
Bug 1343217 - Cut the CssGridHighlighter canvas size to its maximum size;

If the texture size limit is the practical limit for gl canvasses (which don't have a spec-defined maximum size, that I can find), then checking gl.getParameter(gl.MAX_TEXTURE_SIZE) is the right way to get that limit.

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8842823 [details]
Bug 1343217 - Cut the CssGridHighlighter canvas size to its maximum size;

As discussed last week in a call, the various solutions we are contemplating to really fix this seem to be complex and time-consuming, so we should at the very least fix the crash quickly. This patch seems to do the trick.

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8842823 [details]
Bug 1343217 - Cut the CssGridHighlighter canvas size to its maximum size;

https://reviewboard.mozilla.org/r/116598/#review119150

::: devtools/shared/layout/utils.js:669
(Diff revision 1)
> + * @param {DOMNode|DOMWindow|DOMDocument} node The node to get the window for.
> + * @return {Number} the max size allowed
> + */
> +function getMaxSurfaceSize(node) {
> +  let canvas = getWindowFor(node).document.createElement("canvas");
> +  let gl = canvas.getContext("webgl");

Under some conditions, canvas.getContext("webgl") can return null. In this case, we should fallback to a sensible default value.

For instance I tested the patch on a Ubuntu VM, and got this issue.

Comment 10

[Julian Descottes [:jdescottes]]

Comment on attachment 8842823 [details]
Bug 1343217 - Cut the CssGridHighlighter canvas size to its maximum size;

Same comment as Patrick, just make sure to address my comment about potentially null webgl contexts.

Comment 11

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8842823 [details]
Bug 1343217 - Cut the CssGridHighlighter canvas size to its maximum size;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116598/diff/1-2/

Comment 12

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8842823 [details]
Bug 1343217 - Cut the CssGridHighlighter canvas size to its maximum size;

https://reviewboard.mozilla.org/r/116598/#review119150

> Under some conditions, canvas.getContext("webgl") can return null. In this case, we should fallback to a sensible default value.
> 
> For instance I tested the patch on a Ubuntu VM, and got this issue.

Thanks for the good catch!

Comment 13

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8842823 [details]
Bug 1343217 - Cut the CssGridHighlighter canvas size to its maximum size;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116598/diff/2-3/

Comment 14

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8842823 [details]
Bug 1343217 - Cut the CssGridHighlighter canvas size to its maximum size;

Added Patrick as reviewer since I updated him with new info about the bug this morning, but I'd like to have a feedback from you too Julian.

Comment 15

[Julian Descottes [:jdescottes]]

Comment on attachment 8842823 [details]
Bug 1343217 - Cut the CssGridHighlighter canvas size to its maximum size;

https://reviewboard.mozilla.org/r/116598/#review120016

::: devtools/server/actors/highlighters/css-grid.js:61
(Diff revision 3)
>  // WeakMap key for the Row grid pattern.
>  const ROW_KEY = {};
>  // WeakMap key for the Column grid pattern.
>  const COLUMN_KEY = {};
>  
> +// That's the maximum size we can allocate for the cavas, in bytes. See:

cavas -> canvas

::: devtools/server/actors/highlighters/css-grid.js:667
(Diff revision 3)
> +      // Therefore we don't only check if `height` is greater than `width`, but also
> +      // that `width` is not greater than MAX_ALLOC_PIXELS_PER_SIDE (otherwise we'll end
> +      // up to reduce `height` in favor of `width`, for example).
> +      if (height > width && width < MAX_ALLOC_PIXELS_PER_SIDE) {
> +        height = (MAX_ALLOC_PIXELS / width) |0;
> +      } else if (width > height && height < MAX_ALLOC_PIXELS) {

MAX_ALLOC_PIXELS -> MAX_ALLOC_PIXELS_PER_SIDE ?

::: devtools/server/actors/highlighters/css-grid.js:684
(Diff revision 3)
> +      // Unfortunately that would also display the source, where if clicked , will ends
> +      // in a non-existing document.
> +      // It's not ideal, but from an highlighter there is no an easy way to show such
> +      // notification elsewhere.
> +      this.win.console.warn("The CSS Grid Highlighter could have been clipped, since " +
> +                            "the size of the document inspected.\n" +

feels like this is missing the end of the sentence? "since the size of the document inspected exceeded the limitations" ?

::: devtools/server/actors/highlighters/css-grid.js:693
(Diff revision 3)
> +
>      // Resize the canvas taking the dpr into account so as to have crisp lines.
> -    this.canvas.setAttribute("width", width * ratio);
> -    this.canvas.setAttribute("height", height * ratio);
> -    this.canvas.setAttribute("style", `width:${width}px;height:${height}px;`);
> +    this.canvas.setAttribute("width", width);
> +    this.canvas.setAttribute("height", height);
> +    this.canvas.setAttribute("style",
> +      `width:${width / ratio}px;height:${height / ratio}px;`);

not sure, but maybe we should round here ?

::: devtools/shared/layout/utils.js:670
(Diff revision 3)
> + * @return {Number} the max size allowed
> + */
> +function getMaxSurfaceSize(node) {
> +  let canvas = getWindowFor(node).document.createElement("canvas");
> +  let gl = canvas.getContext("webgl");
> +  let size = 4096;

Maybe move to a const. Either way, let's add a comment explaining this is a fallback when webgl context is unavailable.

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8842823 [details]
Bug 1343217 - Cut the CssGridHighlighter canvas size to its maximum size;

Looks good to me, addresses the issue I mentioned. Thanks.

Comment 17

[Patrick Brosset <:pbro>]

Comment on attachment 8842823 [details]
Bug 1343217 - Cut the CssGridHighlighter canvas size to its maximum size;

https://reviewboard.mozilla.org/r/116598/#review120022

Only a couple more comments on top of what Julian already said.
Please address our comments and feel free to land this and request aurora uplift approval.
Thanks.

::: commit-message-6d0ac:1
(Diff revision 3)
> +Bug 1343217 - Grid Highlighter fails on http://gridbyexample.com/examples/; r=pbro

Please do not simply restate the bug number in the commit message's first line.

This would work fine I guess:

```
Bug 1343217 - Cut the CssGridHighlighter canvas size to its maximum size; r=pbro

This is a temporary fix to prevent the CssGridHighlighter from crashing on very big pages.
This way, at least some parts of the highlighter are visible.
See bug 1345434 for the real fix.
```

::: devtools/server/actors/highlighters/css-grid.js:678
(Diff revision 3)
> +      // We display the warning in the web console, so the user will be able to see it.
> +      // Unfortunately that would also display the source, where if clicked , will ends
> +      // in a non-existing document.
> +      // It's not ideal, but from an highlighter there is no an easy way to show such
> +      // notification elsewhere.

You should add to this comment that this is only a temporary workaround and is supposed to be non-localized.
Please mention that bug 1345434 will get rid of this.

Comment 18

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8842823 [details]
Bug 1343217 - Cut the CssGridHighlighter canvas size to its maximum size;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116598/diff/3-4/

Comment 19

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8842823 [details]
Bug 1343217 - Cut the CssGridHighlighter canvas size to its maximum size;

https://reviewboard.mozilla.org/r/116598/#review120408

::: devtools/server/actors/highlighters/css-grid.js:684
(Diff revision 3)
> +      // Unfortunately that would also display the source, where if clicked , will ends
> +      // in a non-existing document.
> +      // It's not ideal, but from an highlighter there is no an easy way to show such
> +      // notification elsewhere.
> +      this.win.console.warn("The CSS Grid Highlighter could have been clipped, since " +
> +                            "the size of the document inspected.\n" +

As discussed over IRC, I will replace "since" with "due", without adding anything at the sentence.

::: devtools/server/actors/highlighters/css-grid.js:693
(Diff revision 3)
> +
>      // Resize the canvas taking the dpr into account so as to have crisp lines.
> -    this.canvas.setAttribute("width", width * ratio);
> -    this.canvas.setAttribute("height", height * ratio);
> -    this.canvas.setAttribute("style", `width:${width}px;height:${height}px;`);
> +    this.canvas.setAttribute("width", width);
> +    this.canvas.setAttribute("height", height);
> +    this.canvas.setAttribute("style",
> +      `width:${width / ratio}px;height:${height / ratio}px;`);

No, that's fine. We can't safely round the value since the rounding logic depends by the pixel ratio – and something we need to have from the platform, I've been in contact with Brad for that.

Usually this is an issue because we use SVG and canvas, but in that case is not since we're actually setting the size to an HTML element (the canvas) the platform will take care of the rounding internally, using its own logic / algorithm.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4746dc671cc2
Cut the CssGridHighlighter canvas size to its maximum size; r=pbro

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4746dc671cc2

Comment 22

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8842823 [details]
Bug 1343217 - Cut the CssGridHighlighter canvas size to its maximum size;

Approval Request Comment
[Feature/Bug causing the regression]: CSS Grid Inspector (Bug 1181227)
[User impact if declined]: The CSS Grid Inspector will crash on document where the size are greater than the maximum canvas size allowed; so nothing will be displayed; also the checkbox for the grid selected won't be checked as it should (because the crash).
[Is this code covered by automated tests?]: no.
[Has the fix been verified in Nightly?]: yes.
[Needs manual test from QE? If yes, steps to reproduce]: the STR are in the description of this bug.
[List of other uplifts needed for the feature/fix]: Bug 1312103
[Is the change risky?]: it shouldn't.
[Why is the change risky/not risky?]: we just make some calculation to reduce the size of the canvas; that will avoid the inspector to crash.
[String changes made/needed]: none.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

We shipped this feature in 52. Marking 53/54 as affected.

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8842823 [details]
Bug 1343217 - Cut the CssGridHighlighter canvas size to its maximum size;

Let's take the quick fix for beta 2 and test it there.

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Andrei can your team test next week when this lands in beta? Thanks!

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8842823 [details]
Bug 1343217 - Cut the CssGridHighlighter canvas size to its maximum size;

This wasn't nominated for Aurora approval for some reason?

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/20b587aa00a8

Comment 28

[Gerry Chang [:gchang]]

Comment on attachment 8842823 [details]
Bug 1343217 - Cut the CssGridHighlighter canvas size to its maximum size;

We need this in 54. Aurora54+.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1f56b8617108

Comment 30

[Andrei Vaida [:avaida]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #25)
> Andrei can your team test next week when this lands in beta? Thanks!

Added to our todo list. Keeping the ni? in place until this is done.

Comment 31

[Bogdan Maris, Desktop Test Engineering]

Verified that this is fixed using Firefox 53 beta 2, latest Developer Edition 54.0a2 and latest Nightly 55.0a1 on Windows 7 64bit, macOS 10.12.3 and Ubuntu 16.04 32bit.