1413693 - After closing a tab, remaining tab labels turn white along with tab separators

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[Aaron Benson]

Attachment: macos_tab_bug.png

Comment 1

[Markus Stange [:mstange]]

[Tracking Requested - why for this release]: very annoying glitch whenever the tab bar scrolls when using the default theme on macOS

82:17.81 INFO: No more inbound revisions, bisection finished.
82:17.81 INFO: Last good revision: 97031d4ea2bd29ca2d5f21ad76ef37e3ec4069d3
82:17.81 INFO: First bad revision: 2d2922ab35d243657108303c7262f4c2793cbe86
82:17.81 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=97031d4ea2bd29ca2d5f21ad76ef37e3ec4069d3&tochange=2d2922ab35d243657108303c7262f4c2793cbe86

This was caused by bug 1092294.

Comment 2

[Ethan Lin[:ethlin]]

Based on comment 1, ni :jnicol.

Comment 3

[Jamie Nicol [:jnicol]]

I wanted to move the paintRegion.SubOut call to only occur if copying from both the discardedFrontBuffer and discardedFrontBuffer on white was successful, to ensure that if the latter failed it would still be painted in the new backbuffer.

But my logic was wrong - this means that when there is no onWhite versions, we no longer SubOut ever. So the backbuffer is both copied to from the discardedFrontBuffer, and then painted to later on in the function.

Need to rejig it a wee bit so we get all required locks (front, and frontOnWhite if necessary) first then either copy to all buffers (back and backOnWhite if necessary) or none.

Comment 4

[Jamie Nicol [:jnicol]]

Attachment: Bug 1413693 - Ensure data copied from discarded front buffer isn't also painted.

Bug 1092294 introduced a regression in to the code to copy from
the discarded front buffers to the new backbuffers in
SingleTiledContentClient. The aim was to ensure that if locking the
the frontbuffers failed, meaning the region could not be copied, that it
would be painted instead. However due to incorrect logic the
region would both be copied and painted in cases where
there was no onWhite buffers.

To fix this we take both locks (frontLock, and frontOnWhiteLock if
required) up front, so that we either copy both buffers or neither.

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

Comment 5

[Code Review Bot [:reviewbot]]

Comment on attachment 8924523 [details]
Bug 1413693 - Ensure data copied from discarded front buffer isn't also painted.

https://reviewboard.mozilla.org/r/195786/#review200948


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: gfx/layers/client/SingleTiledContentClient.cpp:205
(Diff revision 1)
>      copyableRegion.SubOut(aDirtyRegion);
>  
>      if (!copyableRegion.IsEmpty()) {
>        TextureClientAutoLock frontLock(discardedFrontBuffer,
>                                        OpenMode::OPEN_READ);
> -      if (frontLock.Succeeded()) {
> +      Maybe<TextureClientAutoLock> frontOnWhiteLock = Nothing();

Error: Variable of type 'maybe<mozilla::layers::textureclientautolock>' is not valid in a temporary [clang-tidy: mozilla-scope]

Comment 6

[Marcia Knous [:marcia]]

Tracking 58+ for this visual issue.

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8924523 [details]
Bug 1413693 - Ensure data copied from discarded front buffer isn't also painted.

https://reviewboard.mozilla.org/r/195786/#review201080

This probably needs a different approach if Maybe<TextureClientAutoLock> is not accepted.

Comment 8

[Jamie Nicol [:jnicol]]

Comment on attachment 8924523 [details]
Bug 1413693 - Ensure data copied from discarded front buffer isn't also painted.

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

Comment 9

[Jamie Nicol [:jnicol]]

Comment on attachment 8924523 [details]
Bug 1413693 - Ensure data copied from discarded front buffer isn't also painted.

https://reviewboard.mozilla.org/r/195786/#review200948

> Error: Variable of type 'maybe<mozilla::layers::textureclientautolock>' is not valid in a temporary [clang-tidy: mozilla-scope]

Fixed by using the default constructor of Maybe, avoiding the temporary Nothing.

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8924523 [details]
Bug 1413693 - Ensure data copied from discarded front buffer isn't also painted.

https://reviewboard.mozilla.org/r/195786/#review201110

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f7cae36d92d
Ensure data copied from discarded front buffer isn't also painted. r=mstange

Comment 12

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/6f7cae36d92d

Comment 13

[Oana Botisan, Desktop QA]

I managed to reproduce the bug using an older version of Firefox (2017-11-01) on macOS 10.13. 
I retested everything using latest Nightly 59.0a1 and beta 58.0b4, but the bug is not reproducing anymore.