1569724 - Canvas redraw doesn't update on screen after made hidden, then visible

Status: VERIFIED FIXED

(Core :: Graphics: WebRender, defect, P3)

Description

[Ori Avtalion (salty-horse)]

Attachment: Interactive testcase for hiding, showing, and redrawing a canvas

Tested on latest Nightly with WebRender enabled.

The attached testcase hides and shows a canvas. Clicking the canvas redraws it.
After the canvas is visible again, the next draw operation(s) is not displayed, as if nothing happens.
Subsequent draw operations after that do appear correctly.

This only happens with WebRender enabled, and with a canvas element (e.g. doing the same thing with a div works correctly).
Hiding and showing the canvas in the testcase is done by setting the "visibility" property, but the same happens by changing the "display" property, or using other tricks such as calling "replaceWith()".

Comment 1

[Darkspirit]

Debian Testing, KDE, X11

mozregression --good 2018-01-15 --bad 2018-06-15 --pref gfx.webrender.all:true browser.tabs.drawInTitlebar:true browser.startup.homepage:'https://bug1569724.bmoattachments.org/attachment.cgi?id=9081359'

4:29.76 INFO: Last good revision: 4d5c3fa215602dd850bf4a994e19afc2065d74b7 (2018-02-12)
4:29.76 INFO: First bad revision: e43f2f6ea111c2d059d95fa9a71516b869a69698 (2018-02-13)
4:29.76 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4d5c3fa215602dd850bf4a994e19afc2065d74b7&tochange=e43f2f6ea111c2d059d95fa9a71516b869a69698

5:04.51 INFO: There are no build artifacts on inbound for these changesets (they are probably too old).

Comment 2

[Ori Avtalion (salty-horse)]

In the regression range, a likely culprit may be bug 1237454: "Optimize animations in visibility:hidden elements".

Comment 3

[Jessie [:jbonisteel] pls NI]

Hiro, can you take a look to see if Bug 1237454 might have an impact here?

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Yeah, I will take a look of course, but I don't think bug 1237454 caused this because it affects only animations. The issue looks like more hit-testing issue (on WebRender), so this commit maybe?

Comment 5

[Jessie [:jbonisteel] pls NI]

Jeff - I see you were the reviewer for the commit Hiro linked to above, not sure if that might be related somehow?

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Confirmed that the revision right before bug 1436058 doesn't have the issue and the last commit of bug 1436058 has the issue.

Comment 7

[Sotaro Ikeda [:sotaro]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)

and the last commit of bug 1436058 has the issue.

The patch was originally reviewed at Bug 1432708

Comment 8

[Sotaro Ikeda [:sotaro]]

I want to take a bug.

Comment 9

[Sotaro Ikeda [:sotaro]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)

Confirmed that the revision right before bug 1436058 doesn't have the issue and the last commit of bug 1436058 has the issue.

The above was related to a root cause. AsyncImagePipelineManager::UpdateImageKeys() requests to use different TextureHost for updating rendering. And PersistentBufferProviderShared does not work well with AsyncImagePipelineManager and WR in the user case.

Comment 10

[Sotaro Ikeda [:sotaro]]

When TextureClient is managed by PersistentBufferProviderShared, AsyncImagePipelineManager expects that read lock of TextureHost is locked during rendering. Read lock prevents that TextureClient is reused during rendering. But when the problem happened, the read lock was not held.

When the canvas became hidden, WebRenderCanvasRendererAsync was destroyed and then the read lock of TextureHost was released. When the canvas became visible again, WebRenderCanvasRendererAsync was re-created and the previous TextureClient/TextureHost was rendered as initial content. But its read lock was not held. At a next canvas update, the same TextureClient wa used for canvas rendering, since it was not read locked. It broke AsyncImagePipelineManager's assumption.

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: Bug 1569724 - Do not use front TextureClient as next back buffer when PersistentBufferProviderShared::ClearCachedResources() is called

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5cd0b643ffd2
Do not use front TextureClient as next back buffer when PersistentBufferProviderShared::ClearCachedResources() is called r=nical

Comment 13

[Sotaro Ikeda [:sotaro]]

The problem does not happen on Windows. PersistentBufferProviderShared is not used when compositor supports texture direct mapping. See PersistentBufferProviderShared::Create().
https://searchfox.org/mozilla-central/source/gfx/layers/PersistentBufferProvider.cpp#106

Comment 14

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/5cd0b643ffd2

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider uplifting to Beta for Fx69 or can this ride with Fx70 to release?

Comment 16

[Sotaro Ikeda [:sotaro]]

The problem does not happen on Windows, then we could stay on Fx70.

Comment 17

[Negritas Sergiu, Desktop QA]

I have managed to reproduce the reproduce the issue using Fx 70.0a1 (2019-07-22), I can confirm that the issue is fixed on Fx 70.0b12.