967671 - Scrolling start page and then snapping metroFx causes vertical black bar

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Firefox Start, defect, P1)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: scrollStartSnapBug.png

STR:
  1. Scroll the metroFx start page all the way to the right
  2. Snap metroFx to the left of the screen

Attached screenshot shows what my screen looks like after following the STR above.

Comment 1

[Marina Samuel [:emtwo]]

Attachment: snapped.png

Comment 2

[Jim Mathies [:jimm]]

The black bar width is equal to the distance scrolled.

Comment 3

[Jim Mathies [:jimm]]

Attachment: debug patch

Comment 4

[Jim Mathies [:jimm]]

debug output when scrolling ~150 px to the right, then snap another app into view, cutting our view in half:

mozilla::widget::winrt::FrameworkView::OnWindowSizeChanged
Resize: 0.000000 0.000000 945.000000 1080.000000
APZC: 0EDDC370 got a NotifyLayersUpdated with aIsFirstPaint=0: i=(8 4) cb=(0 0 945 984) dp=(-149.067 0.000 2224.000 705.000) v=(0.000 0.000 1376.000 705.000) s=(0.000 0.000) sr=(0.000 0.000 1376.000 1537.900) z=(1.395 1.000 1.000 1.395) 0
apzc.js SizeChanged 
port: 677 774 
displayPort: 0 0 812.4 928.8 
APZC: 0EDDC370 got a NotifyLayersUpdated with aIsFirstPaint=0: i=(8 4) cb=(0 0 945 984) dp=(0.000 0.000 812.400 928.800) v=(0.000 0.000 1376.000 705.000) s=(0.000 0.000) sr=(0.000 0.000 1376.000 1537.900) z=(1.395 1.000 1.000 1.395) 0
APZC: 0EDDC370 got a NotifyLayersUpdated with aIsFirstPaint=0: i=(8 4) cb=(0 0 945 984) dp=(0.000 0.000 812.400 928.800) v=(0.000 0.000 677.000 705.000) s=(0.000 0.000) sr=(0.000 0.000 677.000 3257.900) z=(1.395 1.000 1.000 1.395) 0
APZC: Calculated displayport as (-149.066589 0.000000 677.000000 2467.500000) from velocity (0.000000 0.000000) paint time 33.766003 metrics: i=(8 4) cb=(0 0 945 984) dp=(-149.067 0.000 2224.000 705.000) v=(0.000 0.000 677.000 705.000) s=(149.067 0.000) sr=(0.000 0.000 677.000 3257.900) z=(1.395 1.000 1.000 1.395) 0

I'm wondering about that first calculated display port of (-149.066589 0.000000 677.000000 2467.500000). The -149 represents the amount content is offset to the left, and also corresponds to the width of the black bar on the right. It appears the origin we compose to is off.

kats, anything stand out to you as being wrong here?

Comment 5

[Jim Mathies [:jimm]]

Attachment: ~150px scroll

Comment 6

[Jim Mathies [:jimm]]

Hmm, I commented our our display port setting up in apzc.js to see if it was playing into this, and it didn't have any effect.

Comment 7

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

There's a few odd things in the dump you pasted in comment 4. For one thing all the NotifyLayersUpdated say s=(0.000 0.000) which means layout thinks the scroll offset is at 0,0. The "Calculated displayport" line however says s=(149.067 0.000) which means the APZ thinks the scroll offset is at x=149.067. It is computing the displayport accordingly (dp's x is -149.067) so that the displayport extends all the way to the left edge of the about:start page.

Given that the displayport is already at -149.067 in the first NotifyLayersUpdated, that means that APZ must already have requested that displayport before, presumably along with the 149.067 scroll offset. But layout didn't "accept" that scroll offset, or maybe it accepted it and then clobbered it with something else without notifying APZ. Either way the fact that the scroll offsets are out of sync seems wrong.

I would stick a breakpoint or logging at http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#1995 and record all the calls made there, to see who's calling it and with what scroll offset.

Comment 8

[Jim Mathies [:jimm]]

When we flip to this snapped mode, layout of the start page changes. The page loses its horizontal scroll and scrolls vertically instead. I'll try to catch that transition in some debug output plus the gfx scroll frame info.

Comment 9

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

Ok, that explains the changes in the scrollable rect (the "sr" values in the dump above). However at some point layout clamps the scroll position to 0,0 (which sounds reasonable if the layout changes) but doesn't inform the APZ. Specifically, the code at [1] needs to get executed but doesn't.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=319abcb8c59e#646

Comment 10

[Kamil Jozwiak [:kjozwiak]]

Kats, could Bug #964140 be related to this issue? Rather then staying in snapped view, I moved fxmetro back to full view and refreshed the screen while it was scrolled all the way to the right side. Refreshing the screen would produce a vertical black bar. Sometimes I was able to get the vertical black bar by simply scrolling to the right and refreshing (without going into snapped view and then full view)

Comment 11

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

It's possible, yeah. It certainly sounds like the root cause might be the same.

Comment 12

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)
> Ok, that explains the changes in the scrollable rect (the "sr" values in the
> dump above). However at some point layout clamps the scroll position to 0,0
> (which sounds reasonable if the layout changes) but doesn't inform the APZ.
> Specifically, the code at [1] needs to get executed but doesn't.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.
> cpp?rev=319abcb8c59e#646

This coded isn't getting called because scrollableFrame->OriginOfLastScroll() returns null after a reflow. I think we just need to take that valid pointer check out, although I'm not sure about side effects.

last scroll origin: apz
APZC: 0A8AAA90 got a NotifyLayersUpdated with aIsFirstPaint=0: i=(5 2) cb=(0 0 1920 984) dp=(-184.900 0.000 2224.000 705.000) v=(0.000 0.000 1376.000 705.000) s=(184.900 0.000) sr=(0.000 0.000 2224.000 705.000) z=(1.395 1.000 1.000 1.395) u=(0 4294967296)
mozilla::widget::winrt::FrameworkView::OnWindowActivated
Resize: 0.000000 0.000000 1920.000000 1080.000000
RecordFrameMetrics  scroll overflow: x:0 y:0  rect: 0.000000,0.000000 2224.000000,705.000000  position: 0.000000,0.000000
last scroll origin: apz
APZC: 0A8AAA90 got a NotifyLayersUpdated with aIsFirstPaint=0: i=(5 2) cb=(0 0 1920 984) dp=(-184.900 0.000 2224.000 705.000) v=(0.000 0.000 1376.000 705.000) s=(184.900 0.000) sr=(0.000 0.000 2224.000 705.000) z=(1.395 1.000 1.000 1.395) u=(0 4294967296)

mozilla::widget::winrt::FrameworkView::OnWindowSizeChanged
Resize: 0.000000 0.000000 945.000000 1080.000000
viewstate change event fired, about:start layout updated. 

RecordFrameMetrics  scroll overflow: x:1 y:1  rect: 0.000000,0.000000 1376.000000,1537.900024  position: 0.000000,0.000000
last scroll origin: unknown
APZC: 0A8AAA90 got a NotifyLayersUpdated with aIsFirstPaint=0: i=(5 2) cb=(0 0 945 984) dp=(-184.900 0.000 2224.000 705.000) v=(0.000 0.000 1376.000 705.000) s=(0.000 0.000) sr=(0.000 0.000 1376.000 1537.900) z=(1.395 1.000 1.000 1.395) u=(0 4294967296)

RecordFrameMetrics  scroll overflow: x:1 y:1  rect: 0.000000,0.000000 1376.000000,1537.900024  position: 0.000000,0.000000
last scroll origin: unknown
APZC: 0A8AAA90 got a NotifyLayersUpdated with aIsFirstPaint=0: i=(5 2) cb=(0 0 945 984) dp=(-184.900 0.000 2224.000 705.000) v=(0.000 0.000 1376.000 705.000) s=(0.000 0.000) sr=(0.000 0.000 1376.000 1537.900) z=(1.395 1.000 1.000 1.395) u=(0 4294967296)
RecordFrameMetrics  scroll overflow: x:1 y:1  rect: 0.000000,0.000000 677.000000,3257.899902  position: 0.000000,0.000000

APZC: Calculated displayport as (-184.899979 0.000000 677.000000 2467.500000) from velocity (0.000000 0.000000) paint time 37.151878 metrics: i=(5 2) cb=(0 0 945 984) dp=(-184.900 0.000 2224.000 705.000) v=(0.000 0.000 677.000 705.000) s=(184.900 0.000) sr=(0.000 0.000 677.000 3257.900) z=(1.395 1.000 1.000 1.395) u=(0 759607606732914688)

Comment 13

[Jim Mathies [:jimm]]

Attachment: fix

This gets set to null here - 

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.h#308

and when we create the frame. It gets set to something valid in ScrollToImpl. So it seems we should update the offsets when it's null in nsDisplayList.

Comment 14

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

Comment on attachment 8375535 [details] [diff] [review]
fix

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

::: layout/base/nsDisplayList.cpp
@@ +641,5 @@
>      // If the frame was scrolled since the last layers update, and by
>      // something other than the APZ code, we want to tell the APZ to update
>      // its scroll offset.
>      nsIAtom* originOfLastScroll = scrollableFrame->OriginOfLastScroll();
> +    if (!originOfLastScroll || originOfLastScroll != nsGkAtoms::apz) {

I don't think this is right. In particular if we're in a stable state (i.e. page is loaded, nobody's scrolling anything) and there's some sort of animation going on the page, then layout is going to keep painting each frame of animation, and all of those will send scroll offset updates to APZ. That means if you start panning, there will be noticeable jitter and jumpiness in that scenario.

I think a better solution might be to initialize mOriginOfLastScroll in nsGfxScrollFrame to something that's non-null. nsGkAtoms::other is fine. Can you check if that fixes the problem?

Comment 15

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

(In reply to Jim Mathies [:jimm] from comment #13)
> This gets set to null here - 
> 
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/
> nsGfxScrollFrame.h#308
> 
> and when we create the frame.

Note that the line you pointed out above only ever gets called when APZ acknowledges a scroll offset update, which based on the data you're showing me, I don't think is happening. So it's more likely that the frame is destroyed and re-created, and mOriginOfLastScroll gets reset as a result. That's why I think initializing it to something non-null should also fix the problem, and with fewer side-effects.

Comment 16

[Jim Mathies [:jimm]]

Attachment: fix v.2

That works too.

Comment 17

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

Comment on attachment 8375553 [details] [diff] [review]
fix v.2

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

r=me but this will require an additional change in APZC::NotifyLayersUpdated, which I'll put up in a second.

Also, would like tn to review the nsGfxScrollFrame change as well. In a nutshell what's happening is that the window resizes, so the scrollframe helper is destroyed and recreated (perhaps the scrollframe itself is destroyed and recreated?). The scroll offset is therefore reset, and so is originOfLastScroll. The scrollId hasn't changed but the scroll offset effectively has, so we need to notify the APZ. Note that this means the first time a scroll frame appears we will also send the scroll offset update to APZ, which is fine because in this case we take the entire FrameMetrics anyway. The patch I will attach will ensure that the acknowledgement is sent even in this case.

Comment 18

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

Attachment: Ensure we acknowledge the scroll offset update always

This patch is good to have regardless of the other patch. But if we go with that fix then it becomes more likely we'll need this patch to avoid regressions.

Comment 19

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

(Note: I haven't even compiled the above patch. It would be good to push both together to try to ensure nothing breaks).

Comment 20

[Jim Mathies [:jimm]]

Did you need to split that out? Looks like you have two aLayerMetrics.GetScrollOffsetUpdated() if blocks, one right after the other.

Comment 21

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #20)
> Did you need to split that out? Looks like you have two
> aLayerMetrics.GetScrollOffsetUpdated() if blocks, one right after the other.

oh, nm, that first one is nested inside something else.

Comment 22

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> (Note: I haven't even compiled the above patch. It would be good to push
> both together to try to ensure nothing breaks).

I have both patches applied locally, everything looks ok.

Comment 23

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

Cool. I also pushed them to try here:

https://tbpl.mozilla.org/?tree=Try&rev=c3956d2fc415

Comment 24

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aaa3019c7653
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbcd1f3a1703

thanks for the help guys!

Comment 25

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #24)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/aaa3019c7653
> https://hg.mozilla.org/integration/mozilla-inbound/rev/fbcd1f3a1703
> 
> thanks for the help guys!

oops, right push set here:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbcd1f3a1703
https://hg.mozilla.org/integration/mozilla-inbound/rev/af48e36fd5a3

Comment 26

[Jim Mathies [:jimm]]

Kats, do you think this will be ok to uplift to aurora so we can get it out a bit sooner? (Beta seems a bit risky to me.)

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/fbcd1f3a1703
https://hg.mozilla.org/mozilla-central/rev/af48e36fd5a3

Comment 28

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

Yeah I'm fine with uplifting to Aurora and actually even Beta. Note that only attachment 8375553 [details] [diff] [review] should be uplifted; the other patch is dependent on bug 963278 which was never uplifted. I looked at the code currently on beta and I'm around 95% sure that just uplifting the one patch won't cause issues. I'll leave it up to you decide how far you want to uplift it.

Comment 29

[Cornel Ionce [:noni] [Hubs QA]]

I can confirm this issue is no longer reproducing on latest Nightly (build ID: 20140217030202).

Comment 30

[Jim Mathies [:jimm]]

Comment on attachment 8375553 [details] [diff] [review]
fix v.2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: corner case rendering bug when resizing the main browser view.
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): kats seems positive there's little risk, I'll take his word for it. :)
String or IDL/UUID changes made by this patch: none

Comment 31

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

(In reply to Jim Mathies [:jimm] from comment #25)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/af48e36fd5a3

Please make sure you copy the commit message and author from the patch next time! :)

Comment 32

[Jim Mathies [:jimm]]

(In reply to Phil Ringnalda (:philor) from comment #27)
> https://hg.mozilla.org/mozilla-central/rev/fbcd1f3a1703


This is the only checkin to uplift.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f24272656ba2
https://hg.mozilla.org/releases/mozilla-beta/rev/ca5a047a97d5

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/ca5a047a97d5

Comment 35

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.3; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Windows NT 6.3; rv:29.0) Gecko/20100101 Firefox/29.0

Verified as fixed on Firefox 28 beta 4 (build ID: 20140218122424) and on latest Aurora (build ID: 20140219004002).

Comment 36

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

Note that the second patch on this bug (attachment 8375575 [details] [diff] [review]) got uplifted into 1.3 as part of the uplift of bug 980679, in this cset:

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/ab227cdd984c