1228028 - Crash when DOM_DELTA_PAGE scrolling with Android APZ enabled [B2GDroid]

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P1)

Description

[sgiles]

Attachment: apz-crash-bt.log

The JavaScript accessibility features (enabled for B2GDroid with the part2 patch in bug 1224604) cause a page scroll with certain gestures.  This crashes B2GDroid, see attached back trace.

Comment 1

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

Hm, so as long as the JS code doesn't expect the page scroll to happen synchronously, we can work around this crash. If the JS code *does* expect the page scroll to happen synchronously then we'll need to update the JS code as well. By "synchronously" I mean that they expect the scroll position to have changed during the call. Without APZ that would have been the case, but with APZ the generated wheel event will get queued onto other threads and so will be processed later.

The work around for the crash is to add a page type along with the line and pixel types in InputData.h, and update the conversion functions DeltaTypeForDeltaMode and DeltaModeForDeltaType accordingly. That will allow the ScrollWheelInput to be created and it will get passed into the APZ. The WillHandleInput call at [1] will probably be returning true because it will go into the !NS_IsMainThread() branch. However, we want to augment that check so that it returns false for the page scroll events. Since the function is templated to also take PanGestureInput events, you might need to just remove the templating and have the two specializations, or do some other template condition comparison goop, or something.

Once that function is returning false for page scroll events, APZ will reject the wheel input (which is good, because it doesn't know how to handle page scroll events yet) and the main thread will end up handling it. That should then result in normal behaviour (assuming, as I mentioned above, that the JS code doesn't expect it to be synchronous).

[1] https://dxr.mozilla.org/mozilla-central/rev/099f695d31326c39595264c34988a0f4b7cbc698/gfx/layers/apz/src/APZCTreeManager.cpp#736

Comment 2

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

(Also CC'ing dvander because it might be trivial to add support for page scrolling, in which case we can just do that instead. I'm not sure how complicated page scroll support would be.)

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

I don't think it would be too hard to fully support page scrolling:

https://mxr.mozilla.org/mozilla-central/source/dom/events/EventStateManager.cpp#2459

Currently we give APZ the LineScrollAmount via ComputeFrameMetrics. So we would just need to send the PageScrollAmount in a similar fashion. There may be special cases elsewhere but I haven't looked outside EventStateManager::DoScrollText.

Comment 4

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

This crash is being hit on B2G as well with accessibility enabled. dvander, do you have cycles to implement page scrolling? If not we can handle it like I suggested in comment 1 for now.

Comment 5

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

Attachment: Part 1 - Add page constants to InputData

We'll need this regardless.

Comment 6

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

Attachment: Part 2 - Return false for WillHandleInput for off-main-thread wheel events

This splits the template function for PanGestureInput and WheelInput, so that we do handle PanGestureInput off the main thread, but we don't for WheelInputs. Although in theory this should make the accessibility page-swipe thing work by using main thread scrolling on B2G, it doesn't seem to actually work, and I'm not sure why. The wheel event is getting dispatched as expected.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

A few minutes too late, but here's a try run with page scrolling support: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56f024943873

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: page scroll support

Taking w/ permission

Comment 9

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

Comment on attachment 8694614 [details] [diff] [review]
page scroll support

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

Nice, thanks!

Comment 10

[Pulsebot]

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

Comment 11

[Carsten Book [:Tomcat]]

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