1519462 - Scrolling the debugger editor hangs the browser

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Jason Laster [:jlast]]

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

Comment 1

[Ryan Hunt [:rhunt] (on leave until early May)]

(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.

Comment 2

[Jason Laster [:jlast]]

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

Comment 3

[Ryan Hunt [:rhunt] (on leave until early May)]

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

Comment 4

[Daniel Holbert [:dholbert]]

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)

Comment 5

[Daniel Holbert [:dholbert]]

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.)

Comment 6

[Ryan Hunt [:rhunt] (on leave until early May)]

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

Comment 7

[Ryan Hunt [:rhunt] (on leave until early May)]

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.

Comment 8

[Ryan Hunt [:rhunt] (on leave until early May)]

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.

Comment 9

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: Bug 1519462 - Coalesce all scroll anchor adjustments to be performed after layout when flushing notifcations. r?dholbert

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

Comment 12

[Ryan Hunt [:rhunt] (on leave until early May)]

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

Comment 13

[gwarser]

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 .

Comment 14

[Ryan Hunt [:rhunt] (on leave until early May)]

(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.

Comment 15

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: reduced-test.html

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase (standalone) for verification etc.

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e7a368e513a
Coalesce all scroll anchor adjustments to be performed after layout when flushing notifcations. r=dholbert

Comment 18

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/8e7a368e513a

Comment 19

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14929 for changes under testing/web-platform/tests

Comment 20

[Cristian Fogel [:cfogel]]

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