1860872 - test_scroll_position_restore.html's helper, file_scroll_position_restore.html, takes forever to load in debug builds, with many many reflows

Status: ASSIGNED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

[Noticed while poking at bug 1857246 and noticing that I can reproduce super long delay in an enable-debug disable-optimize build...]

The file file_scroll_position_restore.html (a giant directory-listing HTML helper-file for test_scroll_position_restore.html) loads fairly quickly in opt builds -- like within a couple seconds. However, in a debug build, it can take 30-60 seconds at-best; at worst, sometimes >10min.

This seems to be due to pathological condition(s) causing us to render it in many many successive reflows, rather than just once. Filing this bug to investigate and hopefully improve things. (Not sure yet if we'll scope this as a band-aid-for-this-testcase vs. more general improvements.)

Two profiles from a local debug build that show the pathological >30-second-pageload-time issue, to get started here:
(1) https://share.firefox.dev/3SaVvUW
Notably, we seem to be swapping back and forth between parsing (nsHtml5TreeOperation::Append and similar) and restyle/layout. Ideally, I think we'd rather not be doing that swapping (or, we'd rather be doing it less).
The RefreshDriverTick shows "Tick Reasons: HasObservers" with Scrollbar Fade Animation being one of the observers, and 1x Style Flush Observer, and 1x Layout Flush Observer

(2) https://share.firefox.dev/494SYSo
(In this one, I toggled layout.css.visited_links_enabled and widget.gtk.overlay-scrollbars.enabled to false, on a hunch that the visitedness-query-restyles and overlay-scrollbar-fading might be involved in making us restyle/reflow so aggressively, but those changes don't seem to make a bug difference to the overall load time.)

Comment 1

[Daniel Holbert [:dholbert]]

Here's a third profile where I also turned off interruptible reflow (layout.interruptible-reflow.enabled to false), on top of having visited links and overlay scrollbars disabled as noted above for the second one:
(3) https://share.firefox.dev/3s4OISd

(This one is worse than the ones above, though don't read too much into that; it just demonstrates that it's not interruptible-reflow-interruptions that are causing us trouble here.)

Comment 2

[Daniel Holbert [:dholbert]]

Looks like the parser is getting interrupted not due to layout or events, but just due to a timeout. We're returning to the event loop (and doing a refresh driver tick, and laying out what we've got so far) via this check in nsContentSink::DidProcessATokenImpl:
https://searchfox.org/mozilla-central/rev/ffdc4971dc18e1141cb2a90c2b0b776365650270/dom/base/nsContentSink.cpp#783-786

// Check if it's time to return to the main event loop
if (PR_IntervalToMicroseconds(PR_IntervalNow()) > mCurrentParseEndTime) {
  return NS_ERROR_HTMLPARSER_INTERRUPTED;
}

mCurrentParseEndTime is dependent on content.sink.perf_parse_time which is 30000 which is in microseconds; so, that's 30 milliseconds. So, we're only letting the parser run for 30ms at a time before we force it to yield. This creates thrashing going back and forth between parsing and incremental layout.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: WIP: Bug 1860872: Give the HTML parser more time during mochitest test_scroll_position_restore.html, to address timeouts on Wayland. r?tnikkel

See the new code-comment in the test for an explanation. This patch lets the
parser run longer during this test, and reenables the test on Wayland.

(And while we're here, this patch also adds a message to this test's sole
'is()' statement; otherwise, that comparison just shows up in the logs as
"undefined assertion name" which isn't helpful.)