1416921 - Graphical corruption at https://blog.figma.com/new-faces-at-the-figma-helm-89a353200e6b

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: screenshot 1 (poor-quality jpeg, sorry for jpeg artifacts)

STR:
 1. Visit https://blog.figma.com/new-faces-at-the-figma-helm-89a353200e6b
 2. Scroll down (with keyboard or mouse)

ACTUAL RESULTS:
On a decent fraction of pageloads (maybe around half), the page shows graphical corruption:
 - duplicated/misplaced/partially-not-painted text
 - chunks of the header image being painted behind text, in the wrong spots

EXPECTED RESULTS:
No such corruption.

Comment 1

[Daniel Holbert [:dholbert]]

I believe this is a regression, but I've had a hard time coming up with a regression range since it's not 100% reproducible.

The oldest cset I've reproduced this in so far is https://hg.mozilla.org/mozilla-central/rev/092ccc6335a5 (which is a merge commit for this merge: https://hg.mozilla.org/mozilla-central/rev/092ccc6335a5 )

I'm initially suspicious that this may be a regression from bug 1399692 (and maybe even the same underlying cause as bug 1416873 which is also marked as a regression from that bug).

Also, possibly-related -- the header-graphic at this site is pretty large (2000px by 971px, https://cdn-images-1.medium.com/max/2000/1*b3l9VyjhwiASBiy1JHTxSQ.png ).  I'm suspicious that this factor (large images) might be involved in triggering the bug here, given that part of the corruption sometimes looks like chunks of that image.

Comment 2

[Daniel Holbert [:dholbert]]

I'm using latest Nightly 59.0a1 (2017-11-13) (64-bit) on Ubuntu 17.10 (linux), BTW.

rhunt, perhaps you could take a look? (particularly if this does turn out to be a regression from bug 1399692)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: screenshot 2

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: screenshot 3 (bottom edge of text is duplicated/echoed)

The corruption is a little different every time I reproduce the bug (and it doesn't always happen, but I can usually make it happen by shift+reloading and then scrolling.  Not sure if the shift-reload is necessary -- I just use shift for good measure).

Here's an interesting case -- in this verison, the bottom edges of many lines of text are "echoed". I'm guessing this was from us failing to invalidate the correct old-region when scrolling, or something like that.

These artifacts persisted even when I change tabs away & back.

Comment 5

[Ryan Hunt [:rhunt] (on leave until early May)]

I'm able to reproduce some graphical corruption on this page, it definitely could be related to bug 1399692. I'll look into it further.

Comment 6

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: Revert to using EnsureBackBufferIfFrontBuffer (bug 1416921, )

This commit reverts DoubleBufferedContentClient to creating a back buffer
if there is an existing front buffer. This reverts back to a sequence of a
finalize frame, unrotate, and initialize new back buffer. When we are
not async painting we do each buffer operation immediately. If we are
async painting, then we collect all of the buffer operations and perform
them on the paint thread.

Note on locking: The destination for buffer copies must always be opened
with the async write flag, and the source must be unlocked.

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

Comment 7

[Ryan Hunt [:rhunt] (on leave until early May)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3e5d5a5e3eaf84682c0178de5252054d0294268

Comment 8

[Ryan Hunt [:rhunt] (on leave until early May)]

Forgot to mention in the commit, this change also reverts the changes to manipulating the syncing of back buffer rect and rotation from FinalizeFrame to ContentClientDoubleBuffered::BeginPaint. This is to just completely revert to existing behavior, minus the OMTP parts.

Comment 9

[Bas Schouten (:bas.schouten)]

Comment on attachment 8929977 [details]
Revert to using EnsureBackBufferIfFrontBuffer (bug 1416921, )

https://reviewboard.mozilla.org/r/201146/#review206360

This is a shame.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/614d705be06c
Revert to using EnsureBackBufferIfFrontBuffer (bug 1416921, r=bas)

Comment 11

[Guilherme Lima]

https://hg.mozilla.org/mozilla-central/rev/614d705be06c - landed 21 hours ago

Comment 12

[Ryan Hunt [:rhunt] (on leave until early May)]

This should be uplifted, but after a forgotten line of code is added back.

Comment 13

[Ryan Hunt [:rhunt] (on leave until early May)]

Comment on attachment 8929977 [details]
Revert to using EnsureBackBufferIfFrontBuffer (bug 1416921, )

Approval Request Comment
[Feature/Bug causing the regression]: An optimization in content client to be smarter about when to create a back buffer broke some implicit conditions in code. This bug backed out the optimization.
[User impact if declined]: User will see graphical regressions like in this bug
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: Not really, the steps in this bug can reproduce it.
[List of other uplifts needed for the feature/fix]: Uplift bug 1421871 after this bug.
[Is the change risky?]: Slightly
[Why is the change risky/not risky?]: This code is very sensitive to changes, but reverting to prior behavior is generally safe
[String changes made/needed]: None

Comment 14

[Gerry Chang [:gchang]]

Comment on attachment 8929977 [details]
Revert to using EnsureBackBufferIfFrontBuffer (bug 1416921, )

Fix a graphical corruption issue. Beta58+.

Comment 15

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

https://hg.mozilla.org/releases/mozilla-beta/rev/73a12159e3d7e829bab40a251d44f9a0e39b4286