1847440 - getTextAtLineColumn causes Firefox to hang (astro dev)

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[James]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.5.2 Safari/605.1.15

Steps to reproduce:

Inspected an element on a development version of a small Astro.js website.

This affects both Firefox and Firefox Developer Edition (117.0b3) on macOS. I can reproduce with a clean profile, and refreshing Firefox makes no difference.

Actual results:

Opening the inspector causes 100% CPU usage and an unresponsive UI while executing the regular expression in getTextAtLineColumn.

https://hg.mozilla.org/mozilla-central/file/tip/devtools/server/actors/utils/style-utils.js#l197
https://share.firefox.dev/45fkppY

Expected results:

The inspector should open nearly instantly, selecting the target element ready for inspection.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::DOM: Core & HTML' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Julian Descottes [:jdescottes]]

Thanks for filing!

I tried inspecting a few websites built with Astro.js, but could not reproduce the issue. Is there any way you can share your website with us?

Comment 3

[James]

I've extracted the home page from my website and pushed it up to GitHub here:

https://github.com/jcf/mini-me

I'm able to reproduce the issue when running in development. When I build the full site and push to Vercel, the problem goes away.

If you clone the repo, you can start up Astro with pnpm dev. The dev version should start up on port 3000.

Comment 4

[Julian Descottes [:jdescottes]]

Thanks for sharing, will test your scenario!

Comment 5

[Julian Descottes [:jdescottes]]

It seems that we are not computing the correct line number for the rules of the stylesheets used in this page. And then we end up trying to apply an invalid regex to extract the rule text.

It's not related to the stylesheet itself, because a static page using the same inline sheets doesn't show the same issue. I guess the hot reload mechanism used by astro/vite must confuse our helpers.

On DevTools side, the line number is computed at:
https://searchfox.org/mozilla-central/rev/dc8348b3730c0d29dafd01c653d9151eaa9bc30f/devtools/server/actors/style-rule.js#87

this.line = InspectorUtils.getRelativeRuleLine(this.rawRule);

And it returns a value which doesn't match the actual stylesheet. For instance, for a stylesheet which contains less than 1000 lines, it might return something above 1500, as if it was applying an offset.

I checked a bit the GetRelativeRuleLine implementation in InspectorUtils, and the lineNumber returned by aRule.GetLineNumber(); seems to have an offset and the value is wrong (eg 1895), but linkLineIndex0 is 0 so we don't manage to correct it.

Emilio, I can see there is already a comment about relative line numbers and dynamically inserted rules in this method. Do you think we might be hitting an edge case here?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

I can repro and can look into what might be going on, but... Is this regex really the fastest way of getting the nth line?

Also, does DevTools use the line+column for something else than this? Because if not we could hand you the utf-8 byte offset (or even utf-16 code point, maybe) where the rule starts, which seems to be what you're trying to compute?

Comment 7

[Julian Descottes [:jdescottes]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

I can repro and can look into what might be going on, but... Is this regex really the fastest way of getting the nth line?

Also, does DevTools use the line+column for something else than this? Because if not we could hand you the utf-8 byte offset (or even utf-16 code point, maybe) where the rule starts, which seems to be what you're trying to compute?

It's probably not the best solution, it's been implemented quite a long time ago, and I don't think it was discussed much.
We could definitely use the offset for getTextAtLineColumn, and if it's correct it would fix the issue here.

From a quick check, we also use the line number to display it in the ruleview. (eg inline:1780) and clicking on this link jumps to the style editor at the specified line. So we would still need some way to compute the correct line.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(Confirming since Julian and I have reproduced it)

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1847440 - Part 1: Cache failure in getAuthoredCssText(). r=jdescottes,nchevobbe

If our line numbers etc are wrong, then we might throw and retry later.

Given the empty string already represents failure, make
getAuthoredCssText not throw and just return the empty string on
failure.

This avoids retrying over and over just to fail. It doesn't fix the
initial hang, but makes it more manageable.

Will still try to find a proper fix / root cause of course, but this
seems harmless enough either way.

Also expand canSetRuleText so that it's easier to reason about.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1847440 - Always store relative line numbers in CSS rules. r=nchevobbe,jdescottes

CSS rules were storing absolute rather than relative line numbers (this
was done to match the old style system).

So when we hit the cached inline stylesheet code-path, for which we
share the CSS rules of the stylesheet, for the cache hit the line
numbers were completely off.

This particular page was probably regressed by bug 1834081, but the
issue could happen before with Shadow DOM.

Always store relative numbers and convert to absolute when asked by the
inspector, rather than the other way around.

This is simpler and makes the cache work.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1847440 - Part 0: Split canSetRuleText so that it's easier to follow. r=nchevobbe

No behavior change.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1847440 - Part 2: Simplify some callers now that getAuthoredCssText doesn't throw. r=nchevobbe

Depends on D185906

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1847440 - Other misc cleanups in style-rule.js. r=nchevobbe

No need to explicitly return a promise in async functions.

Depends on D185916

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d94cc9b0a86
Part 0: Split canSetRuleText so that it's easier to follow. r=nchevobbe,devtools-reviewers
https://hg.mozilla.org/integration/autoland/rev/8e19ac289028
Part 1: Cache failure in getAuthoredCssText(). r=nchevobbe,devtools-reviewers
https://hg.mozilla.org/integration/autoland/rev/e210cb558e6c
Part 2: Simplify some callers now that getAuthoredCssText doesn't throw. r=nchevobbe,devtools-reviewers

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d01d82bb998
Other misc cleanups in style-rule.js. r=nchevobbe,devtools-reviewers

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc9f0909aa39
Always store relative line numbers in CSS rules. r=zrhoffman

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5bbfdb7e9277
Fix line numbers in CSS error reports.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f564d87edcf1
Fix expectation in browser_rules_media_queries now that constructable stylesheets use 1-based line numbers like everything else.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58406f20a08f
Fix another test.

Comment 20

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/2d94cc9b0a86
https://hg.mozilla.org/mozilla-central/rev/8e19ac289028
https://hg.mozilla.org/mozilla-central/rev/e210cb558e6c
https://hg.mozilla.org/mozilla-central/rev/6d01d82bb998
https://hg.mozilla.org/mozilla-central/rev/dc9f0909aa39
https://hg.mozilla.org/mozilla-central/rev/5bbfdb7e9277
https://hg.mozilla.org/mozilla-central/rev/f564d87edcf1
https://hg.mozilla.org/mozilla-central/rev/58406f20a08f

Comment 21

[Sebastian Zartner [:sebo]]

FYI, this patch (specifically https://hg.mozilla.org/mozilla-central/rev/dc9f0909aa39) fixed a bug related to incorrectly displayed properties inside rules.

My STR were:

1. On https://baldursgate3.game/
2. Inspect the element with class .frame__corner--left-top
3. In the Rules View look at the .frame__corner[data-v-7a211648] rule

It was displayed as

.frame__corner[data-v-7a211648] {
  height: 1015px;
  position: relative;
}

Now it's correctly displayed as

.frame__corner[data-v-7a211648] {
  position: absolute;
  width: 24px;
  height: 24px;
  background-image: url(/svg/corner.svg);
  background-size: contain;
  background-position: 50%;
}

(Wanted to file a bug for that but saw that it was already fixed in Nightly. So, running mozregression then brought me here.)

Sebastian