934420 - APZC scrolling stops working if page layout changes during a scroll

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Jim Mathies [:jimm]]

STR:

1) open facebook or a local search on bing.com that includes a map
2) scroll the page down until the ad panel on fb or map on bing shifts

result: scrolling will stop.

I'll try to work up a simplified test case for this.

Comment 1

[Jim Mathies [:jimm]]

On bing.com, the element that shifts is a div, id = 'wpc_ag'. You can find this through desktop dev tools.

Comment 2

[Jim Mathies [:jimm]]

Attachment: stationarydiv.html

test case

Comment 3

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

Botond, if you have some time, can you look into what's going on here?

Comment 4

[Stephen A Pohl [:spohl] (OOO until 2/27)]

With the DEBUG_CONTROLLER #define turned on in APZController.cpp we get the log below. When scrolling breaks (see bottom of log), we're calling MetroAppShell::MarkEventQueueForPurge from MetroInput::OnPointerReleased. Not sure why we receive a PointerReleased event (yet), but this looks wrong.

[...]
APZController: detected subframe or content editable
APZController::UpdateScrollOffset: 0 131 == 0 0
APZController::RequestContentRepaint scroll id = 1
APZController: mScrollOffset: 0.000000 224.883316
WARNING: huh? IsTab should always get a sub document for a parameter: file c:/mo
zilla/mozilla-central/widget/windows/winrt/APZController.cpp, line 42
APZController: detected subframe or content editable
APZController::UpdateScrollOffset: 0 225 == 0 0
APZController::RequestContentRepaint scroll id = 1
APZController: mScrollOffset: 0.000000 296.666626
WARNING: huh? IsTab should always get a sub document for a parameter: file c:/mo
zilla/mozilla-central/widget/windows/winrt/APZController.cpp, line 42
APZController: detected subframe or content editable
APZController::UpdateScrollOffset: 0 297 == 0 0
APZController::RequestContentRepaint scroll id = 1
APZController: mScrollOffset: 0.000000 353.616669
WARNING: huh? IsTab should always get a sub document for a parameter: file c:/mo
zilla/mozilla-central/widget/windows/winrt/APZController.cpp, line 42
APZController: detected subframe or content editable
APZController::UpdateScrollOffset: 0 353 == 0 0
MetroAppShell::MarkEventQueueForPurge
MetroAppShell::DispatchAllGeckoEvents
mozilla::widget::winrt::FrameworkView::OnWindowActivated
mozilla::widget::winrt::MetroApp::OnSuspending: IsMainThread:1 ThreadId:147C

Comment 5

[Stephen A Pohl [:spohl] (OOO until 2/27)]

(In reply to Stephen Pohl [:spohl] from comment #4)
> With the DEBUG_CONTROLLER #define turned on in APZController.cpp we get the
> log below. When scrolling breaks (see bottom of log), we're calling
> MetroAppShell::MarkEventQueueForPurge from MetroInput::OnPointerReleased.
> Not sure why we receive a PointerReleased event (yet), but this looks wrong.

My mistake, this PointerReleased event is fired when I release the screen to switch back to Desktop view, so is expected. By turning on DEBUG_INPUT in MetroInput.cpp I can now see that MetroInput::OnPointerMoved events continue to be dispatched even after the page stops scrolling, which is good. However, we no longer get calls to APZController::RequestContentRepaint, or any other APZController method.

Comment 6

[Stephen A Pohl [:spohl] (OOO until 2/27)]

When this breaks, the GeckoContentController is suddenly NULL here [1]. Consequently, we can no longer post the GeckoContentController::RequestContentRepaint task [2].

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#1136
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#1139

Comment 7

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

That sounds like we're using a destroyed APZC instance.

Comment 8

[Stephen A Pohl [:spohl] (OOO until 2/27)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> That sounds like we're using a destroyed APZC instance.

It does. We do indeed destroy the instance that we previously interacted with here [1].

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/APZCTreeManager.cpp#95

Comment 9

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

In this bug does the scrolling start working again, or does it stay stopped completely? If it's only the one scroll/fling that aborts then this might just be expected behaviour.

Comment 10

[Stephen A Pohl [:spohl] (OOO until 2/27)]

It does start working again after releasing the screen and touching it again.

Comment 11

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

So then this is likely expected behaviour, although perhaps not very desirable. The reason it's happening is that the relayout changes the layer structure sufficiently that we have to throw away and re-create the APZC tree. needinfo'ing mattwoodrow because retaining the layer structure in the face of relayouts is his area of expertise. Matt, given the attached test case, is the layer retention supposed to work?

Comment 12

[Jim Mathies [:jimm]]

You can also reproduce this on facebook (due to the add panel on the right), and bing local with maps.

Comment 13

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

Attachment: WIP

This patch handles this use case on the APZ side, by re-associating the same APZC instance with the new layer even in the face of layer tree changes. However to make it work we need to check the presshell id as well which will break B2G. We need to fix B2G to propagate the presshell id everywhere before we can land this. Plus this needs more testing etc.

Comment 14

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #13)
> We need to fix B2G to propagate the presshell id everywhere
> before we can land this.

Do we have a bug tracking this? It might make a nice refactoring item.

Comment 15

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

Filed bug 937688 for it.

Comment 16

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

Attachment: Patch

This seems to fix the problem just fine on Metro and doesn't have any other adverse effects as far as I can tell. Try run at https://tbpl.mozilla.org/?tree=Try&rev=aad7e383de59

Comment 17

[Jim Mathies [:jimm]]

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

This fixed scrolling on facebook, bing local w/maps, and the little recent news thumbnail scroller on the bing homepage. :)

Comment 18

[Botond Ballo [:botond]]

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

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1540,5 @@
>  bool AsyncPanZoomController::Matches(const ScrollableLayerGuid& aGuid)
>  {
> +  return aGuid.mLayersId == mLayersId
> +      && aGuid.mPresShellId == mFrameMetrics.mPresShellId
> +      && aGuid.mScrollId == mFrameMetrics.mScrollId;

Can we give ScrollableLayerGuid an operator==?

Comment 19

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

It has one already, but in this case I would need to create a temporary ScrollableLayerGuid for the APZC instance to do the comparison. Do you think it's worth doing that?

Comment 20

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> It has one already, but in this case I would need to create a temporary
> ScrollableLayerGuid for the APZC instance to do the comparison. Do you think
> it's worth doing that?

I have no strong feelings either way.

Comment 21

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

After some deep soul-searching I decided it would a better reuse of code to create the temporary instance and then == it. Hopefully the compiler will inline that anyway.

https://hg.mozilla.org/integration/b2g-inbound/rev/c45a4d2d13b5

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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