951113 - Application is not repainted correctly when the keyboard is dismissed once the screen is off

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

If an application has the keyboard opened and the screen is turned off, it can repaints incorrectly when the screen is turned on again. 

I/TabChild(25226): id(2, 19)
I/TabChild(25226): scrollOffset: 0.000000, 1755.000000
I/TabChild(25226): displayPort: 0.000000, 1485.000000, 320.000000, 450.000000
I/TabChild(25226): compositionBounds: 0, 50, 320, 180
I/TabChild(25226): viewport: 0.000000, 50.000000, 320.000000, 180.000000
I/TabChild(25226): sr(0.000000, 0.000000, 320.000000, 1895.699951)

Then the power button is pressed and the app visibility is set to false. The keyboard is dismissed but there is no Layers updates since the app is not visible.

The the phone is turned on again, and the app is visible once the lockscreen is repainted.

The compositionBounds has changed so the neeContentRepaint flag is not set and the missing part is not repainted until the user starts to scroll.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Repaint when aLayerMetrics.mPresShellId != mFrameMetrics.mPresShellId

Comment 2

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

Comment on attachment 8348687 [details] [diff] [review]
Repaint when aLayerMetrics.mPresShellId != mFrameMetrics.mPresShellId

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1453,5 @@
>    mPaintThrottler.TaskComplete(GetFrameTime());
> +
> +  // If the content was not visible, we may have missed some repaints, so let's
> +  // force a repaint in this case.
> +  bool needContentRepaint = mFrameMetrics.mPresShellId != aLayerMetrics.mPresShellId;

This condition should only ever be true when "isDefault" is also true, and in that case this condition is actually nonsensical because mFrameMetrics doesn't contain any meaningful values.

The reason I say it should only ever be true when "isDefault" is true is because of the code in APZCTreeManager that calls NotifyLayersUpdated - the code there ensures that the APZC instance always matches the ScrollableLayerGuid of the metrics, and part of that check involves checking that the presShellIds are the same.

It also makes sense to me that isDefault will be true in the scenario you described with locking the homescreen, because when you lock the homescreen the layers for the app are destroyed and then when you unlock they are re-created. This means the APZC for the app is also destroyed and created fresh.

That being said, I think the code in TabChild::HandlePossibleViewportChange should already be updating the composition bounds and the displayport when you rotate the device, even if the app is not visible. That code also sets the displayport in layout, so when you unlock and gecko repaints the app, it should paint the correct area. If that's not happening I think there's a bug in TabChild or I'm making a bad assumption about what layout is doing. Or there's a bug in layout.

Comment 3

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

Chris, can you help Vivien with this?

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> Comment on attachment 8348687 [details] [diff] [review]
> Repaint when aLayerMetrics.mPresShellId != mFrameMetrics.mPresShellId
> 
> Review of attachment 8348687 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/AsyncPanZoomController.cpp
> @@ +1453,5 @@
> >    mPaintThrottler.TaskComplete(GetFrameTime());
> > +
> > +  // If the content was not visible, we may have missed some repaints, so let's
> > +  // force a repaint in this case.
> > +  bool needContentRepaint = mFrameMetrics.mPresShellId != aLayerMetrics.mPresShellId;
> 
> This condition should only ever be true when "isDefault" is also true, and
> in that case this condition is actually nonsensical because mFrameMetrics
> doesn't contain any meaningful values.
> 
> The reason I say it should only ever be true when "isDefault" is true is
> because of the code in APZCTreeManager that calls NotifyLayersUpdated - the
> code there ensures that the APZC instance always matches the
> ScrollableLayerGuid of the metrics, and part of that check involves checking
> that the presShellIds are the same.

ah. Makes sense.

> 
> It also makes sense to me that isDefault will be true in the scenario you
> described with locking the homescreen, because when you lock the homescreen
> the layers for the app are destroyed and then when you unlock they are
> re-created. This means the APZC for the app is also destroyed and created
> fresh.
> 
> That being said, I think the code in TabChild::HandlePossibleViewportChange
> should already be updating the composition bounds and the displayport when
> you rotate the device, even if the app is not visible. That code also sets
> the displayport in layout, so when you unlock and gecko repaints the app, it
> should paint the correct area. If that's not happening I think there's a bug
> in TabChild or I'm making a bad assumption about what layout is doing. Or
> there's a bug in layout.

Seems like you right again. I dumped everything again and I see that the viewport is wrong for the app.

Before the screen turns off:

I/TabChild(  599): frame metrics id(2, 2)
I/TabChild(  599): scrollOffset: 0.000000, 0.000000
I/TabChild(  599): compositionBounds: 0, 0, 320, 230
I/TabChild(  599): viewport: 0.000000, 0.000000, 320.000000, 230.000000
I/TabChild(  599): sr(0.000000, 0.000000, 320.000000, 230.000000)
I/TabChild(  599): layer metrics id(2, 2)
I/TabChild(  599): scrollOffset: 0.000000, 0.000000
I/TabChild(  599): compositionBounds: 0, 0, 320, 230
I/TabChild(  599): viewport: 0.000000, 0.000000, 320.000000, 230.000000
I/TabChild(  599): sr(0.000000, 0.000000, 320.000000, 230.000000)
I/TabChild(  599): Repaint:
I/TabChild(  599): frame metrics id(2, 20)
I/TabChild(  599): scrollOffset: 0.000000, 1434.516724
I/TabChild(  599): compositionBounds: 0, 50, 320, 141
I/TabChild(  599): viewport: 0.000000, 50.000000, 320.000000, 141.000000
I/TabChild(  599): sr(0.000000, 0.000000, 320.000000, 1953.650024)
I/TabChild(  599): layer metrics id(2, 20)
I/TabChild(  599): scrollOffset: 0.000000, 1434.516724
I/TabChild(  599): compositionBounds: 0, 50, 320, 141
I/TabChild(  599): viewport: 0.000000, 50.000000, 320.000000, 141.000000
I/TabChild(  599): sr(0.000000, 0.000000, 320.000000, 1953.650024)

After the screen turned on:

I/TabChild(  599): frame metrics id(-1, 0)
I/TabChild(  599): scrollOffset: 0.000000, 0.000000
I/TabChild(  599): compositionBounds: 0, 0, 0, 0
I/TabChild(  599): viewport: 0.000000, 0.000000, 0.000000, 0.000000
I/TabChild(  599): sr(0.000000, 0.000000, 0.000000, 0.000000)
I/TabChild(  599): layer metrics id(2, 2)
I/TabChild(  599): scrollOffset: 0.000000, 0.000000
I/TabChild(  599): compositionBounds: 0, 0, 320, 230
I/TabChild(  599): viewport: 0.000000, 0.000000, 320.000000, 230.000000
I/TabChild(  599): sr(0.000000, 0.000000, 320.000000, 230.000000)
I/TabChild(  599): Repaint:
I/TabChild(  599): frame metrics id(-1, 0)
I/TabChild(  599): scrollOffset: 0.000000, 0.000000
I/TabChild(  599): compositionBounds: 0, 0, 0, 0
I/TabChild(  599): viewport: 0.000000, 0.000000, 0.000000, 0.000000
I/TabChild(  599): sr(0.000000, 0.000000, 0.000000, 0.000000)
I/TabChild(  599): layer metrics id(2, 20)
I/TabChild(  599): scrollOffset: 0.000000, 1434.516724
I/TabChild(  599): compositionBounds: 0, 50, 320, 141
I/TabChild(  599): viewport: 0.000000, 50.000000, 320.000000, 141.000000
I/TabChild(  599): sr(0.000000, 0.000000, 320.000000, 1953.650024)

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I wonder if the issue is that we don't update the viewport because of http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#534

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug951113.patch

Comment 7

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

Comment on attachment 8348888 [details] [diff] [review]
bug951113.patch

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

Can you explain why this works? In the case where isDefault == true, both mFrameMetrics and mLastPaintRequestMetrics should be empty so it's not obvious to me why this works.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> Comment on attachment 8348888 [details] [diff] [review]
> bug951113.patch
> 
> Review of attachment 8348888 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you explain why this works? In the case where isDefault == true, both
> mFrameMetrics and mLastPaintRequestMetrics should be empty so it's not
> obvious to me why this works.

You're right when isDefault is true both mFrameMetrics and mLastPaintRequestMetrics are empty. Then mFrameMetrics is sync with aLayerMetrics by the |if (aFirstPaint || isDefault) case, so when we receive the next paint request then the needContentRepaint flag is not set since mFrameMetrics and aLayerMetrics are the same, but it does not mean this has been painted on the screen.

Comment 9

[Chris Lord [:cwiiis]]

The patch attached to this bug and the reasoning in comment #8 sound reasonable to me, reassigning back to Vivien who seems to have this in hand.

Comment 10

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

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #8)
> You're right when isDefault is true both mFrameMetrics and
> mLastPaintRequestMetrics are empty. Then mFrameMetrics is sync with
> aLayerMetrics by the |if (aFirstPaint || isDefault) case, so when we receive
> the next paint request then the needContentRepaint flag is not set since
> mFrameMetrics and aLayerMetrics are the same, but it does not mean this has
> been painted on the screen.

It seems like the wrong approach to me to try and drive the paint request from the APZC in this case. I feel like this condition you added is something that just happens to work but there's no conceptual reason for it, which means that at some point in the future it will probably break. I think it would be better to somehow trigger a call to HandlePossibleViewportChange after the content is available again so that the code path there triggers a paint with the right metrics.

Were you able to confirm your hypothesis from comment 5?

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #8)
> > You're right when isDefault is true both mFrameMetrics and
> > mLastPaintRequestMetrics are empty. Then mFrameMetrics is sync with
> > aLayerMetrics by the |if (aFirstPaint || isDefault) case, so when we receive
> > the next paint request then the needContentRepaint flag is not set since
> > mFrameMetrics and aLayerMetrics are the same, but it does not mean this has
> > been painted on the screen.
> 
> It seems like the wrong approach to me to try and drive the paint request
> from the APZC in this case. I feel like this condition you added is
> something that just happens to work but there's no conceptual reason for it,
> which means that at some point in the future it will probably break. I think
> it would be better to somehow trigger a call to HandlePossibleViewportChange
> after the content is available again so that the code path there triggers a
> paint with the right metrics.
> 

The metrics are right actually.

The issue is that we are very strict about needContentRepaint (which is a good thing) and because mFrameMetrics and aLayerMetrics are synced by the copy of aLayerMetrics into mFrameMetrics when the layers are just recreated (isDefault), needContentRepaint is not set on from the forwarding call from NotifyLayersUpdated.


> Were you able to confirm your hypothesis from comment 5?

I looked at that and that's not it. I even added a event listener on mozvisibilitychange to trigger a new HandlePossibleViewportChange when the page is visible again. But it was without any effect.

Comment 12

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

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #11)
> The issue is that we are very strict about needContentRepaint (which is a
> good thing) and because mFrameMetrics and aLayerMetrics are synced by the
> copy of aLayerMetrics into mFrameMetrics when the layers are just recreated
> (isDefault), needContentRepaint is not set on from the forwarding call from
> NotifyLayersUpdated.

See this is the part I don't get. NotifyLayersUpdated happens *after* gecko does a paint. So if we're running NotifyLayersUpdated then that means the paint already happened. Now either the paint painted the correct thing or it painted the incorrect thing. If it painted the correct thing then there's nothing more we need to do. If it painted the incorrect thing then we should find out why it painted the incorrect thing and fix the root cause, rather than triggering a second paint with the correct thing.

> 
> The metrics are right actually.
> 

The metrics I was referring to are the various properties set in layout by APZCCallbackHelper::Update*Frame (i.e. the displayport, the scroll clamping size, the scroll offset, and the resolution). If those properties are correct at the time that the app is shown again, then triggering another RequestContentRepaint will not change them. If they are incorrect then we should find out the first point at which they diverge from the expected values.

Comment 13

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

This is definitely a bug that should be fixed, but if I understand, this only happens when the keyboard is up when we turn the phone off - must we block on it?

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #11)
> > The issue is that we are very strict about needContentRepaint (which is a
> > good thing) and because mFrameMetrics and aLayerMetrics are synced by the
> > copy of aLayerMetrics into mFrameMetrics when the layers are just recreated
> > (isDefault), needContentRepaint is not set on from the forwarding call from
> > NotifyLayersUpdated.
> 
> See this is the part I don't get. NotifyLayersUpdated happens *after* gecko
> does a paint. So if we're running NotifyLayersUpdated then that means the
> paint already happened. Now either the paint painted the correct thing or it
> painted the incorrect thing. If it painted the correct thing then there's
> nothing more we need to do. If it painted the incorrect thing then we should
> find out why it painted the incorrect thing and fix the root cause, rather
> than triggering a second paint with the correct thing.
> 

I don't see any calls to UpdateSubFrame once the page is visible again. Could it be it ?

More generally what is the code that is supposed to trigger the Gecko repaint ?

Comment 15

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

Not sure we should look at this for 1.3

Comment 16

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

(In reply to Milan Sreckovic [:milan] from comment #13)
> This is definitely a bug that should be fixed, but if I understand, this
> only happens when the keyboard is up when we turn the phone off - must we
> block on it?

Yes. Presumably it happens when the screen goes off due to the inactivity timer as well, so it will get hit reasonably frequently. However it's not a persistent problem in that the user can scroll to trigger a repaint and fix it, so maybe not a blocker.

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #14)
> I don't see any calls to UpdateSubFrame once the page is visible again.
> Could it be it ?
> 
> More generally what is the code that is supposed to trigger the Gecko
> repaint ?

I don't know exactly where this code lives. But it would be useful to find this out. I'm like 95% sure that what is *supposed* to happen here is that when the keyboard dismisses, TabChild::RecvUpdateDimensions should be getting called, and that should trickle down to call HandlePossibleViewportChange, which should call ProcessUpdateFrame, which should call UpdateRootFrame. If that is not happening, then we should determine why. If it is happening and the problem is in a subframe then perhaps we need to augment that code path and make HandlePossibleViewportChange kick subframes as well.

Comment 17

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

Comment on attachment 8348888 [details] [diff] [review]
bug951113.patch

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

Clearing queue for now until we figure out why the HandlePossibleViewportChange codepath isn't working as intended.

Comment 18

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

If you're not actively working on this please assign it to me and I can continue looking into it.

Comment 19

[Preeti Raghunath(:Preeti)]

1.3+ for QC CS

Comment 20

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

I wasn't able to reproduce on either peak (master) or hamachi (1.3). BenWa was also unable to reproduce on a couple of devices (I think n7 and flatfish). Can you provide more specific STR? I was using the "add a contact" screen and hit the power button to turn the screen off while the keyboard was up. I also tried just waiting until the idle timer kicked in. In all cases after turning the screen on and unlocking the homescreen the keyboard was gone but the content was fully painted.

Comment 21

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I'm not sure if this is related; I was able to get a slight mispaint when creating a new event in calendar and then clicking in the notes field.  With the keyboard scrolling up, it mispaints.  It's more noticible once the keyboard is dismissed.  This doesn't occur with APZ turned off.

1. turn APZ on
2. launch calendar
3. create a new event
4. without scrolling tap into the notes field.

There's a white space between the notes and the top of the keyboard (word suggestion) if you dismiss the keyboard, by tapping on the "Event" title bar; the whole thing whites out.


1.3
Gaia      22bc6be5b76cdc6d4e9667ff070979041a20ce2f
Gecko     http://hg.mozilla.org/releases/mozilla-aurora/rev/2c8f8683bd0d
BuildID   20140109004002
Version   28.0a2
ro.build.version.incremental=eng.archermind.20131114.105818
ro.build.date=Thu Nov 14 10:58:33 CST 2013
Buri

Comment 22

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

Thanks Naoki! I see this also on my hamachi. It doesn't happen on the Peak though, which is kind of weird.

Comment 23

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

Since I can repro comment 21 I can try to debug it.

Comment 24

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

Starting work on this now that I have bug 952170 dealt with.

Comment 25

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

Attachment: Patch

This is part of the problem, but not the whole problem.

Comment 26

[Chris Lord [:cwiiis]]

Comment on attachment 8359886 [details] [diff] [review]
Patch

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

LGTM, and thanks for the explanation on IRC.

Comment 27

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

Attachment: Patch part 2

The other part of a problem is a race because of the IPC messaging between the compositor and content processes which we need to patch over. Hopefully the comments in the patch make it clear.

Comment 28

[Botond Ballo [:botond]]

Comment on attachment 8359981 [details] [diff] [review]
Patch part 2

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

::: widget/xpwidgets/APZCCallbackHelper.cpp
@@ +101,5 @@
> +  // finished sending a layers update with a scroll offset update to the APZ, in
> +  // which case we might actually be clobbering the content-side scroll offset with
> +  // a stale APZ scroll offset. This is unavoidable because of the async communication
> +  // between APZ and content; however the code in NotifyLayersUpdated should reissue
> +  // a new repaint request to bring everything back into sync.

Can we mention here that this is because the "origin of last scroll" flag is cleared during a layers update? I think that would make the description of the issue clearer.

Comment 29

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

Updated comment as requested and landed:

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/fda9b22393b7
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/3f13d3dd021d

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fda9b22393b7
https://hg.mozilla.org/mozilla-central/rev/3f13d3dd021d

Comment 31

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

Not sure why I marked b2g-v1.3 and 1.4 unaffected before...

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e17047520e4c
https://hg.mozilla.org/releases/mozilla-aurora/rev/b3b07adaf7cd

Comment 33

[Timothy Nikkel (:tnikkel)]

(In reply to Chris Lord [:cwiiis] from comment #26)
> Comment on attachment 8359886 [details] [diff] [review]
> Patch
> 
> Review of attachment 8359886 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, and thanks for the explanation on IRC.

The explanation courtesy http://logbot.glob.com.au for reference

19:53	kats	Cwiiis: so the displayport that's passed in to the APZCCallbackHelper is relative to the scroll offset in the frame metrics
19:53	kats	so to make it relative to the actual scroll offset you want to make it absolute with respect to the frame metrics
19:53	kats	and then make it relative to the actual scroll offset
19:53	kats	so you want to add the frame metrics' scroll offset
19:54	kats	and then subtract the actual scroll offset
19:54	kats	Cwiiis: the calculations below that also all add the actual scroll offset, do stuff, and then subtract it back off
19:54	kats	that code doesn't make sense without my patch