1391996 - webrendest: regression: text is partially invisible on about:preferences, about:crashes etc.

Status: VERIFIED FIXED

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

Description

[Darkspirit]

Attachment: regression_20170819.png

Nightly 57 x64 20170819100442 @ Debian Testing

> mozregression --good 2017-08-18 --bad 2017-08-19 --profile-persistence clone-first
> [...]
> 9:11.06 INFO: Last good revision: 834821ca057915b9458ad7c76a130da4c2b59782
> 9:11.06 INFO: First bad revision: 564959d26e8db243ded3d57380842620e60e89e7
> 9:11.06 INFO: Pushlog:
> https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=834821ca057915b9458ad7c76a130da4c2b59782&tochange=564959d26e8db243ded3d57380842620e60e89e7

https://mozillagfx.wordpress.com/2017/08/16/webrender-newsletter-1/
> layers.async-pan-zoom.enabled;false
> gfx.webrender.enabled;true
> gfx.webrendest.enabled;true
> gfx.webrender.layers-free;true
> gfx.webrender.blob-images;true
> layers.acceleration.force-enabled;true

I think enabling webrender+webrendest are sufficient for showing this regression.

Comment 1

[Darkspirit]

(Websites seem fine)

Comment 2

[Darkspirit]

Confirmed in Nightly 57 x64 20170819100442 @ Windows 10.
Same settings, except gfx.webrender.blob-images: It has to be false, otherwise the window is white inside.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Alexis, can you look into this? The regression window points to your change.

Comment 4

[Aria Beingessner [:Gankra]]

The culprit is this check at the start of nsDisplayText::CreateWebRenderCommands

  if (aManager->IsLayersFreeTransaction()) {
    ContainerLayerParameters parameter;
    if (GetLayerState(aDisplayListBuilder, aManager, parameter) != LAYER_ACTIVE) {
      return false;
    }
  }

Commenting it out makes all the text properly render.

We seem to always do this check in all the CreateWebRenderCommands methods, so I'm not really sure what it's doing or why it might fail. Not sure how to proceed.

Comment 5

[Ethan Lin[:ethlin]]

(In reply to Alexis Beingessner [:Gankro] from comment #4)
> The culprit is this check at the start of
> nsDisplayText::CreateWebRenderCommands
> 
>   if (aManager->IsLayersFreeTransaction()) {
>     ContainerLayerParameters parameter;
>     if (GetLayerState(aDisplayListBuilder, aManager, parameter) !=
> LAYER_ACTIVE) {
>       return false;
>     }
>   }
> 
> Commenting it out makes all the text properly render.
> 
> We seem to always do this check in all the CreateWebRenderCommands methods,
> so I'm not really sure what it's doing or why it might fail. Not sure how to
> proceed.

I think the problem is because we suppose having 'mTextDrawer' means we could create WR commands / having text layer, so we check 'mTextDrawer' in some places, e.g.: 'nsDisplayText::TryMerge'[1]. But it doesn't match the rules in nsDisplayText::GetLayerState. We should unify the checks.


[1] https://dxr.mozilla.org/mozilla-central/rev/7dddbd85047c6dc73ddbe1e423cd643a217845b3/layout/generic/nsTextFrame.cpp#5046

Comment 6

[Ethan Lin[:ethlin]]

Attachment: Bug 1391996 - Move some checks from nsDisplayText::GetLayerState to constructor to prevent ambiguous status.

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

Comment 7

[Ethan Lin[:ethlin]]

I also remove the check of XRE_IsParentProcess() in the patch since it looks like unnecessary.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8899345 [details]
Bug 1391996 - Move some checks from nsDisplayText::GetLayerState to constructor to prevent ambiguous status.

https://reviewboard.mozilla.org/r/170562/#review175922

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06cab6f76000
Move some checks from nsDisplayText::GetLayerState to constructor to prevent ambiguous status. r=jrmuizel

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/06cab6f76000

Comment 11

[Darkspirit]

Verified fixed in Nightly 57 x64 20170823100553 @ Debian Testing.