Scrolling the debugger editor hangs the browser

VERIFIED FIXED in Firefox 66

Status

()

defect
--
major
VERIFIED FIXED
6 months ago
6 months ago

People

(Reporter: jlast, Assigned: rhunt)

Tracking

(Blocks 1 bug, {regression})

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox64 unaffected, firefox65 unaffected, firefox66 verified)

Details

Attachments

(3 attachments)

STR:

  1. open devtools
  2. select the debugger
  3. select a source in the debugger source tree
  4. scroll the source --> beach balls, crashes

I'm using Mac OS X 10.13.6


I noticed while bisecting that this patchset introduced the problem
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fbe6548db11ded24b5221180719ce66e785dc3c6&tochange=ad851d4345c08f7e0e5d5578652004194a6e667f

Flags: needinfo?(rhunt)

(In reply to Jason Laster [:jlast] from comment #0)

STR:

  1. open devtools
  2. select the debugger
  3. select a source in the debugger source tree
  4. scroll the source --> beach balls, crashes

I'm using Mac OS X 10.13.6


I noticed while bisecting that this patchset introduced the problem
https://hg.mozilla.org/integration/mozilla-inbound/
pushloghtml?fromchange=fbe6548db11ded24b5221180719ce66e785dc3c6&tochange=ad85
1d4345c08f7e0e5d5578652004194a6e667f

Do you have a crash report link? I'll spin up a debug build now.

hmm, so disabling layout.css.scroll-anchoring.enabled also fixes it... let me look for a crash.

Looking into it now. Looks like some kind of feedback loop of scroll events -> reflow -> ?? -> scroll events.

Assignee: nobody → rhunt
Flags: needinfo?(rhunt)
Summary: Scrolling the debugger editor crashes the browser → Scrolling the debugger editor hangs the browser

I was able to reproduce, after a minute or so of attempting (doesn't happen right away for me). I was using https://www.mozilla.org/en-US/firefox/nightly/firstrun/ as my testcase, and choosing "analytics.js" from google analytics as the JS source to scroll through.

When it started hanging, I killed Firefox with "kill -11 [pid]" to generate a crash report.

My crash reports were in very different spots:
bp-235abc18-5a1a-49ed-a70f-245160190111
bp-387f801f-9100-4db4-87c6-f15d10190111

The first one is in scroll events -> JS -> .offsetTop -> flush restyles -> reconstruct frames
The second is in a .offsetTop-triggered restyle, without symbolicated caller info unfortunately (I'm guessing it's also scroll events -> JS)

Here's a profile during the hang:
https://perfht.ml/2QEGl7c

(A slow-script dialog appeared at about 13s in this profile, which let me regain control of my browser & capture the profile.)

I accidentally clobbered my debug build, but once it's built I'll take a look at the scroll anchor log. I think that will help here.

Jason also pointed me to some scroll handlers [1] [2] [3]. I'll also try disabling those to narrow this down.

[1] https://github.com/devtools-html/debugger.html/blob/master/src/components/Editor/ConditionalPanel.js#L139-L140
[2] https://github.com/devtools-html/debugger.html/blob/master/src/components/Editor/Preview/index.js#L83
[3] https://github.com/devtools-html/debugger.html/blob/master/src/components/Editor/index.js#L192

Alright, figured it out. We're consuming scroll anchor suppressions during restyling and missing it after reflow. This causes us to enter the feedback loop.

I've got a patch to fix this, but I need to clean it up first. It's very similar to bug 1518926, and I'll probably fix that at the same time.

As an aside, CodeMirror is doing some pretty custom things and reading through the source is pretty difficult.

It doesn't look like CodeMirror needs anything scroll anchoring related, so adding an 'overflow-anchor: none' appropriately could probably save us some work. We still need to fix this issue though.

We currently perform anchor adjustment in three spots:
  1. If the target of RestyleManager::RecomputePosition is in a scroll anchor chain
  2. If the reflow root is in a scroll anchor chain
  3. In nsHTMLScrollFrame::DidReflow, for itself

It looks like it's possible for a scroll anchor container to be adjusted by (1)
and (2 or 3) in the same PresShell flush.

This should be okay, except that we consume mSuppressAnchorAdjustment when
performing an adjustment, and this can lead us to miss the second time that
we perform adjustments in a PresShell flush.

This commit reworks how we run anchor adjustments so that we collect all
scroll anchor containers that should be adjusted, and only perform the
adjustments once.

Depends on D16406
Duplicate of this bug: 1518926
Duplicate of this bug: 1519821
Severity: normal → major
Keywords: regression

Some people have noticed that if during a page load, they scroll to the bottom of the page (such as with the 'End' key), scroll anchoring can bounce them back to the top.

I've found I can reproduce this consistently on nightly by going to [1], opening the 'Network' devtools panel, toggling throttling to a low enough level that page load takes some time, performing a shift-reload, scrolling to end when I see the page is transitioning to the reloaded version.

I also found that it's fixed with my local scroll anchoring fixes. Backing out the patch attached to this bug, brought it back. So it looks like it fixes that issue as well.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1305957

Is this bug specifically about Dev Tools Debugger or maybe CodeMirror in general? Because GitHub uses CodeMirror for editing and I experience browser freezes when editing some long files.

Can be reproduced on CodeMirror demo page: https://codemirror.net/demo/merge.html - just start scrolling using mouse wheel.

This also happens in other demos, but requires text with lot of lines. For example copy text from this page into https://codemirror.net/demo/search.html .

(In reply to gwarser from comment #13)

Is this bug specifically about Dev Tools Debugger or maybe CodeMirror in
general? Because GitHub uses CodeMirror for editing and I experience browser
freezes when editing some long files.

Can be reproduced on CodeMirror demo page:
https://codemirror.net/demo/merge.html - just start scrolling using mouse
wheel.

This also happens in other demos, but requires text with lot of lines. For
example copy text from this page into
https://codemirror.net/demo/search.html .

This probably affects all CodeMirror embeddings.

See Also: → 1520095
Posted file reduced-test.html

Here's a version of the testcase that doesn't rely on testharness.js, FWIW (so it can just be run directly on bugzilla).

Blocks: 1519798
Blocks: 1520095
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e7a368e513a
Coalesce all scroll anchor adjustments to be performed after layout when flushing notifcations. r=dholbert
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14929 for changes under testing/web-platform/tests

Reproduced issue with Firefox 66.0a1(2019-01-11).
Fix verified with 66.0b3 on Windows 10, macOS 10.11.6, Ubuntu 18.04.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.