Closed Bug 1228028 Opened 7 years ago Closed 7 years ago

Crash when DOM_DELTA_PAGE scrolling with Android APZ enabled [B2GDroid]


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

45 Branch



Tracking Status
firefox45 --- fixed


(Reporter: sgiles, Assigned: dvander)


(Depends on 2 open bugs)



(2 files, 2 obsolete files)

Attached file 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.
Attachment #8692058 - Attachment mime type: text/x-log → text/plain
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).

(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.)
See Also: → 1228039
I don't think it would be too hard to fully support page scrolling:

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.
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.
We'll need this regardless.
Attachment #8694366 - Flags: review?(dvander)
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.
Assignee: sgiles → bugmail.mozilla
Attachment #8694368 - Flags: review?(dvander)
A few minutes too late, but here's a try run with page scrolling support:
Taking w/ permission
Assignee: bugmail.mozilla → dvander
Attachment #8694366 - Attachment is obsolete: true
Attachment #8694368 - Attachment is obsolete: true
Attachment #8694366 - Flags: review?(dvander)
Attachment #8694368 - Flags: review?(dvander)
Attachment #8694614 - Flags: review?(bugmail.mozilla)
Comment on attachment 8694614 [details] [diff] [review]
page scroll support

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

Nice, thanks!
Attachment #8694614 - Flags: review?(bugmail.mozilla) → review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Depends on: 1234987
Depends on: 1234988
Depends on: 1238502
Depends on: 1238503
Depends on: 1326650
Depends on: 1328065
You need to log in before you can comment on or make changes to this bug.