1601306 - Default themes are not applied correctly when they are switched from about:addons on Mac OS

Status: VERIFIED FIXED

(Core :: Web Painting, defect)

Description

[Valentina Virlics [:vvalentina], Ecosystem QA]

[Affected Versions]:

• Firefox Beta 72.0b2

[Affected Platforms]:

• All Mac

[Steps to reproduce]:

1. Go to about:addons#Themes page
2. Enable Light theme
3. Switch to Standard theme
4. Observe tabs strip behavior

[Expected results]:

• The new theme is applied and there are no display or layout issues.

[Actual results]:

• The theme is not applied correctly on the tab strip.

[Regression]:
23:31.26 INFO: No more inbound revisions, bisection finished.
23:31.26 INFO: Last good revision: 12df7898b0eee9b11ed6189769f2a6199d93d4e1
23:31.26 INFO: First bad revision: 4d9f24928383f3da9fe40a2900a403ceb76c5983
23:31.26 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=12df7898b0eee9b11ed6189769f2a6199d93d4e1&tochange=4d9f24928383f3da9fe40a2900a403ceb76c5983

It looks like bug 1592739 has caused this behavior.

[Notes]

• Issue observed better while hovering over the tab strip.

@mstange, could you please take a look at this?

Comment 1

[Tim Nguyen :ntim]

Valentina, do you have a screenshot by any chance ? I can't reproduce this on my side.

Also, do you have WebRender enabled ?

Thanks!

Comment 2

[Markus Stange [:mstange]]

I can reproduce this, but only on about:addons, not from the Customize... tab.

Comment 3

[Markus Stange [:mstange]]

Attachment: Screen Shot 2019-12-04 at 12.45.12 PM.png

Comment 4

[Markus Stange [:mstange]]

The PaintedLayer for the chrome contains opaque pixels instead of transparent pixels in the bad paints. That's even though the entire layer is invalidated and there's no SolidColor item in the display list after the switch from light -> default. I'm not sure where the opaque pixels come from. It's possible that we're forgetting to clear previous layer content. The PaintedLayer itself is not marked as opaque. But sometimes its parent layer (the ClientLayerManager's root container layer) is marked as opaque.

This bug does not happen with WebRender enabled.

Comment 5

[Markus Stange [:mstange]]

Attachment: display-dump.log

Comment 6

[Markus Stange [:mstange]]

Attachment: display-dump.log

Comment 7

[Markus Stange [:mstange]]

This has to do with mIsOpaque on the window's root display list. Its value changes between true and false somewhat randomly, as far as I can tell. If mIsOpaque is false in the paint where we change from the light theme to the default theme, we get broken rendering. If it happens to be true, we get correct rendering.

Comment 8

[Markus Stange [:mstange]]

This is a retained display list bug. Sometimes mIsOpaque=true from a previous frame can stick around, and then the bad value circles back through layers. (displayList->IsOpaque() causes us to set CONTENT_OPAQUE on a layer, and then the opaque layer region results in a call to displaylist->SetIsOpaque()).
This bug does not happen if layout.display-list.retain.chrome is set to false.

Comment 9

[Markus Stange [:mstange]]

Attachment: Bug 1601306 - When partial update fails, make sure that mIsOpaque and mForceTransparentSurface on the root display list are false. r=tnikkel

These values start out false in the other two ways display lists can be build:

• Without retained display lists, a new nsDisplayList object is created which starts out with these values being false.
• When partial update succeeds (or at least gets far enough to run PreProcessDisplayList), RestoreState() is called on the root display list which sets these values to false.

But when partial update fails, we have an old nsDisplayList object and we were not clearing those values.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17464983f090
When partial update fails, make sure that mIsOpaque and mForceTransparentSurface on the root display list are false. r=tnikkel

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/17464983f090

Comment 12

[Tim Nguyen :ntim]

Should this be uplifted to 72?

Comment 13

[Alin Ilea, Desktop Test Engineering]

Reproduced the initial issue using an old Nightly build: 20191204215924 and Beta 72.0b2.
Verified - fixed on latest Nightly 73.0a1 (2019-12-05) (Build id: 20191205041108) on Mac OS 10.14.

Comment 14

[Markus Stange [:mstange]]

Comment on attachment 9113597 [details]
Bug 1601306 - When partial update fails, make sure that mIsOpaque and mForceTransparentSurface on the root display list are false. r=tnikkel

Beta/Release Uplift Approval Request

• User impact if declined: UI glitches when switching themes
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: see comment 0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The fix is very simple.
• String changes made/needed:

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 9113597 [details]
Bug 1601306 - When partial update fails, make sure that mIsOpaque and mForceTransparentSurface on the root display list are false. r=tnikkel

macos regression fix, verified in nightly, approved for 72.0b5

Comment 16

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1197cc3af199

Comment 17

[Alin Ilea, Desktop Test Engineering]

Verified - fixed on latest Beta 72.0b5 (Build id: 20191209175918) on Mac OS 10.14.