966507 - [B2G][Wikipedia] Changing the font size results in a blank article page

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

[Nikolai Khristoforov]

Attachment: Logcat

When the user changes the font size through the Settings page, the article page becomes blank sometimes. This is not 100% reproducible and usually happens after scrolling on a page with a lot of text.

Repro Steps:
1) Updated Buri to BuildID: 20140127004002
2) Install the Wikipedia app through Marketplace.
3) Open the Wikipedia app and select one of the linked words to open a new article with a lot of text on it.
4) Scroll down and while the page is scrolling select the gear icon in the bottom right corner.
5) Select the Font field and change the size (it doesn't matter if it's bigger or smaller).
6) If the text remains on the page, try scrolling and changing the font again.

Actual:
The page becomes blank and text returns only when scrolling.

Expected:
Text would remain on the page and it would never remain blank.

Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140127004002
Gaia: 25a45a836a4a21a30f63fa7b544b42e8b781180a
Gecko: c40099a42c1f
Version: 28.0a2
Firmware Version: V1.2-device

Repro frequency: 100%
See attached: Logcat and Youtube video (http://youtu.be/HethvwNHz9o)

Comment 1

[Nikolai Khristoforov]

This issue does not occur on the 1.2. The page never became blank.

Device: Buri 1.2 MOZ
BuildID: 20140128004004
Gaia: 539a25e1887b902b8b25038c547048e691bd97f6
Gecko: d10e1f965d0c
Version: 26.0
Firmware Version: V1.2-device.cfg

Comment 2

[pfield]

Issues appears to be related to APZ and occurs as far back as 10/30/2013 however, due to the original APZ issue that involves a broken FTE, testing this issue on anything prior is extremely difficult. (Issue does not occur when APZ is disabled.)

Environmental Variables:
Device: BURI 1.3 MOZ
BuildID: 20131030040204
Gaia: 162cc9cd7153dcb284e5f54d5598fb27f1ab18aa
Gecko: 829d7bef8b0a
Version: 28.0a1
Firmware Version: V1.2-device.cfg

Comment 3

[Preeti Raghunath(:Preeti)]

Please have this on your radar

Comment 4

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

Seems like an edge case for 1.3+.

Comment 5

[Jason Smith [:jsmith]]

(In reply to Milan Sreckovic [:milan] from comment #4)
> Seems like an edge case for 1.3+.

Wikipedia is a preinstalled partner app. There's no room for discussion on this being a non-blocker - we have a commitment to partners that these apps have to work in parity across the releases.

Comment 6

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

Fwiw it works for me on m-c/master on Keon.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #6)
> Fwiw it works for me on m-c/master on Keon.

Can someone confirm this on 1.4 Buri?

Comment 8

[pfield]

Issue still occurs in the same manner on Buri 1.4

Environmental Variables:
Device: BURI 1.4 MOZ
BuildID: 20140204040201
Gaia: 75e9691f02b9d18585c18a5434beeff39ee7ea20
Gecko: c150845d077d
Version: 30.0a1
Firmware Version: V1.2-device.cfg

Comment 9

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

I could reproduce this earlier but on the latest 1.3 code I cannot reproduce it anymore. It might have been fixed by recent changes. qawanted to verify.

Comment 10

[pfield]

I am able to get it on today's Buri 1.3

Environmental Variables:
Device: Buri 1.3 Moz
BuildID: 20140210004002
Gaia: 5c8416fb1ea4a27f172ee6386ab3c19135448506
Gecko: 9c9382f433c0
Version: 28.0
Firmware Version: V1.2-device.cfg

Comment 11

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

Ok, I can reproduce it still (although more intermittently than before) so I'll take a look.

Comment 12

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

Initial investigation seems to indicate that while the options screen is up, the APZ for the content is destroyed (because it is in the back and non-scrollable). Upon dismissing the options screen, we create a new APZ and populate it with the frame metrics from layout. That frame metrics indicates that the displayport has a y-offset of 2043.950 pixels (in the instance I have logs for) from the scroll position. I don't know where this displayport is coming from - the previous displayport requests issued by the APZ for that scroll id all seem sane. It almost feels like the displayport value is getting corrupted somehow but if that were the case we would probably have run into it sooner. I'll keep looking.

Comment 13

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

Looks like the displayport gets modified to the unwanted thing in MaybeAlignAndClampDisplayPort. I'll narrow it down.

Comment 14

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

So the general problem is that the scroll offset sent by APZ isn't updated, which can happen either because the scrollframe is no longer around, or because it's scroll offset has been updated from something else and we don't want to clobber it (i.e. the two alternative exits of APZCCallbackHelper::ScrollFrameTo).

In the former case I think it makes sense to clear the displayport entirely. In the latter case I'm not sure what the best thing to do is. Also the behaviour for the latter case will be different in 1.3 vs master because master has bug 963278 landed and 1.3 does not. So in the 1.3 case we could get away with just setting the displayport that APZ requested because we know it will get reset once the scroll offset update flag reaches APZ. On master though there's the "acknowledge-scroll-offset" message so the bad displayport might end up sticking around for a while. Therefore I'm leaning towards just centering the displayport when we run into this situation.

Comment 15

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

Attachment: Recenter displayport if the scrollframe disappears

Turns out that there's no way to clear the displayport on an element, so I'd have to add a new domwindowutils method to do that. Instead I figured we probably don't read the displayport for non-scrollable elements anyway, so there's no harm is just leaving it. This patch re-centers the displayport if the scroll offset from APZ was rejected though. This seems to fix it for me locally. (Also for the record, in this bug the scrollframe itself is gone, so we hit the !aFrame case at the top of ScrollFrameTo).

Comment 16

[Timothy Nikkel (:tnikkel)]

Why do we fail to find the scroll frame? Does the content for the old scroll frame never get a scroll frame after this or does it get one right after?

Comment 17

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

It gets one eventually, but not "right after". In terms of the STR in comment 0, the scrollframe is null between step 4 and step 5 - once the fling stops, APZ sends a repaint request and in the handling of that the scrollframe is null. At this point the settings screen is visible on top of the content. Later, when we dismiss the settings, the scrollable frame comes back to life.

Comment 18

[Timothy Nikkel (:tnikkel)]

I guess I'm asking why it's null, does it's content get removed from the document?

Comment 19

[Timothy Nikkel (:tnikkel)]

To answer my own question it looks like the content is being made display: none.

Comment 20

[Chris Lord [:cwiiis]]

Comment on attachment 8375133 [details] [diff] [review]
Recenter displayport if the scrollframe disappears

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

r+ with the comment addressed. (I want to be convinced that isn't necessary if you decide I'm wrong before this is pushed, if that's ok)

::: widget/xpwidgets/APZCCallbackHelper.cpp
@@ +219,5 @@
>      if (element) {
> +        if (scrollUpdated) {
> +            MaybeAlignAndClampDisplayPort(aMetrics, actualScrollOffset);
> +        } else {
> +            RecenterDisplayPort(aMetrics);

I think we still want to call MaybeAlignAndClampDisplayPort, so that the recentered displayport is tile-aligned.

Comment 21

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

It doesn't always make sense to tile-align the recentered displayport here, because tile-alignment depends on the scroll position of the scroll frame, and there might be no scroll frame here at all. If/when the scroll frame reappears its scroll position could be anywhere, so tile-aligning it (assuming a scroll position of 0,0) here doesn't even make sense because it won't be tile aligned with respect to the new scroll position that the scroll frame has.

In the case of this specific bug the scroll position will not be 0,0 and we don't know what it will be at this point in the code so there's no way we can tile align it sanely. We can still do some tile alignment for the other cases (like when content has changed the scroll offset) but that breaks down if content is doing an async scroll in layout (e.g. mouse wheel scrolling on metro) because the scroll position is going to keep changing and invalidate our alignment.

Therefore I think it's better to not do any alignment at all in this case, otherwise we run the risk of aligning it such that we end up with perma-checkerboarding.

Comment 22

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #21)
> It doesn't always make sense to tile-align the recentered displayport here,
> because tile-alignment depends on the scroll position of the scroll frame,
> and there might be no scroll frame here at all. If/when the scroll frame
> reappears its scroll position could be anywhere, so tile-aligning it
> (assuming a scroll position of 0,0) here doesn't even make sense because it
> won't be tile aligned with respect to the new scroll position that the
> scroll frame has.
> 
> In the case of this specific bug the scroll position will not be 0,0 and we
> don't know what it will be at this point in the code so there's no way we
> can tile align it sanely. We can still do some tile alignment for the other
> cases (like when content has changed the scroll offset) but that breaks down
> if content is doing an async scroll in layout (e.g. mouse wheel scrolling on
> metro) because the scroll position is going to keep changing and invalidate
> our alignment.
> 
> Therefore I think it's better to not do any alignment at all in this case,
> otherwise we run the risk of aligning it such that we end up with
> perma-checkerboarding.

This is a reasonable explanation, I think it'd be nice to add a small note as to why it isn't aligned in this case but I'm happy if you don't think that's the case. r+.

Comment 23

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

I updated the comment a little bit and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c26732abea26

Comment 24

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

Comment on attachment 8375133 [details] [diff] [review]
Recenter displayport if the scrollframe disappears

NOTE: This flag is now for security issues only. 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 #): APZ
User impact if declined: In some rare cases, content may end up not getting painted and show blank. This goes away once the user starts interacting with the content again.
Testing completed: locally
Risk to taking this patch (and alternatives if risky): I would probably put this at medium risk considering it's late in the game for 1.3 and this patch has the potential for unexpected side effects. I would be more comfortable if we could let it bake on m-c for a bit to shake out any regressions.
String or UUID changes made by this patch: none

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Comment 28

[bhavana bajaj [:bajaj]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #24)
> Comment on attachment 8375133 [details] [diff] [review]
> Recenter displayport if the scrollframe disappears
> 
> NOTE: This flag is now for security issues only. 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 #): APZ
> User impact if declined: In some rare cases, content may end up not getting
> painted and show blank. This goes away once the user starts interacting with
> the content again.
> Testing completed: locally
> Risk to taking this patch (and alternatives if risky): I would probably put
> this at medium risk considering it's late in the game for 1.3 and this patch
> has the potential for unexpected side effects. I would be more comfortable
> if we could let it bake on m-c for a bit to shake out any regressions.

Given this, lets get some exploratory QA testing around this area before uplifting.

> String or UUID changes made by this patch: none

Comment 29

[bhavana bajaj [:bajaj]]

NI on :jsmith to help push this up the QA verifyme queue.

Comment 30

[Jason Smith [:jsmith]]

I'll have someone look into this on Monday.

Comment 31

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

This issue is not occurring on the latest Buri v 1.4 Mozilla RIL 

Environmental Variables
Device: Buri v 1.4.0 Mozilla RIL
Build ID: 20140224040201
Gecko: https://hg.mozilla.org/mozilla-central/rev/31113754db3b
Gaia: ffb527b84594396ed611edf0a8a5a130d60a742f
Platform Version: 30.0a1
Firmware Version: v1.2-device.cfg

The page never appeared blank after scrolling a page with a lot of text and changing the font size in the Wikipedia app.

Comment 32

[bhavana bajaj [:bajaj]]

Comment on attachment 8375133 [details] [diff] [review]
Recenter displayport if the scrollframe disappears

Given QA verification and baketime on nightly, approving this blocker. Let's keep a close eye on any related fallouts once this lands.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/688ea6912b90

Comment 34

[bmorgan]

1.4: Webpage is displayed properly after switching font sizes
1.3 Webpage is displayed properly after switching font sizes

1.4 Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140528000201
Gaia: cd595be0a8e975559e8938830df5face89bec3e8
Gecko: af180db8a4bf
Version: 30.0
Firmware Version: v1.2-device.cfg

1.3 Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140528024003
Gaia: 0ce948e378cab7ed3db20231281dd7ca2eb99779
Gecko: ce0dd450bffb
Version: 28.0
Firmware Version: v1.2-device.cfg

Comment 35

[Roland Kunkel [:RolandK]]

Verified for v1.3T. I am unable to reproduce this issue in the latest v1.3T Tarako build:

v1.3T Environmental Variables:
Device: Tarako v1.3T MOZ RIL
BuildID: 20140530014002
Gaia: e68858693b71d917c9c5ee7e215f7ceea04635f7
Gecko: 1945abae19ff
Version: 28.1
Firmware Version: SP6821a-Gonk-4.0-5-12