1415485 - Make the pref layout.display-list.retain live again

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Jonathan Watt [:jwatt]]

In bug 1411132 the pref layout.display-list.retain was made non-live (restart required) because of crashes. Unfortunately that breaks doing reftest runs in RDL-on-for-tests-and-off-for-references mode.

Comment 1

[Miko Mynttinen]

There are some problems with this that I can think of immediately.

- Changing from not-retained to retained
We are missing the information from the previous paint and do not know what has changed. We need to do a full rebuild with the retained builder. The solution is the same as below.

- Changing from retained to not-retained to retained:
The modified frames remain in the modified frame lists before the retained builder removes them, so we need to change the logic for the first retained display list build to ensure that we always perform a full build initially. We can detect this by the presence of modified frames prop and/or creation of the retained builder.

- Changing from retained to not-retained:
We set nsDisplayListBuilder::DisplayListBuildingDisplayPortRect() prop on frames, and it might still be set during the not-retained build, if the frame was not modified and processed by the retained builder. The same also applies to new nsIFrame fields nsIFrame::HasOverrideDirtyRegion() and nsIFrame::IsFrameModified().
I think we could fix these by also checking that we are doing a partial build.

Comment 2

[Gary [:streetwolf52]]

The latest Nightly produces text which flashes and disappears when layout.display-list.retain is enabled.  No regression range at the moment.

Comment 3

[Gary [:streetwolf52]]

Here's a site that produces the problem: https://www.facebook.com/pg/SmarTech407/about/?ref=page_internal

Scroll up and down a few times then click on the links on the left such as Home, About, etc.  They disappear.  You can get them back by clicking somewhere else on the page.  Happens to many sites.

Comment 4

[Gary [:streetwolf52]]

Problem appears to have been corrected on the latest Inbound if not on Nightly.

Comment 5

[Miko Mynttinen]

Attachment: Bug 1415485 - Make the pref layout.display-list.retain live again

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

Comment 6

[Miko Mynttinen]

This patch ensures that we clear the frame properties if the frame has RetainedDisplayListBuilder::Cached() property set and if the display list retaining is disabled.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8927915 [details]
Bug 1415485 - Make the pref layout.display-list.retain live again

https://reviewboard.mozilla.org/r/199170/#review204254

Comment 8

[Miko Mynttinen]

Comment on attachment 8927915 [details]
Bug 1415485 - Make the pref layout.display-list.retain live again

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199170/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32958aa32cb5
Make the pref layout.display-list.retain live again r=mattwoodrow

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/32958aa32cb5

Comment 11

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

Backed out for frequently failing for failing reftest layout/reftests/async-scrolling/position-fixed-in-scroll-container.html on Linux x64 QuantumRender:

https://hg.mozilla.org/mozilla-central/rev/077422bf7047cbfc8490aaa193bef3f0af72fb6a

Range with push and failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&fromchange=f501182ed44bf1cd4ab61d17502d39bad24bd6f9&filter-searchStr=03d107ac0cb0c3d6ad8bcfdd0de0cd7ae22ed465&tochange=fbe652b8ecf7e823c0f1b10194652d1387a97df7

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: alternative patch - make the pref static for the lifetime of any given doc

Just in case this turns out to be hard to fix to make fully live, here's an alternative approach where the pref is live across document reloads. Basically the value is stored on a document's presContext when constructed and never updated across the lifetime of the document. That's not quite as nice as having it live across repaitns, but if I understand correctly it should work around the issues with the pref flipping between display list building. At any rate, it should be sufficient for reftest runs with RDL turned on for tests, off for refs.

Comment 13

[Miko Mynttinen]

Comment on attachment 8927915 [details]
Bug 1415485 - Make the pref layout.display-list.retain live again

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199170/diff/2-3/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bdab19ff2fef
Make the pref layout.display-list.retain live again r=mattwoodrow

Comment 15

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/bdab19ff2fef

Comment 16

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

Backed out for letting web platform tests in /css/CSS2/backgrounds/ frequently fail:

https://hg.mozilla.org/mozilla-central/rev/b056526be38e96b3e381b7e90cd8254ad1d96d9d

Push range with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&fromchange=9999a9025f644801944512c075c9ca7ccebb9988&filter-searchStr=a227be8d1aa3369ff9a23249c8c8cc05e6727db8&tochange=90db65abc2b66ea24692b3dc3f655068456d672d&selectedJob=145893905

Comment 17

[Miko Mynttinen]

This is yet another separate bug uncovered by this patch. :(

I'll restore the previous (what should be unnecessary) check for invalidations and attempt to fix these issues in another bug.

Comment 18

[Miko Mynttinen]

Comment on attachment 8927915 [details]
Bug 1415485 - Make the pref layout.display-list.retain live again

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199170/diff/3-4/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/72de8d8efe0d
Make the pref layout.display-list.retain live again r=mattwoodrow

Comment 20

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/72de8d8efe0d