New issue
Advanced search Search tips

Issue 897791 link

Starred by 1 user

Issue metadata

Status: Fixed
Merged: issue 897216
Owner:
Closed: Dec 4
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Bug

Blocking:
issue 654917



Sign in to add a comment

Trackpad pinch-zoom over an oopif on Mac triggers DCHECK in InputProxyHandler::HandleGestureScrollEvent()

Project Member Reported by wjmaclean@chromium.org, Oct 22

Issue description

Chrome Version: 72.0.3589.0 (Developer Build) (64-bit)
OS: MacOSX

What steps will reproduce the problem?
(1) open http://csreis.github.io/tests/cross-site-iframe-simple.html
(2) ensure --site-per-process is enabled
(3) perform a trackpad pinch zoom with the mouse cursor over the iframe

What is the expected result?

The page should pinch-zoom without crashing

What happens instead?

In DCHECK-enabled build, we hit

https://cs.chromium.org/chromium/src/ui/events/blink/input_handler_proxy.cc?rcl=bcab5d9da418dc4667e991fb2f7d50375c785a13&l=650

which is

DCHECK(expect_scroll_update_end_);

inside

InputHandlerProxy::HandleGestureScrollUpdate()

 
Blocking: 654917
Labels: Hotlist-DesktopUIConsider
Components: Blink>Input
Status: Untriaged (was: Available)
Mergedinto: 897216
Status: Duplicate (was: Untriaged)
The DCHECKs in Render_widget_host_impl.cc are removed in the following cl:
https://chromium-review.googlesource.com/c/130156

By disabling the related DCHECKSs in RWHI the incorrect sequence gets caught in InputHandlerProxy instead.
I tried looking at the CL referenced in C#5, and it doesn't seem to exist?
In any case, if the CL is the one I used to disable DCHECKs in RWHI, then I'll point out that this bug was filed before the DCHECKs were disables, so I'm not sure I agree it's a duplicate.
Status: Untriaged (was: Duplicate)
I missed the date that the cl landed since the two issues looked similar (both related to touchpad pinch zoom and oopif)

Thanks for pointing out, I will remove the duplication.
Labels: -Hotlist-DesktopUIConsider
Cc: bokan@chromium.org
Labels: -Pri-1 Pri-3
Owner: mcnee@chromium.org
Status: Assigned (was: Untriaged)
Kevin, mind taking a look when you're back?
Project Member

Comment 12 by bugdroid1@chromium.org, Nov 22

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c88cbd51a3c881e9999d4b34700f3be333764257

commit c88cbd51a3c881e9999d4b34700f3be333764257
Author: Kevin McNee <mcnee@chromium.org>
Date: Thu Nov 22 19:14:16 2018

Use DCHECK_IS_ON() to determine if DCHECKs are enabled in InputHandlerProxy

We use NDEBUG for the condition to enable DCHECKs related to scroll
event sequence validity, but this doesn't consider that DCHECKs could be
enabled in release builds with dcheck_always_on.

Bug:  897791 
Change-Id: I8bf894b9fcfd56d5c8256bafa4d00b3e1eb37ed2
Reviewed-on: https://chromium-review.googlesource.com/c/1348253
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610473}
[modify] https://crrev.com/c88cbd51a3c881e9999d4b34700f3be333764257/ui/events/blink/input_handler_proxy.cc
[modify] https://crrev.com/c88cbd51a3c881e9999d4b34700f3be333764257/ui/events/blink/input_handler_proxy.h

Status: Started (was: Assigned)
Project Member

Comment 14 by bugdroid1@chromium.org, Dec 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b9baa81a2d9f273341371f96e181a7ac51e43a41

commit b9baa81a2d9f273341371f96e181a7ac51e43a41
Author: Kevin McNee <mcnee@chromium.org>
Date: Mon Dec 03 21:11:16 2018

Don't logically coalesce touchpad pinch events with scroll updates

Unlike touchscreen pinch events, touchpad pinch events don't occur
within a gesture scroll sequence. If a touchpad pinch update couldn't
be directly coalesced with a previous pinch update, we now just enqueue
the pinch update rather than producing a scroll update.

We also now allow for pinch updates to be coalesced if their anchors are
approximately equal to accommodate OOPIF coordinate conversion
imprecision.

Bug:  897791 
Change-Id: I3331fc5761b5ca11d4b05386e5ab4468ac2291d7
Reviewed-on: https://chromium-review.googlesource.com/c/1349851
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613245}
[modify] https://crrev.com/b9baa81a2d9f273341371f96e181a7ac51e43a41/ui/events/blink/blink_event_util.cc
[modify] https://crrev.com/b9baa81a2d9f273341371f96e181a7ac51e43a41/ui/events/blink/blink_event_util_unittest.cc
[modify] https://crrev.com/b9baa81a2d9f273341371f96e181a7ac51e43a41/ui/events/blink/compositor_thread_event_queue.cc
[modify] https://crrev.com/b9baa81a2d9f273341371f96e181a7ac51e43a41/ui/events/blink/input_handler_proxy_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment