1419301 - Firefox crashes when Style Editor is open on certain websites

Status: VERIFIED FIXED

(DevTools :: Style Editor, defect, P1)

Description

[Aldi]

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

Steps to reproduce:

1. Open Firefox and navigate to the following website https://www.di.fm
2. On your keyboard press F12 key to open DevTools
3. Click on the Style Editor tab from the DevTools window
4. Notice that the whole browser is not responding and will eventually crash


Actual results:

Firefox crashed when Style Editor was opened on a website like https://www.di.fm


Expected results:

Style Editor should have shown the Stylesheet from the di.fm website.

Comment 1

[Soledad Penades [:sole] [:spenades]]

I can confirm it becomes unresponsive: "Nightly (Not Responding)" shows in the title bar, and can't click anywhere else. Have to force close the browser by right clicking on the icon on the dock and selecting close this window.

Tested on Windows 10 with the Nightly of 20171120.

Comment 2

[Patrick Brosset <:pbro>]

I'm trying to investigate this a little bit. But I don't have a debug build here so I don't see what crashes. I might try to do this in a minute.
In the meantime, I've found that one particular stylesheet is responsible for this:
https://cdn.audioaddict.com/di.fm/assets/application-5f5b82ffb6ee3f364b31aeab1903ae6a.css

And I've also seen that it doesn't crash for me, it becomes unresponsive for like 30 seconds, and then comes back and displays the stylesheet correctly. So it looks to me like that's a case of some very low performance code somewhere.
I'll add more comments here if I find new things.

Comment 3

[Patrick Brosset <:pbro>]

Somewhere in the middle of this stylesheet is a really long sequence of empty spaces. More than 128K of them actually.
I was curious and removed them and the problem went away.
So, I tried to pinpoint a little bit more the issue and it seems that around 70K of them the time it takes the styleeditor to display the stylesheet skyrockets.
Anywhere below that limit, the stylesheet gets displayed in a reasonable amount of time (like around 3 seconds on my machine), and as soon as we go above it, it suddenly takes 30 seconds to display. 
And it doesn't seem to go up linearly. It looks like around 70K is a threshold that makes retrieving or parsing or displaying the stylesheet take huge amount of time.

Comment 4

[Patrick Brosset <:pbro>]

One third of the time seems to be spent prettifying the CSS here https://searchfox.org/mozilla-central/rev/7f45cb7cc0229398fc99849bdc150cb6462d6966/devtools/shared/inspector/css-logic.js#155 as seen in this profile: https://perfht.ml/2AiVXIW

The rest of the time is spent in code I don't understand and I don't see what part of DevTools calls it: https://perfht.ml/2AjCLuA

Comment 5

[Patrick Brosset <:pbro>]

I manually added performance markers in the code and found out that, in fact, the prettifyCSS function alone takes all of the time. I must have been reading the profile incorrectly.

What takes time is this particular line here: https://searchfox.org/mozilla-central/rev/7f45cb7cc0229398fc99849bdc150cb6462d6966/devtools/shared/inspector/css-logic.js#162

> // remove initial and terminating HTML comments and surrounding whitespace
> text = text.replace(/(?:^\s*<!--[\r\n]*)|(?:\s*-->\s*$)/g, "");

It's easy to replicate the problem in a reduced test case like this for instance:

>   var start = performance.now();
> 
>   var text = new Array(70000).join(" ");
>   text.replace(/(?:^\s*<!--[\r\n]*)|(?:\s*-->\s*$)/g, "");
> 
>   var end = performance.now();
>   console.log(`duration for ${l} spaces`, Math.round((end - start)/1000), "sec");

This takes 30 seconds on my machine.

Comment 6

[Patrick Brosset <:pbro>]

Attachment: Bug 1419301 - Make HTML comments removal in prettifyCSS faster;

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

Comment 7

[Patrick Brosset <:pbro>]

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

Comment 8

[Tom Tromey :tromey]

Comment on attachment 8933686 [details]
Bug 1419301 - Make HTML comments removal in prettifyCSS faster;

https://reviewboard.mozilla.org/r/204622/#review210224

Thank you.  This looks good.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/489e3c08c457
Make HTML comments removal in prettifyCSS faster; r=tromey

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/489e3c08c457

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Seems like a useful patch to nominate for Beta uplift so DevEdition 58 can benefit from it. It grafts cleanly, so just needs an approval request :)

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8933686 [details]
Bug 1419301 - Make HTML comments removal in prettifyCSS faster;

Approval Request Comment

[Feature/Bug causing the regression]: This isn't a regression. The prettifyCSS function has always had that very slow edge case.

[User impact if declined]: If declined, stylesheets with a lot of whitespaces in them will be taking a long time to prettify in DevTools (dozens of seconds).

[Is this code covered by automated tests?]: Yes, I added new test cases to cover this.

[Has the fix been verified in Nightly?]: I manually verified this in nightly. This fix has not received other QA.

[Needs manual test from QE? If yes, steps to reproduce]: I don't think we need this. Fairly straightforward fix.

[List of other uplifts needed for the feature/fix]: None.

[Is the change risky?]: No I don't think it is at all. And even if it would be, it would only impact the Style Editor panel in DevTools.

[Why is the change risky/not risky?]: Not risky because this is a change to a non side effect function that only manipulates a string. The only effect it can have is the time it takes to manipulate this string, or the way it manipulates it. In any case, the string is being displayed in the Style Editor at the end.

[String changes made/needed]: None.

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8933686 [details]
Bug 1419301 - Make HTML comments removal in prettifyCSS faster;

Let's take this to fix the long loading issue in Style Editor panel if the stylesheets has a lot of whitespaces. Beta58+.

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/db8021209daf

Comment 15

[Oana Botisan, Desktop QA]

I managed to partially reproduce the bug using an older version of Nightly (2017-11-21) on Windows 10 x64. The browser didn't crash, but it did freeze for a few minutes. 

I retested everything on Firefox 58.0.1, beta 59.0b5 and latest Nightly using Windows 10 x64, mac 10.13 and Ubuntu 16.04 x64 and the browser responds immediately.

Under these circumstances can I mark this bug as verified fixed? Or can you still reproduce it?

Comment 16

[Aldi]

(In reply to Oana Botisan from comment #15)
> I managed to partially reproduce the bug using an older version of Nightly
> (2017-11-21) on Windows 10 x64. The browser didn't crash, but it did freeze
> for a few minutes. 
> 
> I retested everything on Firefox 58.0.1, beta 59.0b5 and latest Nightly
> using Windows 10 x64, mac 10.13 and Ubuntu 16.04 x64 and the browser
> responds immediately.
> 
> Under these circumstances can I mark this bug as verified fixed? Or can you
> still reproduce it?

Tested on FF 58.0.1 (64-bit) and the style-sheets are loading just fine.