Closed
Bug 1256341
Opened 8 years ago
Closed 8 years ago
crash in mozilla::layers::AsyncPanZoomController::GenerateSingleTap
Categories
(Core :: Panning and Zooming, defect)
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)
1.73 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
7.29 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
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()
Reporter | ||
Comment 1•8 years ago
|
||
Fwiw, this signature is currently at #31 in Firefox 48.0a1 Top Crash list. (Together with bug 1256344 they would be at #10.)
Comment 2•8 years ago
|
||
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
Comment 3•8 years ago
|
||
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().
Assignee | ||
Comment 4•8 years ago
|
||
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.
Comment 5•8 years ago
|
||
(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!
Assignee | ||
Comment 6•8 years ago
|
||
I'll take a crack at fixing this since I'm looking at bug 1256344 anyway.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bugmail.mozilla
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8731312 -
Flags: review?(botond)
Assignee | ||
Comment 8•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8731312 -
Flags: review?(botond) → review+
Comment 9•8 years ago
|
||
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+
Assignee | ||
Comment 10•8 years ago
|
||
Yeah, good call. I'll update the patch accordingly.
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a7c00aa2397 https://hg.mozilla.org/integration/mozilla-inbound/rev/772bdc0bc0ea
Comment 12•8 years ago
|
||
bugherder |
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.
Description
•