1240338 - Nightly hangs and consumes 25% CPU when I try to scroll specific stylesheet in Style editor

Status: RESOLVED DUPLICATE of bug 1069183

(DevTools :: Style Editor, defect)

Description

[arni2033]

Attachment: stylesheet 1 - Nightly hangs and consumes 25percents CPU.txt

>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20160116030240
STR:
0. Set mouse option "when I rotate mouse wheel" in your OS to "scroll by 1 page"
1. Open attached "stylesheet 1", select all text there, copy it to clipboard
2. Open this "data:" url for "clear" testing
>   data:text/html,<style></style>
3. Open devtools -> Style Editor, paste the text copied in Step 1 to the existing stylesheet
4. Scroll the stylesheet to the very top, then to the bottom
  (repeat Step 4 if you haven't reproduced the bug)

Result:       
 Nightly becomes unresponsible (freezes) and nightly.exe consumes 25% CPU.

Expectations: 
 No high CPU usage. Nightly shouldn't become unresponsible.

Regression window:
> https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ffdd1a398105&tochange=dac8b4a0bd7c
I think it was caused by bug 971662, though I'm leaving keyword "regressionwindow-wanted"

Notes:
 Step 0 isn't necessary. I can reproduce it with that mouse option set to "Scroll by 3 lines"
 and stylesheet with only 24 lines with 9 "a{}"s in each line, if Style editor is resized
 so that only ~10 lines were visible.
 I just provided the most reliable STR.
 Please request a screencast if you failed to reproduce this bug.

Comment 1

[Alice0775 White]

confirmed.
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=6ab867edb95a&tochange=c469c25e0e45

Comment 2

[arni2033]

Thanks, Alice.

s/unresponsible/unresponsive/
s/nightly.exe/firefox.exe/

Comment 3

[Brian Grinstead [:bgrins]]

I can confirm without changing any mouse options:

1) Open data:text/html,<style></style>
2) paste in the text from https://bugzilla.mozilla.org/attachment.cgi?id=8708742 into the style editor
3) Scroll around a bunch

Patrick, I haven't attempted to profile this but I do see a lot of RDP traffic from highlighters and given the regression window in Comment 1 we might need to either disable the highlight feature when a sheet has too many selectors, disable it while scrolling, or something else.  Do you have some time to take a look at this?

Comment 4

[Patrick Brosset <:pbro>]

I'm investigating the issue now. 
This most certainly comes from the fact that the style-editor parses CSS in order to find out which symbol is the mouse positioned on when it moves.
In fact this isn't related to scrolling at all. If you make sure your mouse doesn't move at all (no mousemove events are emitted), and all you do is scroll, then the issue doesn't occur.
If instead, you don't scroll but just move your mouse over the stylesheet, at some stage the browser will freeze.

The culprit is most probably the _highlightSelectorAt in StyleSheetEditor.jsm, and in particular, the call to this.sourceEditor.getInfoAt(pos) in there.

I'll investigate some more and report findings here.

Comment 5

[Patrick Brosset <:pbro>]

Narrowed it down to resolveState in css-autocompleter.js.
In fact, I was able to freeze firefox by running this script in scratchpad:

function createSource(size) {
  let source = "";
  for (let i = 0; i < size; i ++) {
    source += "a {} ";
    if (i && i % 90 === 0) {
      source += "\n";
    }
  }
  return source;
}
function test() {
  const {devtools} = Cu.import("resource://devtools/shared/Loader.jsm", {});
  const {require} = devtools;
  const CSSCompleter = require("devtools/client/sourceeditor/css-autocompleter");
  
  let completer = new CSSCompleter();
  let bigSource = createSource(99000);
  return completer.resolveState(bigSource, {line: 10, ch: 10});
}
test();

There's a huge while loop in resolveState that gets called thousands of times and that seems to be the cause of the freeze.

I unfortunately don't have much more time today to experiment with a fix, and I think Tom's input on this would be beneficial.
I can see 2 ways forward: stop trying to call getInfoAt in huge stylesheets, as Brian suggested, or start experimenting with running some of the CSSCompleter logic in a worker. Most of this class has no dependencies to the DOM anyway, and is purely string parsing.
If it could run in a worker, we'd be able to set a timer in the main thread and drop everything if it took longer than a certain amount of time.

Comment 6

[Tom Tromey :tromey]

I can't reproduce this hang.  I tried both the data: URL approach and the scratchpad approach.

css-autocompleter does seem a bit inefficient.
It also seems to allocate a lot of memory, spending 30% of its time in the collector.

I'm looking into some possible speedup ideas.

Comment 7

[Tom Tromey :tromey]

(In reply to Tom Tromey :tromey from comment #6)

> I'm looking into some possible speedup ideas.

A couple of things come to mind.

There is a lot of GC activity in my profile.  Maybe this code could be changed
to use less memory, say by avoiding the line/column approach and using string
indices instead.  That could use a more efficient lexing approach -- not lexing
everything up front, running less code, and allocating fewer objects.
(If this is done then the line/column stuff can be removed from the css lexer
webidl entirely, as this is the only user).

The traverseForward/traverseBackwards stuff in getInfoAt seems obscure to me.
It may make many calls to resolveState (though presumably hitting the state cache).
It seems more straightforward to try to track state directly.

getInfoAt makes several calls to limit() where just one would seem to do.
This does a lot of work and allocates memory.


I still can't reproduce the problem, so I probably won't look at this any more.
However it's worth noting that the test in comment #5 doesn't seem to be truly
correct.  IIUC resolveState is passed an input string that is pre-truncated to the
correct line/column:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/sourceeditor/css-autocompleter.js#997

Comment 8

[Doug Turner (:dougt)]

Can we either fix this or back this out asap.  This impacts beta.

Comment 9

[Tom Tromey :tromey]

I tend to think this bug has been around since "forever" - certainly since before
my (minor) changes to css-autocompleter last year.

Also, see bug 1069183 for some more analysis.
IMO a big chunk of css-autocompleter needs to be rewritten to avoid N^2 behavior.
The patches in that bug are a start, but not sufficient.

Comment 10

[Patrick Brosset <:pbro>]

Clearing the needinfo as I think Tom answered in comment 9.

Comment 11

[Joe Walker [:jwalker]]

I'd like a better feel for how commonly people hit this, and I expect it's not common. Is the failure based on number of rules in the file, or rules on a line?

Patrick - comment 5 doesn't work for me. Could you give us some guidelines on what tips this over the edge? I think the idea of disabling fancy features for large docs seems like a get-out-of-jail-free so long as 'large' is bigger than people reasonably can manage in the style editor.

Comment 12

[Patrick Brosset <:pbro>]

(In reply to Joe Walker [:jwalker] (needinfo me or ping on irc) from comment #11)
> I'd like a better feel for how commonly people hit this, and I expect it's
> not common. Is the failure based on number of rules in the file, or rules on
> a line?
> 
> Patrick - comment 5 doesn't work for me. Could you give us some guidelines
> on what tips this over the edge? I think the idea of disabling fancy
> features for large docs seems like a get-out-of-jail-free so long as 'large'
> is bigger than people reasonably can manage in the style editor.

It's a really hard problem. I've spent time this morning reducing the test again and again until I ended up with just this:

a{} a{} a{} a{}
a{} a{} a{} a{}
a{} a{} a{} a{}
a{} a{} a{} a{}
a{} a{} a{} a{}
a{} a{} a{} a{}

And I still manage to make Firefox freeze with just this simple stylesheet.

Note that it's sometimes really hard to reproduce.
I basically just open an empty page, paste this code in the style-editor, then move my mouse around on the source code until firefox freezes.

Sometimes it may take a long time to happen, sometimes it will be faster. I haven't been able to figure out when it happens exactly. Note that it does happen without scrolling at all, just by moving the mouse. Quickly moving from one "a" to the other might do it (but it still needs to be slow enough so that the timer that we use to highlight matching elements in the page kicks in).

I haven't been able to reproduce it with fewer rules on each line, or with fewer lines.
So the number of rules does matter somehow, but this seems to have nothing to do with the fact that the stylesheet is very large.

I've tried on sites like amazon or mozilla, which have pretty huge stylesheets with numerous rules and a lot of selectors, and no matter how hard I tried, I could never reproduce this problem there.
I also haven't been able to conclude if e10s helps or not.

At this point, I've basically tried all combinations of rules, lines, selectors, stylesheets, and I haven't reached any conclusions as to what could be happening.
After some time, firefox just freezes and takes on full CPU core to do something, but I just don't know what.

So, it looks bad, but again, I haven't been able to reproduce on any real sites so far. I'll try again some more.

Sorry, this isn't a very helpful comment, but I figured I'd share my progress so far.

Comment 13

[Joe Walker [:jwalker]]

I also can't get this to break in the real world. I've got a crash on mozilla.org, but only by editing an existing sheet and replacing with the pathological ~ "a{}".repeat(90000) case from comment 0.

Comment 14

[Joe Walker [:jwalker]]

We're working on bug 1069183 and I think this should be duped to that one, which is getting appropriate attention.

Comment 15

[Patrick Brosset <:pbro>]

One thing I noticed:
I think the bug only occurs when you have several rules on the same line. And having several lines makes it easier to reproduce too.
The style-editor auto prettifies stylesheets, and so we never have several rules per line, which might be a reason why we can't reproduce this on real sites.

Anyway, thanks Joe for closing this bug as duplicate. If I find anything else, I'll share my findings on the other bug.

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Clearing status flag on dup.