Closed Bug 1256341 Opened 8 years ago Closed 8 years ago

crash in mozilla::layers::AsyncPanZoomController::GenerateSingleTap

Categories

(Core :: Panning and Zooming, defect)

48 Branch
x86_64
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: kats)

References

Details

(Keywords: crash, Whiteboard: gfx-noted)

Crash Data

Attachments

(2 files)

This bug was filed from the Socorro interface and is 
report bp-f556c786-e8ab-4b45-ac11-03a532160314.
=============================================================

This signature appears to be a regression in v48.
The volume is low so far (for the past 28 days):

Operating System	Percentage	Number Of Crashes
Windows 10	100.00%	10

Product	Version	Percentage	Number Of Crashes
Firefox	48.0a1	100.00%	10

Architecture	Percentage	Number Of Crashes
amd64	100.00%	10

Stack:
mozilla::layers::AsyncPanZoomController::GenerateSingleTap(mozilla::gfx::IntPointTyped<mozilla::ScreenPixel> const&, unsigned short)
mozilla::layers::AsyncPanZoomController::HandleGestureEvent(mozilla::InputData const&)
mozilla::layers::GestureEventListener::TriggerSingleTapConfirmedEvent()
RunnableMethod<mozilla::ipc::MessageChannel, void ( mozilla::ipc::MessageChannel::*)(void), mozilla::Tuple<> >::Run()
MessageLoop::DoDelayedWork(base::TimeTicks*)
mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)
MessageLoop::RunHandler()
MessageLoop::Run()
nsBaseAppShell::Run()
nsAppShell::Run()
nsAppStartup::Run()
XREMain::XRE_mainRun()
See Also: → 1256344
Fwiw, this signature is currently at #31 in Firefox 48.0a1 Top Crash list.
(Together with bug 1256344 they would be at #10.)
Same issue here, CurrentTouchBlock() is null:
http://hg.mozilla.org/mozilla-central/diff/1709c5fdd182/gfx/layers/apz/src/AsyncPanZoomController.cpp#l1427
Flags: needinfo?(bugmail.mozilla)
Whiteboard: gfx-noted
If I had to guess, the caller of TriggerSingleTapConfirmedEvent() (which seems to have been inlined and doesn't show up in the stack trace) is HandleInputTimeoutMaxTap().
The discussion in bug 1256344 applies here as well. However, there's something fishy in this one. On touch-up, the GEL calls AsyncPanZoomController::OnSingleTapUp which tries to generate the single-tap synchronously if double-tap-to-zoom is not allowed. And only if that fails, does it schedule the timer to trigger the single-tap later. On desktop, double-tap-to-zoom should not be allowed, and so we should never be hitting this codepath at all.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> The discussion in bug 1256344 applies here as well. However, there's
> something fishy in this one. On touch-up, the GEL calls
> AsyncPanZoomController::OnSingleTapUp which tries to generate the single-tap
> synchronously if double-tap-to-zoom is not allowed. And only if that fails,
> does it schedule the timer to trigger the single-tap later. On desktop,
> double-tap-to-zoom should not be allowed, and so we should never be hitting
> this codepath at all.

I have a theory about this. GEL schedules the timer if the HandleGestureEvent(TAPGESTURE_UP) call returns nsEventStatus_eIgnore.

APZC returns eIgnore if double-tapping is disallowed, but also in some early-exit paths in GenerateSingleTap() - including, notably, if we're in a fast fling!
I'll take a crack at fixing this since I'm looking at bug 1256344 anyway.
Blocks: 1244402
Component: Graphics: Layers → Panning and Zooming
Flags: needinfo?(bugmail.mozilla)
Assignee: nobody → bugmail.mozilla
Attached patch Part 2 - FixSplinter Review
As opposed to the long-tap case, if we do a tap followed quickly by a wheel event, we do want the tap to get dispatched (because it was fully completed before the wheel event came along). However we don't want it dispatched if it was during a fast fling. So I moved the fast-fling check into GEL for this case (which I'm not really happy about but it felt like the best option) and allowed the GenerateSingleTap to go through even if there was no current touch block. There's still an issue (as you pointed out) about mSingleTapOccurred not getting set in time but that's pre-existing.
Attachment #8731314 - Flags: review?(botond)
Attachment #8731312 - Flags: review?(botond) → review+
Comment on attachment 8731314 [details] [diff] [review]
Part 2 - Fix

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +2076,5 @@
> +      // verified that the single tap is allowed so we let it through.
> +      // XXX there is a bug here that in such a case the touch block that
> +      // generated this tap will not get its mSingleTapOccurred flag set.
> +      // See https://bugzilla.mozilla.org/show_bug.cgi?id=1256344#c6
> +      if (touch && !touch->SetSingleTapOccurred()) {

This is a pre-existing issue, but I'm not a big fan of the way mSingleTapOccurred and mDuringFastFling are entangled. Can we instead do:

  if (touch) {
    if (touch->IsDuringFastFling()) {
      // ignore
    }
    touch->SetSingleTapOccurred();
  }

(and make SetSingleTapOccurred() *just* set mSingleTapOccurred)?

@@ +2077,5 @@
> +      // XXX there is a bug here that in such a case the touch block that
> +      // generated this tap will not get its mSingleTapOccurred flag set.
> +      // See https://bugzilla.mozilla.org/show_bug.cgi?id=1256344#c6
> +      if (touch && !touch->SetSingleTapOccurred()) {
> +        APZC_LOG("%p dropping single-tap because it failed SetSingleTapOccurred\n", this);

(Thsi comment can then also be clearer, "it's during a fast fling" rather than "it failed SetSingleTapOccurred".)
Attachment #8731314 - Flags: review?(botond) → review+
Yeah, good call. I'll update the patch accordingly.
https://hg.mozilla.org/mozilla-central/rev/7a7c00aa2397
https://hg.mozilla.org/mozilla-central/rev/772bdc0bc0ea
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.