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

VERIFIED FIXED in Firefox 53

Status

()

Firefox
Developer Tools: Inspector
P1
normal
VERIFIED FIXED
11 months ago
10 months ago

People

(Reporter: jdescottes, Assigned: zer0)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 wontfix, firefox53 verified, firefox54 verified, firefox55 verified)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

11 months ago
str
(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
(Reporter)

Comment 1

11 months ago
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?
Flags: needinfo?(zer0)
(Assignee)

Comment 2

11 months ago
(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).
Flags: needinfo?(zer0)
(Assignee)

Comment 3

11 months ago
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 hidden (mozreview-request)
(Assignee)

Comment 5

11 months ago
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.
Attachment #8842823 - Flags: feedback?(pbrosset)
Attachment #8842823 - Flags: feedback?(jdescottes)
Attachment #8842823 - Flags: feedback?(bwerth)
(Assignee)

Updated

11 months ago
Assignee: nobody → zer0

Comment 6

11 months ago
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.
Priority: -- → P1

Comment 7

11 months ago
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.
Attachment #8842823 - Flags: feedback?(bwerth) → feedback+

Comment 8

11 months ago
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.
Attachment #8842823 - Flags: feedback?(pbrosset) → feedback+
(Reporter)

Comment 9

11 months ago
mozreview-review
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 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.
Attachment #8842823 - Flags: feedback?(jdescottes) → feedback+
Comment hidden (mozreview-request)
(Assignee)

Comment 12

11 months ago
mozreview-review-reply
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 hidden (mozreview-request)
(Assignee)

Comment 14

11 months ago
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.
Attachment #8842823 - Flags: feedback?(jdescottes)

Updated

11 months ago
Summary: Grid Highlighter fails on http://gridbyexample.com/examples/ → Grid Inspector fails on http://gridbyexample.com/examples/
(Assignee)

Updated

11 months ago
Status: NEW → ASSIGNED
(Assignee)

Updated

11 months ago
See Also: → bug 1345434
(Reporter)

Comment 15

11 months ago
mozreview-review
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 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.
Attachment #8842823 - Flags: feedback?(jdescottes) → feedback+

Comment 17

11 months ago
mozreview-review
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.
Attachment #8842823 - Flags: review?(pbrosset) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 19

11 months ago
mozreview-review
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

11 months ago
Pushed by mferretti@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4746dc671cc2
Cut the CssGridHighlighter canvas size to its maximum size; r=pbro

Comment 21

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4746dc671cc2
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Assignee)

Comment 22

11 months ago
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.
Attachment #8842823 - Flags: approval-mozilla-beta?
We shipped this feature in 52. Marking 53/54 as affected.
status-firefox52: --- → wontfix
status-firefox53: --- → affected
status-firefox54: --- → affected
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.
Attachment #8842823 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Andrei can your team test next week when this lands in beta? Thanks!
Flags: needinfo?(andrei.vaida)
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?
Attachment #8842823 - Flags: approval-mozilla-aurora?

Comment 27

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/20b587aa00a8
status-firefox53: affected → fixed

Comment 28

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

We need this in 54. Aurora54+.
Attachment #8842823 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 29

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/1f56b8617108
status-firefox54: affected → fixed
(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.
Flags: qe-verify+
QA Contact: bogdan.maris
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.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
status-firefox54: fixed → verified
status-firefox55: fixed → verified
Flags: qe-verify+
Flags: needinfo?(andrei.vaida)
(Reporter)

Updated

10 months ago
Depends on: 1348293
You need to log in before you can comment on or make changes to this bug.