1490789 - Scrolling Twitter in Nightly on Android shows many rendering problems

Status: VERIFIED FIXED

(Core :: Graphics, defect, P3)

Description

[Dirkjan Ochtman (:djc)]

See the attached screenshots from my Nexus 4 (Android 5).

This regressed somewhere in the past few weeks.

Comment 1

[Dirkjan Ochtman (:djc)]

Attachment: Screenshot_2018-09-12-20-59-22.png

Comment 2

[Dirkjan Ochtman (:djc)]

Attachment: Screenshot_2018-09-12-20-59-05.png

Comment 3

[Dirkjan Ochtman (:djc)]

Attachment: Screenshot_2018-09-12-20-58-07.png

Comment 4

[Ryan Hunt [:rhunt]]

Is this always reproducible or just intermittently? Are there any special steps to reproduce it besides scrolling on a twitter timeline?

If possible do you have a good build date so we could bisect when the regression was introduced?

Comment 5

[Dirkjan Ochtman (:djc)]

It's not always reproducible. It seems it might be related to images or ads in tweets.

Sorry, I don't have an exact build date, I would start with something like 8-12 weeks ago.

Comment 6

[Ryan Hunt [:rhunt]]

Jamie, have you seen anything like this before? Or are you able to reproduce it?

Comment 7

[Jamie Nicol [:jnicol]]

Hmm, I can reproduce this very intermittently, going to be hard to get a regression range! I'll look in to it further.

Comment 9

[Julien Wajsberg [:julienw]]

I see this on Beta, setting the flags appropriately.

Comment 10

[Jamie Nicol [:jnicol]]

There's a few issues on twitter currently, so let's make this bug solely about content being rendered in the wrong place, and remaining there until another invalidation or scroll occurs. I've found the regression range: bug 1477693. This can be worked around by setting layout.display-list.flatten-transform=false. Note this is a separate issue than the sticky header bar flickering. Also, there appears to be yet another issue with content briefly flashing in the wrong place, which has been around for a long time, which I'll file a follow up for.

After discussing this with Miko, I think we understand this and have a fix. In ComputeGeometryChangeForItem, we apply a shift to some rects and geometries to account for a change in scroll offset. With transform flattening, the shift is taken care of by the root transform in the item's TransformClipTree, so we no longer need to apply it manually. One of the invalidation code branches combines the old area and new area of the invalidated frame. Prior to bug 1477693, this combined area was transformed with the new transform, which takes account of the shift. Post bug 1477693, the old area is transformed by the old transform, which does not take in account the new shift. We must therefore apply the shift before combining it with the new area.

Miko can correct me if the above is wrong. Patch on try currently.

Comment 11

[Mike Taylor [:miketaylr]]

Any progress here? 

Also, P3 seems low for twitter.com.

Comment 12

[Sorina Florean [:sflorean]]

We tried to reproduce this issue also but unfortunately, we don't have Nexus 4 and we tried with Nexus 5(Android 6.0.1) and Xiaomi Mi4i(Android 5.0.2) and we couldn't reproduce it. We will investigate more.

Comment 13

[Julien Wajsberg [:julienw]]

FWIW I still see this on latest nightly.

Comment 14

[Julien Wajsberg [:julienw]]

This is how I reproduce it:
1. scroll down twitter with a momentum (with a "swipe down" gesture)
2. when it's white because of the overscroll, tap once so that the scroll stops
3. wait until the overscroll stops

Comment 15

[Pascal Chevrel:pascalc]

  (In reply to Jamie Nicol [:jnicol] from comment #10)
> There's a few issues on twitter currently, so let's make this bug solely
> about content being rendered in the wrong place, and remaining there until
> another invalidation or scroll occurs. I've found the regression range: bug
> 1477693. This can be worked around by setting
> layout.display-list.flatten-transform=false.

Miko, Jamie, would it be possible to set the above pref to false for 63? What would be the trade off?

Comment 16

[Jamie Nicol [:jnicol]]

Sorry for my inactivity on this bug. I have a patch which looks good on try, I'll get it landed today.

The trade-off would be a performance regression, this patch is simple so it'll be better to land it instead.

Comment 17

[Pascal Chevrel:pascalc]

(In reply to Jamie Nicol [:jnicol] from comment #16)
> Sorry for my inactivity on this bug. I have a patch which looks good on try,
> I'll get it landed today.
> 
> The trade-off would be a performance regression, this patch is simple so
> it'll be better to land it instead.

Could  your patch be ready to be uplifted this week before we enter RC week?

Comment 18

[Jamie Nicol [:jnicol]]

Yes, easily.

Comment 19

[Jamie Nicol [:jnicol]]

Attachment: Bug 1490789 - Apply shift when calculating old area of invalidated frame

When calculating which regions of a layer to invalidate we usually
apply a shift to the area to account for changes in scroll offset. For
items within flattened transforms we do not do this, because the
transform itself includes the scroll offset. However, when calculating
the old area of an invalidated frame, we use the old transform. This
includes the previous scroll offset rather than the current, so we
must therefore still apply the shift.

Not doing so was causing the incorrect region to be invalidated, and
content to be rendered at the wrong location.

Comment 20

[Pascal Chevrel:pascalc]

Tracking for 63 as it affects a top site and we have a patch in view.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf2d65b646d1
Apply shift when calculating old area of invalidated frame r=miko

Comment 22

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/cf2d65b646d1

Comment 23

[Pascal Chevrel:pascalc]

Jamie, could you request uplift to beta? Thanks

Comment 24

[Jamie Nicol [:jnicol]]

Comment on attachment 9015509 [details]
Bug 1490789 - Apply shift when calculating old area of invalidated frame

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1477693

User impact if declined: Content drawn in wrong position on some websites, including twitter

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Minor change. Ensures we invalidate the correct area.

String changes made/needed:

Comment 25

[Jamie Nicol [:jnicol]]

I meant to say no to needs manual test, I have verified it myself. But in any case STR are scrolling quickly on twitter then stopping it. Or alternatively pull down to refresh, then as it is loading quickly tap on a tweet then the phone's back button.

Comment 26

[Pascal Chevrel:pascalc]

Comment on attachment 9015509 [details]
Bug 1490789 - Apply shift when calculating old area of invalidated frame

Fix for a visible rendering issue on Android affecting at least one major site, approved for uplift to our last 63 beta. Thanks.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c4fd0c5a2d5a

Comment 28

[Ioana Chiorean]

I tried the mentioned steps (scrolling quickly) but did not see anything out of place (neither text nor images) with latest 2 builds of Nightly 64.0a1(2018.10.11-12) both in browser and PWA on several devices: 
* Motorola Nexus 6(Android 7.1.1) 
* Nexus 9(Android 6.0.1)
* Pixel 2 (Android 9)
* Samsung S8 Edge (Android 8.0.0)

I did see  a slightly delay in showing the pictures content (like 2sec) like in the second uploaded picture (displays a rectangle with the color major from the picture) BUT the image is aligned and not overlapping lines nor text. That is twitter intended behavior. 
We will also check this at the next beta/tue

Comment 29

[Bogdan Surd, QA [:bsurd, NI]]

Devices:
 - OnePlus 5T (Android 8.1.0)
 - Huawei P9 Lite (Android 6)

Tried in the latest beta (63.0b15), everything worked as expected in both normal browsing as well as PWA. Marking as verified.