Infinite recursion in blink::VisualViewport::setScaleAndLocation => blink::VisualViewport::didSetScaleOrLocation |
|||||||||||
Issue descriptionForked 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.
,
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
,
Mar 18 2017
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
,
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.
,
Mar 20 2017
Ketaki says that the build is still going through qual, so for now we're hanging tight.
,
Mar 20 2017
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.
,
Mar 21 2017
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 "==>"
,
Mar 21 2017
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.
,
Mar 21 2017
@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
,
Mar 21 2017
Agreed. Fixing both is definitely the right way to go.
,
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.
,
Mar 21 2017
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.
,
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
,
Mar 21 2017
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.
,
Mar 21 2017
SGTM for 58.
,
Mar 21 2017
Re #12 Nice detective work. Yet another argument for always landing in TOT first.
,
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
,
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
,
Mar 21 2017
,
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
,
Mar 22 2017
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
,
Mar 22 2017
As per IM with ketaki, landed disable trackpad pinch-to-zoom in M-57. bokan's fix will stay m-59
,
Mar 25 2017
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
,
Mar 27 2017
@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.
,
Mar 30 2017
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
,
Jan 22 2018
|
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by diand...@chromium.org
, Mar 17 2017