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

Issue 607001 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

OOPIF: Debug crash in EventHandler::scrollBox when scrolling

Project Member Reported by creis@chromium.org, Apr 26 2016

Issue description

Version: 52.0.2717.0
OS: All

What steps will reproduce the problem?
(0) Start a debug build of Chrome with --site-per-process
(1) Visit http://csreis.github.io/tests/cross-site-iframe.html
(2) Click "Go cross-site (complex page)" to load the build waterfall in an OOPIF
(3) Scroll in the iframe

What is the expected output?

The iframe should scroll.

What do you see instead?

We crash in EventHandler::scrollBox on ASSERT(m_frame->isMainFrame());

Note that Nick is not seeing this in a trunk build on Windows, but we may be sync'd to different revisions.  Ken, can you take a look to see if it repros for you?


ASSERTION FAILED: m_frame->isMainFrame()
../../third_party/WebKit/Source/core/input/EventHandler.cpp(644) : blink::ScrollResult blink::EventHandler::scrollBox(blink::LayoutBox *, blink::ScrollGranularity, const blink::FloatSize &, const blink::FloatPoint &, const blink::FloatSize &, bool *)

Received signal 11 SEGV_MAPERR 0000fbadbeef
#0 0x7f92ed135bbe base::debug::StackTrace::StackTrace()
#1 0x7f92ed1356ff base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7f92daf64340 <unknown>
#3 0x7f92d5a8eafe blink::EventHandler::scrollBox()
#4 0x7f92d5a8f278 blink::EventHandler::physicalScroll()
#5 0x7f92d5a990bc blink::EventHandler::handleGestureScrollUpdate()
#6 0x7f92d5a961b0 blink::EventHandler::handleGestureScrollEvent()
#7 0x7f92d5a95c84 blink::EventHandler::handleGestureEvent()
#8 0x7f92e0e73477 blink::WebFrameWidgetImpl::handleGestureEvent()
#9 0x7f92e0e05dbc blink::PageWidgetDelegate::handleInputEvent()
#10 0x7f92e0e716ef blink::WebFrameWidgetImpl::handleInputEvent()
#11 0x7f92e7f1391c content::RenderWidgetInputHandler::HandleInputEvent()
#12 0x7f92e807b030 content::RenderWidget::OnHandleInputEvent()
#13 0x7f92e7ef9f3d _ZN4base20DispatchToMethodImplIPN7content16IdleUserDetectorEMS2_FvPKN5blink13WebInputEventERKN2ui11LatencyInfoENS1_22InputEventDispatchTypeEEJS7_S9_SC_EJLm0ELm1ELm2EEEEvRKT_T0_RKSt5tupleIJDpT1_EENS_13IndexSequenceIJXspT2_EEEE
#14 0x7f92e808a285 _ZN4base16DispatchToMethodIPN7content12RenderWidgetEMS2_FvPKN5blink13WebInputEventERKN2ui11LatencyInfoENS1_22InputEventDispatchTypeEEJS7_S9_SC_EEEvRKT_T0_RKSt5tupleIJDpT1_EE
#15 0x7f92e808a22f _ZN3IPC16DispatchToMethodIN7content12RenderWidgetEMS2_FvPKN5blink13WebInputEventERKN2ui11LatencyInfoENS1_22InputEventDispatchTypeEEvSt5tupleIJS6_S8_SB_EEEEvPT_T0_PT1_RKT2_
#16 0x7f92e8083cd9 _ZN3IPC8MessageTI30InputMsg_HandleInputEvent_MetaSt5tupleIJPKN5blink13WebInputEventEN2ui11LatencyInfoEN7content22InputEventDispatchTypeEEEvE8DispatchINS9_12RenderWidgetESE_vMSE_FvS6_RKS8_SA_EEEbPKNS_7MessageEPT_PT0_PT1_T2_
#17 0x7f92e807a026 content::RenderWidget::OnMessageReceived()
#18 0x7f92e4fac08b IPC::MessageRouter::RouteMessage()
#19 0x7f92e601ea48 content::ChildThreadImpl::ChildThreadMessageRouter::RouteMessage()
#20 0x7f92e4fac00e IPC::MessageRouter::OnMessageReceived()
#21 0x7f92e6022074 content::ChildThreadImpl::OnMessageReceived()
#22 0x7f92e4f720a8 IPC::ChannelProxy::Context::OnDispatchMessage()
#23 0x7f92e4f76736 _ZN4base8internal15RunnableAdapterIMN3IPC12ChannelProxy7ContextEFvRKNS2_7MessageEEE3RunIS4_JS7_EEEvRK13scoped_refptrIT_EDpOT0_
#24 0x7f92e4f7664e _ZN4base8internal12InvokeHelperILb0EvNS0_15RunnableAdapterIMN3IPC12ChannelProxy7ContextEFvRKNS3_7MessageEEEEE8MakeItSoIJRK13scoped_refptrIS5_ES8_EEEvSB_DpOT_
#25 0x7f92e4f765fd _ZN4base8internal7InvokerINS_13IndexSequenceIJLm0ELm1EEEENS0_9BindStateINS0_15RunnableAdapterIMN3IPC12ChannelProxy7ContextEFvRKNS6_7MessageEEEEFvPS8_SB_EJSF_SB_EEENS0_12InvokeHelperILb0EvSE_EEFvvEE3RunEPNS0_13BindStateBaseE
#26 0x7f92ed115f6e base::Callback<>::Run()
#27 0x7f92ed13b5ae base::debug::TaskAnnotator::RunTask()
#28 0x7f92d8f51ca4 scheduler::TaskQueueManager::ProcessTaskFromWorkQueue()
#29 0x7f92d8f4fc02 scheduler::TaskQueueManager::DoWork()
#30 0x7f92d8f574fe _ZN4base8internal15RunnableAdapterIMN9scheduler16TaskQueueManagerEFvNS_9TimeTicksEbEE3RunIJRKS4_RKbEEEvPS3_DpOT_
#31 0x7f92d8f573fa _ZN4base8internal12InvokeHelperILb1EvNS0_15RunnableAdapterIMN9scheduler16TaskQueueManagerEFvNS_9TimeTicksEbEEEE8MakeItSoINS_7WeakPtrIS4_EEJRKS5_RKbEEEvS8_T_DpOT0_
#32 0x7f92d8f57378 _ZN4base8internal7InvokerINS_13IndexSequenceIJLm0ELm1ELm2EEEENS0_9BindStateINS0_15RunnableAdapterIMN9scheduler16TaskQueueManagerEFvNS_9TimeTicksEbEEEFvPS7_S8_bEJNS_7WeakPtrIS7_EERS8_bEEENS0_12InvokeHelperILb1EvSB_EEFvvEE3RunEPNS0_13BindStateBaseE
#33 0x7f92ed115f6e base::Callback<>::Run()
#34 0x7f92ed13b5ae base::debug::TaskAnnotator::RunTask()
#35 0x7f92ed1b330c base::MessageLoop::RunTask()
#36 0x7f92ed1b35a8 base::MessageLoop::DeferOrRunPendingTask()
#37 0x7f92ed1b3772 base::MessageLoop::DoWork()
#38 0x7f92ed1c5083 base::MessagePumpDefault::Run()
#39 0x7f92ed1b2d1f base::MessageLoop::RunHandler()
#40 0x7f92ed2593c4 base::RunLoop::Run()
#41 0x7f92ed1b1d94 base::MessageLoop::Run()
#42 0x7f92e80a3cb8 content::RendererMain()
#43 0x7f92e8416036 content::RunZygote()
#44 0x7f92e84162e0 content::RunNamedProcessTypeMain()
#45 0x7f92e8417ee9 content::ContentMainRunnerImpl::Run()
#46 0x7f92e8415702 content::ContentMain()
#47 0x7f92edd135a0 ChromeMain
#48 0x7f92edd13532 main
#49 0x7f92d922aec5 __libc_start_main
#50 0x7f92edd13435 <unknown>

 

Comment 1 by kenrb@chromium.org, Apr 27 2016

Cc: bokan@chromium.org
bokan@:

This crash is an OOPIF regression from your CL https://codereview.chromium.org/1895323002. Any thoughts on this?

I am guessing OOPIFs shouldn't reach this... is the problem that DocumentElement::updateViewportAndApplyScroll() is interpreting a local root Document (i.e. a Document on a frame with a remote frame parent) as a main frame?

Comment 2 by bokan@chromium.org, Apr 27 2016

Cc: -bokan@chromium.org kenrb@chromium.org
Owner: bokan@chromium.org
Yep, neither OOPIF nor non-OOPIF should reach there but the problem must lie where we register the viewport apply scroll callback in Document.cpp. I've got a CL in the commit queue right now that modifies that code significantly. If it still repros after that lands I'll take a closer look and put up a fix tomorrow.

Comment 3 by bokan@chromium.org, Apr 29 2016

I just tried the repro in the original report and I don't get any crashes. Can someone confirm?

Comment 4 by kenrb@chromium.org, Apr 29 2016

Cc: dcheng@chromium.org
Is the idea that this might have fixed it?
https://codereview.chromium.org/1913843004/

I am building now with your change, but I have concerns with what is happening there. It looks like you are using Document::ownerElement() as a signal for whether this is the main frame's Document or not (ownerElement() => not main frame), but this is not an accurate generalization because I don't OOPIF Document's have ownerElements, which I interpreted to be the likely cause of this bug.

The ownerElement situation isn't fully resolved, as is evident by the call to Frame::deprecatedLocalOwner() that returns nullptr for OOPIFs.

Comment 5 by bokan@chromium.org, Apr 29 2016

Ok, I assumed ownerElement() would return non-null for an iframe document even in the OOPIF case. What's the correct way to check for that? FYI: there's plenty of code in event handling that uses that as an assumption.

Comment 6 by dcheng@chromium.org, Apr 29 2016

Hmm. There are several options:

1) Add a FrameOwner getter to Document, like Frame::owner() and fix the comment on Document::ownerElement().

2) Alternatively, just use document().frame()->owner() for this particular case (but you might also need to check frame() is not null, which is unfortunate...)

Comment 7 by kenrb@chromium.org, Apr 29 2016

#5: "FYI: there's plenty of code in event handling that uses that as an assumption."

I noticed that this morning. tbh I am surprised this hasn't resulted in a lot more bugs.

dcheng's first suggestion matches what I had in mind. The second is a bit awkward given all the places this is used.

Comment 8 by bokan@chromium.org, Apr 29 2016

Ok, I'll fix this with dcheng's suggestion and update the other usages shortly.
Project Member

Comment 9 by bugdroid1@chromium.org, May 10 2016

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

commit f66e6b578fce9b4aed705f973acc547235e73e2c
Author: bokan <bokan@chromium.org>
Date: Tue May 10 17:29:11 2016

Rename Document::ownerElement to localOwner and fix main frame checks.

Document::ownerElement returns nullptr if the owner is in a remote frame so the
new name is more clear of that.

I've updated call sites that were using !ownerElement as a signal that the
document was in the main frame to check the frame's isMainFrame method instead
since that should work in OOPIF scenarios.

BUG= 607001 

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

[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/dom/Document.h
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/dom/Element.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/dom/Fullscreen.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/dom/Node.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/html/ImageDocument.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/html/MediaDocument.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/html/PluginDocument.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/html/TextDocument.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/input/EventHandler.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/input/EventHandler.h
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/input/TouchActionUtil.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/input/TouchEventManager.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/inspector/InspectorResourceAgent.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/layout/LayoutBox.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/layout/LayoutView.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/loader/SinkDocument.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/page/TouchAdjustment.cpp
[modify] https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c/third_party/WebKit/Source/core/paint/ViewPainter.cpp

Comment 10 by creis@chromium.org, May 16 2016

bokan@: Can this be marked fixed now?  I don't see any of these crashes since 52.0.2733.0, which I think was the first Canary after your CL landed.  (Thanks!)

Comment 11 by creis@chromium.org, May 16 2016

Status: Fixed (was: Assigned)
Sounds like bokan@ is out, so I'll mark this fixed.

Sign in to add a comment