New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 702771 link

Starred by 12 users

Issue metadata

Status: Archived
Owner:
Closed: Mar 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 0
Type: Bug

Blocking:
issue 665083



Sign in to add a comment

Infinite recursion in blink::VisualViewport::setScaleAndLocation => blink::VisualViewport::didSetScaleOrLocation

Project Member Reported by diand...@chromium.org, Mar 17 2017

Issue description

Forked from bug #665083

...people are also talking about this in  bug #651989 , though that's a bit of a mega bug...

---

Example stack crawl (from report 0183239480000000):

...
...
...
0x300b5ba7	(chrome -VisualViewport.cpp:289 )	blink::VisualViewport::didSetScaleOrLocation(float, blink::FloatPoint const&)
0x300b5a2f	(chrome -VisualViewport.cpp:242 )	blink::VisualViewport::setScaleAndLocation(float, blink::FloatPoint const&)
0x300b5a77	(chrome -VisualViewport.cpp:181 )	blink::VisualViewport::clampToBoundaries()
0x300b5ba7	(chrome -VisualViewport.cpp:289 )	blink::VisualViewport::didSetScaleOrLocation(float, blink::FloatPoint const&)
... 775 more
0x300b5a77	(chrome -VisualViewport.cpp:181 )	blink::VisualViewport::clampToBoundaries()
0x300b5ba7	(chrome -VisualViewport.cpp:289 )	blink::VisualViewport::didSetScaleOrLocation(float, blink::FloatPoint const&)
0x300b5a2f	(chrome -VisualViewport.cpp:242 )	blink::VisualViewport::setScaleAndLocation(float, blink::FloatPoint const&)
0x300b5a77	(chrome -VisualViewport.cpp:181 )	blink::VisualViewport::clampToBoundaries()
...
...
...

---

* This bug seems to hit minnie (Asus Chromebook Flip) hard, but right now it's unclear why.

* We think this bug has been happening at least as of M54 (067c3fe080000000 is example), but crashes seemed to have spiked very high recently.

* It was hard to debug this crash previously due to issues with how the kernel stack guard page was working (see bug #665083 or the series ending in http://crosreview.com/453411).  That's fixed now so debugging should be easier as of:

R57 - 9202.53.0
R58 - 9334.11.0
R59 - 9368.0.0

---

One thought is that m_scale might be NaN, and NaN != NaN so we just keep recursing.  Unclear how it might have gotten to be NaN.  In my analysis of one minidump file I found:

(gdb) print $s16
$3 = nan(0x400000)

We're hoping that <https://codereview.chromium.org/2754323003> will help catch before we hit the infinite recursion so we can get better debugging.  That's supposed to go to M57, I think.


 
We still can't reproduce yet, but according the the dev who works on this code, it's related to pinch-zoom and scrolling.  If anyone who can reproduce can use this to find some nice steps, that'd be great.  Specifically:

1. Tablet mode, or laptop mode?
2. Do you use pinch to zoom on the screen?
3. Did you just scroll using the touchscreen, or scroll some other way?
4. Any particular pages that reproduce this?
5. Task bar showing?  Bookmark bar showing?
6. Any special zoom mode on the page?  Do you use Ctrl + or Ctrl - to zoom in and out too?
7. What do you have your screen resolution set to?
Project Member

Comment 2 by bugdroid1@chromium.org, Mar 18 2017

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

commit 3685db28900ee3e99c5e2e6ce8bfecb78805f1de
Author: bokan <bokan@chromium.org>
Date: Sat Mar 18 01:43:21 2017

Temporarily add CHECKs to VisualViewport::didSetScaleAndLocation

Adding CHECKS to help track down why we're getting infinite recursion in this
function. It looks like that may happen if the scale of offset ever become NaN.

BUG=665083, 702771 

Review-Url: https://codereview.chromium.org/2754323003
Cr-Commit-Position: refs/heads/master@{#457920}

[modify] https://crrev.com/3685db28900ee3e99c5e2e6ce8bfecb78805f1de/third_party/WebKit/Source/core/frame/VisualViewport.cpp

Project Member

Comment 3 by bugdroid1@chromium.org, Mar 18 2017

Labels: merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/12b6df011abab4773df645d7a39cdab42f1336b2

commit 12b6df011abab4773df645d7a39cdab42f1336b2
Author: David Bokan <bokan@chromium.org>
Date: Sat Mar 18 01:51:22 2017

Temporarily add CHECKs to VisualViewport::didSetScaleAndLocation

Adding CHECKS to help track down why we're getting infinite recursion in this
function. It looks like that may happen if the scale of offset ever become NaN.

BUG=665083, 702771 

Review-Url: https://codereview.chromium.org/2754323003
Cr-Commit-Position: refs/heads/master@{#457920}
(cherry picked from commit 3685db28900ee3e99c5e2e6ce8bfecb78805f1de)

Review-Url: https://codereview.chromium.org/2758773004 .
Cr-Commit-Position: refs/branch-heads/2987@{#846}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/12b6df011abab4773df645d7a39cdab42f1336b2/third_party/WebKit/Source/core/frame/VisualViewport.cpp

Comment 4 by bokan@chromium.org, Mar 20 2017

My debugging patch should be in 57.0.2987.115 and later but it seems it hasn't been pushed yet (at least it doesn't show up in chromecrash yet). FYI, I'm actually on vacation until Wednesday so I'm probably not a good owner for this. But I'll check in every few hours and if the CHECKs confirm my guess I can land a fix tonight.
Ketaki says that the build is still going through qual, so for now we're hanging tight.
Also: copying feedback from  bug #651989 :

---

To answer your question.
1. laptop mode
2. it happens without pinching or zooming.  It was happening so often, so
there were so me times that I had used pinching and zooming, but certainly
MANY, many times it happened without my doing that
3. scrolled down using touchpad
4.  happened frequently in google docs, but also happened on many other
sites
5. task bar is showing ( well if I'm correct about what a taskbar is)   the
bookmark bar is not showing unless I was opening up a new tab, then it
appears on that page
6. I have never used Ctrl plus or minus to zoom in or out
7. I have never changed the original settings for resolution.  Please tell
me how to find out and I will tell you.  If you mean the information listed
under advance settings for web content....font size- medium, page zoom 100%
I hope that can help in some way.
Cc: afakhry@chromium.org denniskempin@chromium.org adlr@chromium.org abodenha@chromium.org
OK, we've got an example crash: 1f8663a160000000 on 57.0.2987.115:

0x36b6ada2	(chrome -VisualViewport.cpp:266 )	blink::VisualViewport::didSetScaleOrLocation(float, blink::FloatPoint const&)
0x36b6ac87	(chrome -VisualViewport.cpp:242 )	blink::VisualViewport::setScaleAndLocation(float, blink::FloatPoint const&)
0x36b6afe1	(chrome -VisualViewport.cpp:323 )	blink::VisualViewport::magnifyScaleAroundAnchor(float, blink::FloatPoint const&)
0x36806a05	(chrome -WebViewImpl.cpp:2237 )	blink::WebViewImpl::handleInputEvent(blink::WebInputEvent const&)
0x37241527	(chrome -render_widget_input_handler.cc:309 )	content::RenderWidgetInputHandler::HandleInputEvent(blink::WebInputEvent const&, ui::LatencyInfo const&, content::InputEventDispatchType)
0x371c16ed	(chrome -tuple.h:91 )	content::RenderWidget::OnMessageReceived(IPC::Message const&)
0x371b8855	(chrome -render_view_impl.cc:1265 )	content::RenderViewImpl::OnMessageReceived(IPC::Message const&)
0x375ae7cf	(chrome -message_router.cc:56 )	IPC::MessageRouter::RouteMessage(IPC::Message const&)
0x3671bccd	(chrome -child_thread_impl.cc:750 )	content::ChildThreadImpl::OnMessageReceived(IPC::Message const&)
0x3723e8bd	(chrome -callback.h:85 )	content::InputEventFilter::HandleEventOnMainThread(int, blink::WebInputEvent const*, ui::LatencyInfo const&, content::InputEventDispatchType)
0x3724036f	(chrome -main_thread_event_queue.cc:198 )	content::MainThreadEventQueue::DispatchInFlightEvent()
0x372405a3	(chrome -main_thread_event_queue.cc:229 )	content::MainThreadEventQueue::DispatchSingleEvent()
0x33dce8c3	(chrome -callback.h:68 )	base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*)
0x33e79bd1	(chrome -task_queue_manager.cc:377 )	blink::scheduler::TaskQueueManager::ProcessTaskFromWorkQueue(blink::scheduler::internal::WorkQueue*, blink::scheduler::LazyNow, base::TimeTicks*)
0x33e7aa2b	(chrome -task_queue_manager.cc:245 )	blink::scheduler::TaskQueueManager::DoWork(base::TimeTicks, bool)
0x367c7dd7	(chrome -bind_internal.h:214 )	base::internal::Invoker<base::internal::BindState<void (blink::scheduler::TaskQueueManager::*)(base::TimeTicks, bool), base::WeakPtr<blink::scheduler::TaskQueueManager>, base::TimeTicks, bool>, void ()>::Run(base::internal::BindStateBase*)
0x33dce8c3	(chrome -callback.h:68 )	base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*)
0x33dc19ab	(chrome -message_loop.cc:421 )	base::MessageLoop::DoWork()
0x33dc1cff	(chrome -message_pump_default.cc:33 )	base::MessagePumpDefault::Run(base::MessagePump::Delegate*)
0x3510b3a9	(chrome -run_loop.cc:37 )	base::RunLoop::Run()
0x371c627d	(chrome -renderer_main.cc:200 )	content::RendererMain(content::MainFunctionParams const&)
0x34e63c65	(chrome -content_main_runner.cc:344 )	content::RunZygote(content::MainFunctionParams const&, content::ContentMainDelegate*)
0x34e63e65	(chrome -content_main_runner.cc:793 )	content::ContentMainRunnerImpl::Run()
0x34e62fbd	(chrome -content_main.cc:20 )	content::ContentMain(content::ContentMainParams const&)
0x33f6a04f	(chrome -chrome_main.cc:112 )	ChromeMain
0xa74a087d	(libc-2.23.so -libc-start.c:289 )	__libc_start_main
0x33f69efb	(chrome + 0x00797efb )	_start
0x37e1cd67	(chrome -elf-init.c:87 )	__libc_csu_init
0xa7af1e43	(ld-2.23.so + 0x0000be43 )	_dl_sort_fini

--

Source code:

https://chromium.googlesource.com/chromium/src/+/57.0.2987.115/third_party/WebKit/Source/core/frame/VisualViewport.cpp

Line 266:

  CHECK(std::isfinite(clampedOffset.width()) &&
        std::isfinite(clampedOffset.height()));

--

Source code:

https://chromium.googlesource.com/chromium/src/+/57.0.2987.115/third_party/WebKit/Source/web/WebViewImpl.cpp

  // Unhandled pinch events should adjust the scale.
  if (inputEvent.type() == WebInputEvent::GesturePinchUpdate) {
    const WebGestureEvent& pinchEvent =
        static_cast<const WebGestureEvent&>(inputEvent);
    // For touchpad gestures synthesize a Windows-like wheel event
    // to send to any handlers that may exist. Not necessary for touchscreen
    // as touch events would have already been sent for the gesture.
    if (pinchEvent.sourceDevice == WebGestureDeviceTouchpad) {
      result = handleSyntheticWheelFromTouchpadPinchEvent(pinchEvent);
      if (result != WebInputEventResult::NotHandled)
        return result;
    }
    if (pinchEvent.data.pinchUpdate.zoomDisabled)
      return WebInputEventResult::NotHandled;
==> if (page()->frameHost().visualViewport().magnifyScaleAroundAnchor(
            pinchEvent.data.pinchUpdate.scale,
            FloatPoint(pinchEvent.x, pinchEvent.y)))
      return WebInputEventResult::HandledSystem;
  }

Line 2237 is marked with "==>"






Status: Assigned (was: Untriaged)
From offline discussion: We've discovered that touchpad pinch-to-zoom was incorrectly enabled on minnie, and only on minnie. There's a strong correlation with when that happened and when this bug appeared.

Obvious next step here is to disable touchpad pinch to zoom on minnie.
@8: Yup, I think we want to both (A) disable pinch-to-zoom and (B) also make this infinite recursion impossible.

Here's (A):

https://chromium-review.googlesource.com/457556
Agreed. Fixing both is definitely the right way to go.

Comment 11 by bokan@chromium.org, Mar 21 2017

The traces I've seen would only occur via touchpad pinch-zoom. Agree we should fix (B) too though, got a patch in progress that'll replace the CHECKs to keep us running and I'll add a TODO to remove the reentrancy so we can avoid this risk altogether.
Funny.  Even though the original "pinch to zoom" landed in M-53 <https://chromium-review.googlesource.com/337958>, it was a puzzle about why the "__divdi3" issues (bug #665083) showed up in M-54.

The answer is that we landed a backported revert in M-53 at <https://chromium-review.googlesource.com/c/365061>.  ...but that revert never landed in ToT (see <https://chromium-review.googlesource.com/c/365060>).  

So that makes it pretty clear why this issue showed up starting at M-54.  That's the first build that shipped out where pinch-to-zoom via Trackpad was turned on.
Project Member

Comment 13 by bugdroid1@chromium.org, Mar 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/xorg-conf/+/c72b5f688bfe853ca5d0788c90fde6149c2b6a69

commit c72b5f688bfe853ca5d0788c90fde6149c2b6a69
Author: Douglas Anderson <dianders@chromium.org>
Date: Tue Mar 21 21:04:03 2017

touchpad-cmt: Disable pinch to zoom on minnie trackpad

Pinch to zoom is a huge suspect for a big crasher on minnie.  It also
doesn't work terribly well.  Disable it until these issues are solved.

BUG= chromium:702771 
TEST=minnie trackpad pinch to zoom no longer works.

Change-Id: I7d4e70747cabbe6fff5dcc3e03934b6510de6a09
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/457556
Reviewed-by: Dennis Kempin <denniskempin@google.com>
Reviewed-by: Andrew de los Reyes <adlr@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Ketaki Deshpande <ketakid@chromium.org>

[modify] https://crrev.com/c72b5f688bfe853ca5d0788c90fde6149c2b6a69/60-touchpad-cmt-veyron_minnie.conf

Cc: bhthompson@chromium.org
Labels: M-58
Officially requesting merge of the above CL to M-57 and M-58.  I'll let TPMs decide when they're happy enough with testing and when they actually want it merged.
Labels: Merge-Approved-58 Merge-Request-57
SGTM for 58.
Re #12 Nice detective work. Yet another argument for always landing in TOT first.
Project Member

Comment 17 by bugdroid1@chromium.org, Mar 21 2017

Labels: merge-merged-release-R58-9334.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/xorg-conf/+/5a099b385c9f6bb844d3d7aed3aaddc82fb829ef

commit 5a099b385c9f6bb844d3d7aed3aaddc82fb829ef
Author: Douglas Anderson <dianders@chromium.org>
Date: Tue Mar 21 22:41:08 2017

touchpad-cmt: Disable pinch to zoom on minnie trackpad

Pinch to zoom is a huge suspect for a big crasher on minnie.  It also
doesn't work terribly well.  Disable it until these issues are solved.

BUG= chromium:702771 
TEST=minnie trackpad pinch to zoom no longer works.

Change-Id: I7d4e70747cabbe6fff5dcc3e03934b6510de6a09
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/457556
Reviewed-by: Dennis Kempin <denniskempin@google.com>
Reviewed-by: Andrew de los Reyes <adlr@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Ketaki Deshpande <ketakid@chromium.org>
(cherry picked from commit c72b5f688bfe853ca5d0788c90fde6149c2b6a69)
Reviewed-on: https://chromium-review.googlesource.com/457433

[modify] https://crrev.com/5a099b385c9f6bb844d3d7aed3aaddc82fb829ef/60-touchpad-cmt-veyron_minnie.conf

Project Member

Comment 18 by bugdroid1@chromium.org, Mar 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/xorg-conf/+/5a099b385c9f6bb844d3d7aed3aaddc82fb829ef

commit 5a099b385c9f6bb844d3d7aed3aaddc82fb829ef
Author: Douglas Anderson <dianders@chromium.org>
Date: Tue Mar 21 22:41:08 2017

touchpad-cmt: Disable pinch to zoom on minnie trackpad

Pinch to zoom is a huge suspect for a big crasher on minnie.  It also
doesn't work terribly well.  Disable it until these issues are solved.

BUG= chromium:702771 
TEST=minnie trackpad pinch to zoom no longer works.

Change-Id: I7d4e70747cabbe6fff5dcc3e03934b6510de6a09
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/457556
Reviewed-by: Dennis Kempin <denniskempin@google.com>
Reviewed-by: Andrew de los Reyes <adlr@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Ketaki Deshpande <ketakid@chromium.org>
(cherry picked from commit c72b5f688bfe853ca5d0788c90fde6149c2b6a69)
Reviewed-on: https://chromium-review.googlesource.com/457433

[modify] https://crrev.com/5a099b385c9f6bb844d3d7aed3aaddc82fb829ef/60-touchpad-cmt-veyron_minnie.conf

Labels: -Merge-Approved-58 merge-merged-58

Comment 20 by bokan@chromium.org, Mar 22 2017

I landed the patch mentioned in #11 but used the wrong bug number on the commit. Commit message below:

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

commit 582807cc0713109b51ec8e1adca6d04879a00c18
Author: bokan <bokan@chromium.org>
Date: Tue Mar 21 21:16:06 2017

Avoid setting visual viewport location/scale if it's infinite/NaN

This CL replaces CHECKs added in r457920 with early-outs. Crash stats
revealed we can enter this method with the location argument being
infinity. Seems to only happen on ChromeOS with touchpad pinch-zoom.

BUG=702711

Review-Url: https://codereview.chromium.org/2768603002
Cr-Commit-Position: refs/heads/master@{#458548}

[modify] https://crrev.com/582807cc0713109b51ec8e1adca6d04879a00c18/third_party/WebKit/Source/core/frame/VisualViewport.cpp
Project Member

Comment 21 by bugdroid1@chromium.org, Mar 22 2017

Labels: merge-merged-release-R57-9202.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/xorg-conf/+/6043b27c3573c14cfb5edcedc698863545efc3d1

commit 6043b27c3573c14cfb5edcedc698863545efc3d1
Author: Douglas Anderson <dianders@chromium.org>
Date: Wed Mar 22 18:40:16 2017

touchpad-cmt: Disable pinch to zoom on minnie trackpad

Pinch to zoom is a huge suspect for a big crasher on minnie.  It also
doesn't work terribly well.  Disable it until these issues are solved.

BUG= chromium:702771 
TEST=minnie trackpad pinch to zoom no longer works.

Change-Id: I7d4e70747cabbe6fff5dcc3e03934b6510de6a09
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/457556
Reviewed-by: Dennis Kempin <denniskempin@google.com>
Reviewed-by: Andrew de los Reyes <adlr@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Ketaki Deshpande <ketakid@chromium.org>
(cherry picked from commit c72b5f688bfe853ca5d0788c90fde6149c2b6a69)
Reviewed-on: https://chromium-review.googlesource.com/458520

[modify] https://crrev.com/6043b27c3573c14cfb5edcedc698863545efc3d1/60-touchpad-cmt-veyron_minnie.conf

Labels: -Merge-Request-57 Merge-Merged
Status: Fixed (was: Assigned)
As per IM with ketaki, landed disable trackpad pinch-to-zoom in M-57.

bokan's fix will stay m-59
just updated (ref below) - aw-snap/tab crashing on Asus Flip not resolved - multiple tab crashed within minutes - active tab as well as some not active tabs (google inbox and calendar tabs unaffected)

Version 57.0.2987.123 beta
Platform 9202.56.1 (Official Build) beta-channel veyron_minnie
ARC Version 3831942
Firmware Google_Veyron_Minnie.6588.237.0


@23: Yup, you gotta wait for one of these (or newer):

R57 - 9202.58.0
R58 - 9334.18.0
R59 - 9389.0.0

I can't promise when that R57 will be pushed, but that's what you need to look for.
so far so good... no "aw snaps" since yesterday afternoon (GMT+11) upgrade

Version 58.0.3029.31 beta
Platform 9334.18.0 (Official Build) beta-channel veyron_minnie
ARC Version 3836087
Firmware Google_Veyron_Minnie.6588.237.0

last crash (of about 50) for the past week:

Crash ID ChromeOS (Server ID: cc02046360000000)
Crash report uploaded on Wednesday, 29 March 2017 at 15:26:29

keeping fingers crossed for now

Comment 26 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Sign in to add a comment