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

VERIFIED FIXED in Firefox 58

Status

()

defect
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: abenson, Assigned: jnicol)

Tracking

58 Branch
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 unaffected, firefox58+ verified, firefox59 verified)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments)

Reporter

Description

2 years ago
Posted image macos_tab_bug.png
No description provided.
[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.
Blocks: 1092294
Component: Widget: Cocoa → Graphics: Layers
Summary: [10.13] After closing a tab, remaining tab labels turn white along with tab separators → After closing a tab, remaining tab labels turn white along with tab separators
Version: 57 Branch → 58 Branch
Based on comment 1, ni :jnicol.
Flags: needinfo?(jnicol)
Assignee

Comment 3

2 years ago
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.
Flags: needinfo?(jnicol)
Comment hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
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]
Tracking 58+ for this visual issue.
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.
Attachment #8924523 - Flags: review?(mstange)
Comment hidden (mozreview-request)
Assignee

Comment 9

2 years ago
mozreview-review-reply
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 on attachment 8924523 [details]
Bug 1413693 - Ensure data copied from discarded front buffer isn't also painted.

https://reviewboard.mozilla.org/r/195786/#review201110
Attachment #8924523 - Flags: review?(mstange) → review+

Comment 11

2 years ago
Pushed by jnicol@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6f7cae36d92d
Ensure data copied from discarded front buffer isn't also painted. r=mstange

Updated

2 years ago
Assignee: nobody → jnicol
Status: NEW → ASSIGNED
Whiteboard: [gfx-noted]

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6f7cae36d92d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.