1461231 - No repaint after removing an element that has `position: fixed` (exact Firefox build is known)

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Marat Tanalin]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180510160705

Steps to reproduce:

After removing a fixed-positioned element from DOM, Firefox 61+ does not repaint window until an extra click is done anywhere on the page, or something that forces repaint — e.g. resizing Firefox window or hovering DOM nodes in Developer Tools.

AFFECTED VERSIONS

The bug was introduced in the build 20180404100127 (mozilla-central). The previous build 20180403220040 was NOT affected.

The latest tested build 20180513220424 is still affected.

Stable Firefox 60 is NOT affected, so there is a chance to prevent leaking the issue into the stable branch and affecting majority of users.

Both 64-bit and 32-bit versions of Firefox are affected.

Tested on two different physical computers with the same results.

TESTCASE

Please see the testcase URL.

Also, looks like exactly the same issue (introduced in the same Firefox build) is the reason of glitches with a Lightbox-like script used for displaying enlarged image on iXBT.com, e.g. see the first image (A08 robot photo) here (click the normal image version to see the glitches during showing its enlarged version):

https://www.ixbt.com/news/2018/05/13/boston-dynamics-spotmini.html

POSSIBLE REASONS

Based on what pushes have been made between the two days the unaffected and affected builds were built, one of these bugs may (or not) be the reason of the current bug given that they apparently affect the layout part of the Firefox engine, especially those related to invalidation (1450189 and 1443380):

Bug 1450266 -- Remove nsGlobalWindowInner::CleanUp in favor of FreeInnerObjects()
Bug 1450189 -- Don't invalidate display items when a frame gets a new style context
Bug 1443380 -- We don't need to mark frames modified when we recurse into InvalidateFrame
Bug 1442844 -- FinishAndStoreOverflow's aNewSize parameter isn't always used
Bug 1370575 -- Overpainting when composing a reply in GMail

Comment 1

[Jonathan Kew [:jfkthame]]

Mozregression brought me to:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6415ccbf739f36f71548540bd92c3b86ab9e1529&tochange=d112cf7b2b60e6244099dc3b599a2444ba0d1da3

Bug 1443027 there sounds like it might be relevant.

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Looks like Matt is on this. Removing my NI.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1461231 - Don't omit deleted items when constructing the DAG, since we need to record that we made a modification during MergeDisplayLists.

We can unconditionally delete the item though, and just put a placeholder entry into the DAG.

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

Comment 4

[Matt Woodrow (:mattwoodrow)]

Thanks Marat for the awesome reduced testcase! Made this super easy to track down.

Comment 5

[Miko Mynttinen]

Comment on attachment 8975706 [details]
Bug 1461231 - Don't omit deleted items when constructing the DAG, since we need to record that we made a modification during MergeDisplayLists.

https://reviewboard.mozilla.org/r/243936/#review250016

LGTM.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f288a2c9c439
Don't omit deleted items when constructing the DAG, since we need to record that we made a modification during MergeDisplayLists. r=miko

Comment 7

[Eliza Balazs [:ebalazs_]]

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

Comment 8

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8975706 [details]
Bug 1461231 - Don't omit deleted items when constructing the DAG, since we need to record that we made a modification during MergeDisplayLists.

Approval Request Comment
[Feature/Bug causing the regression]: Retained-dl, bug 1443027
[User impact if declined]:  If the only changes made are the removal of content, then sometime we forget to paint the change.
[Is this code covered by automated tests?]: New reftest added.
[Has the fix been verified in Nightly?]: Reftest passes!
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Just ensures that we don't take the 'no changes made' early return when a deletion change has been made.
[String changes made/needed]: None.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8975706 [details]
Bug 1461231 - Don't omit deleted items when constructing the DAG, since we need to record that we made a modification during MergeDisplayLists.

Retained display list fix needed for the feature to ship in Fx61. Approved for 61.0b6.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2932a4b5f7e7

Comment 11

[Marat Tanalin]

I don’t see the issue anymore in Firefox Beta (Dev. Edition) 61.0b6 (20180517141400), nor in the latest Nightly 62.0a1 (20180518100150), including the more complex real-world case that I originally discovered the issue with. It’s great that the issue has not leaked to stable Firefox.

Thanks, Matt and everyone involved.