1113435 - [SMS] Black flickering box appears before the keyboard pops up when composing a message

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[KTucker [:KTucker][Inactive 3/4/2016]]

A black flickering box appears before the keyboard pops up when composing a new message in SMS.

Repro Steps:
1)  Updated Flame to Build ID: 20141215040201
2)  Tap on the SMS app.
3)  Tap on the "Compose New Message" icon.
4)  Pay close attention to the bottom of the screen before the keyboard appears.

Actual:
A flickering black box appears before the keyboard pops up.

Expected:
No flickering occurs. 

Environmental Variables:
Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141218040201
Gaia: 58734e8a48157f99d5b733412b600c2e04c954fe
Gecko: 5c7a6294b82a
Gonk: e5c6b275d77ca95fb0f2051c3d2242e6e0d0e442
Version: 37.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Notes:
Repro frequency: 5/5 100%
See attached: video, logcat

Comment 1

[KTucker [:KTucker][Inactive 3/4/2016]]

Attachment: Keyboard Flicker logcat

Comment 2

[Julien Wajsberg [:julienw]]

I've seen this as well, you see this also when you enter a normal thread, so it's not at all keyboard related. Thank you for filing the bug.

If you can find a regression window for this (using an existing thread because it's easier without the keyboard), I'd be happy.

Comment 3

[Chris Kreinbring [:CKreinbring]]

Regression window
Last working
BuildID: 20141023104735
Gaia: f46d56d812480bff7f3b35e8cacbedfa4d49edc5
Gecko: 9781037ac408
Platform Version: 36.0a1
Firmware Version: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First broken
BuildID: 20141023110739
Gaia: f46d56d812480bff7f3b35e8cacbedfa4d49edc5
Gecko: d8de0d7e52e0
Platform Version: 36.0a1
Firmware Version: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Working Gaia / Broken Gecko = Repro
Gaia: f46d56d812480bff7f3b35e8cacbedfa4d49edc5
Gecko: d8de0d7e52e0
Broken Gaia / Working Gecko = No repro
Gaia: f46d56d812480bff7f3b35e8cacbedfa4d49edc5
Gecko: 9781037ac408
Gecko pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9781037ac408&tochange=d8de0d7e52e0


Mozilla Inbound
Last working
BuildID: 20141020171137
Gaia: dc496d04907dd314f9736ff78bab3bd27156f79a
Gecko: 223e2f4b0d47
Platform Version: 36.0a1
Firmware Version: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First broken
BuildID: 20141020172632
Gaia: dc496d04907dd314f9736ff78bab3bd27156f79a
Gecko: fa9c6845338e
Platform Version: 36.0a1
Firmware Version: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Working Gaia / Broken Gecko = Repro
Gaia: dc496d04907dd314f9736ff78bab3bd27156f79a
Gecko: fa9c6845338e
Broken Gaia / Working Gecko = No repro
Gaia: dc496d04907dd314f9736ff78bab3bd27156f79a
Gecko: 223e2f4b0d47
Gecko pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=223e2f4b0d47&tochange=fa9c6845338e

Comment 4

[Joshua Mitchell (Inactive)]

Caused by the patch to Bug 1085223 - can you take a look Matt

Comment 5

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: regression. very ugly.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Benoit, I'm not sure if Matt has the bandwidth with MSE, so can you take a look, you reviewed the original patch...

Comment 7

[Benoit Girard (:BenWa)]

I'm already assigned to a series of very similar bug. There's several things going wrong here that I'm going to be addressing in pieces. I'll leave this assigned to me until I can properly dupe it.

Comment 8

[Benoit Girard (:BenWa)]

Bug 1112332 will help. It will fix it for the given STR since we're not scrolling. But we still need to deal with making sure we cull correctly when we hit paint heuristics.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

The fix for bug 1112332 may end up dealing with bug 1114731 as well, let's keep an eye on it (the black rectangle part towards the end of the video.)

Comment 10

[Oleg Zasypkin [:azasypkin]]

Attachment: Black rectangle in Thread List

Hey, I also see black rectangle at the top when I navigate from Thread to Thread List panel in SMS using the most recent PVT build.

Not sure if it's the same issue as discussed in this bug, so attaching video just in case.

Comment 11

[Benoit Girard (:BenWa)]

Thanks. Yes it's the same one. They're a little all over b2g, moving to getting this fixed ASAP.

Comment 14

[Benoit Girard (:BenWa)]

Need to check if this is fixed.

Comment 15

[Julien Wajsberg [:julienw]]

I think I've seen it just today with the latest build from pvtbuilds :(

Comment 16

[Benoit Girard (:BenWa)]

I can still reproduce this 100% too. Still a bug. I'll pick this up tomorrow.

Should keep an eye on bug 1129467 for this.

Comment 17

[Benoit Girard (:BenWa)]

It's not bug 1129467
http://people.mozilla.org/~bgirard/cleopatra/#report=24c6f342c1e24830847051e942b55cd2ebc4fbd1&filter=[{"type"%3A"RangeSampleFilter","start"%3A3743,"end"%3A5169}]

Comment 18

[Julien Wajsberg [:julienw]]

I don't know if this is related, I see also something similar in the Settings app when entering subpanels. Sometimes white, sometimes black.

Comment 19

[Benoit Girard (:BenWa)]

This is fixed by comment out ApplyOcclusionCulling.

I'd rather we fix ApplyOcclusionCulling but if it comes to it the risk of disabling ApplyOcclusionCulling is nearly zero so we can take that to ship 2.2.

Comment 20

[Benoit Girard (:BenWa)]

Ok, I found the bug. When drawing progressively we no longer adjust the layer' visible region, only the compositable.

When we cull we only look at the layers' visible region which isn't opaque in the case of progressive drawing. Thus we punch a hole in the bottom layer and get garbage.

I'm a bit surprised that the garbage I see is always the same however.

Comment 21

[Benoit Girard (:BenWa)]

Attachment: Simple culling fix (to be uplifted)

See Comment 20 for an explanation of the fix.

Comment 22

[Benoit Girard (:BenWa)]

We could also fix this by having the progressive draw update the shadow visible region. That change is however more complex and regression prone.

Comment 23

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8560150 [details] [diff] [review]
Simple culling fix (to be uplifted)

Review of attachment 8560150 [details] [diff] [review]:
-----------------------------------------------------------------

It would be better if occlusion culling just used the correct region instead of testing whether the region that it's using is different from the correct one.

Comment 24

[Benoit Girard (:BenWa)]

Attachment: culling fixes v2

Comment 25

[Benoit Girard (:BenWa)]

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

Comment 26

[Benoit Girard (:BenWa)]

Try push before jeff' comment:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ae9e70e1e83

Comment 28

[Larissa Shapiro]

Benoit,

Is 1109516 a duplicate of this? Would love to get rid of it if so.

--Larissa

Comment 30

[Benoit Girard (:BenWa)]

I replied in bug 1109516 with instruction how to verify if it's a dupe of this.

Comment 31

[Benoit Girard (:BenWa)]

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

Comment 32

[Benoit Girard (:BenWa)]

Comment on attachment 8560150 [details] [diff] [review]
Simple culling fix (to be uplifted)

The plan:
- Take this patch on central and uplift once it bakes.
- Find the failures for the second version and put that one on central after.

Comment 33

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/820649ecb0fa

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Backed out for making the leftA/topA reftests permafail on Android.
https://hg.mozilla.org/integration/mozilla-inbound/rev/14fad3f56818

Comment 35

[Chris Lord [:cwiiis]]

Comment on attachment 8560985 [details] [diff] [review]
culling fixes v2

Review of attachment 8560985 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +1152,5 @@
>    mCompositor = aManager->GetCompositor();
>  }
>  
> +const nsIntRegion&
> +LayerComposite::GetRenderedVisibleRegion() {

Fly-by comment, I wonder if it's work calling this GetRenderedOpaqueRegion, as it excludes the low precision region (which is visible, but not necessarily opaque, and probably shouldn't be considered as such).

Comment 36

[Chris Lord [:cwiiis]]

Comment on attachment 8560985 [details] [diff] [review]
culling fixes v2

Review of attachment 8560985 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +1154,5 @@
>  
> +const nsIntRegion&
> +LayerComposite::GetRenderedVisibleRegion() {
> +  if (TiledLayerComposer* tiled = GetTiledLayerComposer()) {
> +    return tiled->GetValidRegion();

Another fly-by, maybe related to the failure; Although this should be unnecessary, I wonder if it's worth And-ing this with the ShadowVisibleRegion, for safety?

Comment 37

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #36)
> Comment on attachment 8560985 [details] [diff] [review]
> culling fixes v2
> 
> Review of attachment 8560985 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/composite/LayerManagerComposite.cpp
> @@ +1154,5 @@
> >  
> > +const nsIntRegion&
> > +LayerComposite::GetRenderedVisibleRegion() {
> > +  if (TiledLayerComposer* tiled = GetTiledLayerComposer()) {
> > +    return tiled->GetValidRegion();
> 
> Another fly-by, maybe related to the failure; Although this should be
> unnecessary, I wonder if it's worth And-ing this with the
> ShadowVisibleRegion, for safety?

try seems to confirm that as a possible solution(?): https://treeherder.mozilla.org/#/jobs?repo=try&revision=04045acc659a

Comment 38

[Benoit Girard (:BenWa)]

(In reply to Chris Lord [:cwiiis] from comment #35)
> Comment on attachment 8560985 [details] [diff] [review]
> culling fixes v2
> 
> Review of attachment 8560985 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/composite/LayerManagerComposite.cpp
> @@ +1152,5 @@
> >    mCompositor = aManager->GetCompositor();
> >  }
> >  
> > +const nsIntRegion&
> > +LayerComposite::GetRenderedVisibleRegion() {
> 
> Fly-by comment, I wonder if it's work calling this GetRenderedOpaqueRegion,
> as it excludes the low precision region (which is visible, but not
> necessarily opaque, and probably shouldn't be considered as such).

I wont be landing this version so this wont be an issue. Thanks for the feedback.

(In reply to Chris Lord [:cwiiis] from comment #37
> > Another fly-by, maybe related to the failure; Although this should be
> > unnecessary, I wonder if it's worth And-ing this with the
> > ShadowVisibleRegion, for safety?
> 
> try seems to confirm that as a possible solution(?):
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=04045acc659a

Nice. thanks a lot Cwiiis! I'll prepare a patch based on that.

Comment 39

[Benoit Girard (:BenWa)]

(In reply to Benoit Girard (:BenWa) from comment #38)
> I wont be landing this version so this wont be an issue. Thanks for the
> feedback.
> 

Opps, I got the patch mixed up, I will be landing that one. I don't like having Opaque in the name since the function doesn't check if it's opaque or not nor should it. How about GetFullyRenderedRegion?

Comment 40

[Chris Lord [:cwiiis]]

(In reply to Benoit Girard (:BenWa) from comment #39)
> (In reply to Benoit Girard (:BenWa) from comment #38)
> > I wont be landing this version so this wont be an issue. Thanks for the
> > feedback.
> > 
> 
> Opps, I got the patch mixed up, I will be landing that one. I don't like
> having Opaque in the name since the function doesn't check if it's opaque or
> not nor should it. How about GetFullyRenderedRegion?

That sounds good to me.

Comment 41

[Benoit Girard (:BenWa)]

Attachment: culling fix v3 r=jrmuizel

Carrying forward r=jrmuizel

Comment 42

[Benoit Girard (:BenWa)]

Since https://treeherder.mozilla.org/#/jobs?repo=try&revision=04045acc659a is effectively the same patch with a different name I'm going to skip pushing the same patch to try.

Comment 43

[Benoit Girard (:BenWa)]

In that try push culling was still disabled so the try push was just testing dead code:

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

Comment 44

[Benoit Girard (:BenWa)]

This is a culling problem, it's not related to bug 1112332 or paint heuristics.

Comment 45

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3315ae2e30d9

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3315ae2e30d9

Comment 47

[Benoit Girard (:BenWa)]

Comment on attachment 8566147 [details] [diff] [review]
culling fix v3 r=jrmuizel

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1085223
User impact if declined: possible graphical glitches, particularly during animations/transitions
Testing completed: been on master for several days, some test coverage
Risk to taking this patch (and alternatives if risky): low, this patch makes us cull less.
String or UUID changes made by this patch: none

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/6b3c3dc0779c

Comment 51

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue is verified fixed for the latest Nightly 3.0 and 2.2 builds.

Actual Results:  There is not a black flicker before the keyboard comes up.
	
Environmental Variables:
Device: Flame 3.0 KK (Full Flash) (319 MB)
BuildID: 20150303010233
Gaia: c8ed1085a67490a1ecd7f275e5de9487e1b93b1d
Gecko: 0b3c520002ad
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 39.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Environmental Variables:
Device: Flame 2.2 KK (Full Flash) (319 MB)
BuildID: 20150303002527
Gaia: 3d188c414e30acc392253d5389a42352fcfbc183
Gecko: c89aad487aa5
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0