Bug 81741 – [Crash] Crash by right click if <meter> is in a shadow subtree.

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: DOM

[Shinya Kawanaka]

I found Repro for Bug 81311 causes crash by right click.

[Shinya Kawanaka]

This is stackTrace in chromium port.

#0  0x00007f7ba6e5959d in WebCore::Node::document (this=0x0) at third_party/WebKit/Source
#1  0x00007f7ba774c0d2 in WebCore::VisibleSelection::firstRange (this=0x7f7ba435ec48)
    at third_party/WebKit/Source/WebCore/editing/VisibleSelection.cpp:132
#2  0x00007f7ba6e93583 in WebKit::WebViewImpl::caretOrSelectionRange (this=0x7f7ba436a800
    length=0x7fffecf67d68) at third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.c
#3  0x00007f7ba8bdae1e in RenderViewImpl::SyncSelectionIfRequired (this=0x7f7ba42d5400) a
#4  0x00007f7ba8bd1088 in RenderViewImpl::didChangeSelection (this=0x7f7ba42d5400, is_emp
    at content/renderer/render_view_impl.cc:1697
#5  0x00007f7ba6eb854d in WebKit::EditorClientImpl::respondToChangedSelection (this=0x7f7
    at third_party/WebKit/Source/WebKit/chromium/src/EditorClientImpl.cpp:266
#6  0x00007f7ba76fa910 in WebCore::Editor::respondToChangedSelection (this=0x7f7ba435eb60
    at third_party/WebKit/Source/WebCore/editing/Editor.cpp:482
#7  0x00007f7ba7707f52 in WebCore::Editor::respondToChangedSelection (this=0x7f7ba435eb60
    at third_party/WebKit/Source/WebCore/editing/Editor.cpp:2938
#8  0x00007f7ba7715295 in WebCore::FrameSelection::setSelection (this=0x7f7ba435ec20, new
    align=WebCore::FrameSelection::AlignCursorOnScrollIfNeeded, granularity=WebCore::Char
    at third_party/WebKit/Source/WebCore/editing/FrameSelection.cpp:298
#9  0x00007f7ba6e6590d in WebCore::FrameSelection::setSelection (this=0x7f7ba435ec20, sel
    granularity=WebCore::CharacterGranularity) at third_party/WebKit/Source/WebCore/editi
#10 0x00007f7ba7714eae in WebCore::FrameSelection::setNonDirectionalSelectionIfNeeded (th
    granularity=WebCore::CharacterGranularity, endpointsAdjustmentMode=WebCore::FrameSele
    at third_party/WebKit/Source/WebCore/editing/FrameSelection.cpp:241
#11 0x00007f7ba78a371c in WebCore::EventHandler::updateSelectionForMouseDownDispatchingSe
    targetNode=0x7f7ba436d380, newSelection=..., granularity=WebCore::CharacterGranularit
    at third_party/WebKit/Source/WebCore/page/EventHandler.cpp:409
#12 0x00007f7ba78a42d0 in WebCore::EventHandler::handleMousePressEventSingleClick (this=0
    at third_party/WebKit/Source/WebCore/page/EventHandler.cpp:544
#13 0x00007f7ba78a46b8 in WebCore::EventHandler::handleMousePressEvent (this=0x7f7ba435ed
    at third_party/WebKit/Source/WebCore/page/EventHandler.cpp:624
#14 0x00007f7ba78a7ca6 in WebCore::EventHandler::handleMousePressEvent (this=0x7f7ba435ed
    at third_party/WebKit/Source/WebCore/page/EventHandler.cpp:1617
#15 0x00007f7ba6e8e3fd in WebKit::WebViewImpl::mouseDown (this=0x7f7ba436a800, event=...)
    at third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp:499
#16 0x00007f7ba6e921f9 in WebKit::WebViewImpl::handleInputEvent (this=0x7f7ba436a800, inp
    at third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp:1596
#17 0x00007f7ba8bf2952 in RenderWidget::OnHandleInputEvent (this=0x7f7ba42d5400, message=
#18 0x00007f7ba8bf8dbb in IPC::Message::Dispatch<RenderWidget, RenderWidget> (msg=0x7f7b9
    sender=0x7f7ba42d5400, func=
    (void (RenderWidget::*)(RenderWidget * const, const IPC::Message &)) 0x7f7ba8bf2674 <
const&)>) at ./ipc/ipc_message.h:154
#19 0x00007f7ba8bf0a83 in RenderWidget::OnMessageReceived (this=0x7f7ba42d5400, message=.
#20 0x00007f7ba8bccb98 in RenderViewImpl::OnMessageReceived (this=0x7f7ba42d5400, message
    at content/renderer/render_view_impl.cc:838
#21 0x00007f7ba6b06b9c in MessageRouter::RouteMessage (this=0x7f7ba42868f0, msg=...) at c
#22 0x00007f7ba6b06b3e in MessageRouter::OnMessageReceived (this=0x7f7ba42868f0, msg=...)
#23 0x00007f7ba6a08691 in ChildThread::OnMessageReceived (this=0x7f7ba42868c8, msg=...) a
#24 0x00007f7ba6054544 in IPC::ChannelProxy::Context::OnDispatchMessage (this=0x7f7ba4287
    at ipc/ipc_channel_proxy.cc:268
#25 0x00007f7ba6057729 in base::internal::RunnableAdapter<void (IPC::ChannelProxy::Contex
    this=0x7fffecf6a710, object=0x7f7ba4287700, a1=...) at ./base/bind_internal.h:188
#26 0x00007f7ba60572ef in base::internal::InvokeHelper<false, void, base::internal::Runna
)(IPC::Message const&)>, void (IPC::ChannelProxy::Context* const&, IPC::Message const&)>:
 (IPC::ChannelProxy::Context::*)(IPC::Message const&)>, IPC::ChannelProxy::Context* const
    a1=@0x7f7b96893f20, a2=...) at ./base/bind_internal.h:896
#27 0x00007f7ba6056cae in base::internal::Invoker<2, base::internal::BindState<base::inte
:Context::*)(IPC::Message const&)>, void (IPC::ChannelProxy::Context*, IPC::Message const
ssage)>, void (IPC::ChannelProxy::Context*, IPC::Message const&)>::Run(base::internal::Bi
    at ./base/bind_internal.h:1254
#28 0x00007f7ba56f96c3 in base::Callback<void ()>::Run() const (this=0x7fffecf6aa68) at .
#29 0x00007f7ba5f9f3b0 in MessageLoop::RunTask (this=0x7fffecf6b370, pending_task=...) at
#30 0x00007f7ba5f9f4c7 in MessageLoop::DeferOrRunPendingTask (this=0x7fffecf6b370, pendin
#31 0x00007f7ba5f9fce9 in MessageLoop::DoWork (this=0x7fffecf6b370) at base/message_loop.
#32 0x00007f7ba5fa79bc in base::MessagePumpDefault::Run (this=0x7f7ba42af180, delegate=0x
    at base/message_pump_default.cc:28
#33 0x00007f7ba5f9f077 in MessageLoop::RunInternal (this=0x7fffecf6b370) at base/message_
#34 0x00007f7ba5f9ef2a in MessageLoop::RunHandler (this=0x7fffecf6b370) at base/message_l
#35 0x00007f7ba5f9e85f in MessageLoop::Run (this=0x7fffecf6b370) at base/message_loop.cc:
#36 0x00007f7ba8c072ee in RendererMain (parameters=...) at content/renderer/renderer_main
#37 0x00007f7ba5f066d1 in (anonymous namespace)::RunZygote (main_function_params=..., del
    at content/app/content_main_runner.cc:245
#38 0x00007f7ba5f068f6 in (anonymous namespace)::RunNamedProcessTypeMain (process_type="z
    delegate=0x7fffecf6bdd0) at content/app/content_main_runner.cc:290
#39 0x00007f7ba5f0711c in (anonymous namespace)::ContentMainRunnerImpl::Run (this=0x7f7ba
    at content/app/content_main_runner.cc:511
#40 0x00007f7ba5f06031 in content::ContentMain (argc=3, argv=0x7fffecf6bf38, delegate=0x7
    at content/app/content_main.cc:35
#41 0x00007f7ba514136d in ChromeMain (argc=3, argv=0x7fffecf6bf38) at chrome/app/chrome_m
#42 0x00007f7ba514132c in main (argc=3, argv=0x7fffecf6bf38) at chrome/app/chrome_exe_mai

[Shinya Kawanaka]

Created attachment 132988 [details]
Patch

[Shinya Kawanaka]

I'm not sure VisibleSelection can return null if anchorNode is null...

I think rniwa can review this...

[Ryosuke Niwa]

Why is this a security bug if it's a clean null crash?

[Ryosuke Niwa]

Comment on attachment 132988 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132988&action=review

> Source/WebCore/editing/VisibleSelection.cpp:133
> +    if (!start.anchorNode() || !end.anchorNode())

You should check start.isNull() and end.isNull() instead.

> ManualTests/meter-in-shadow.html:2
> +

Why do we need this blank line?

> ManualTests/meter-in-shadow.html:6
> +<head>
> +    <title>Hoge</title>
> +</head>

Do we really need this head & title?

[Shinya Kawanaka]

(In reply to comment #4)
> Why is this a security bug if it's a clean null crash?

Sorry, I didn't have a good policy to mark as security bug...
Chromium bugs were aften marked as security bug these days, so I used the same strategy...

[Shinya Kawanaka]

Created attachment 132992 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #5)
> (From update of attachment 132988 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132988&action=review
> 
> > Source/WebCore/editing/VisibleSelection.cpp:133
> > +    if (!start.anchorNode() || !end.anchorNode())
> 
> You should check start.isNull() and end.isNull() instead.

Done.

> 
> > ManualTests/meter-in-shadow.html:2
> > +
> 
> Why do we need this blank line?

Removed.

> 
> > ManualTests/meter-in-shadow.html:6
> > +<head>
> > +    <title>Hoge</title>
> > +</head>
> 
> Do we really need this head & title?

No. Removed.

[Ryosuke Niwa]

This is not a security bug.

[Ryosuke Niwa]

Comment on attachment 132992 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132992&action=review

> ManualTests/meter-in-shadow.html:6
> +var sr = new WebKitShadowRoot(container);

Please don't use abbreviations like sr. Also, can't you use event sender here?

[Shinya Kawanaka]

(In reply to comment #10)
> (From update of attachment 132992 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132992&action=review
> 
> > ManualTests/meter-in-shadow.html:6
> > +var sr = new WebKitShadowRoot(container);
> 
> Please don't use abbreviations like sr. Also, can't you use event sender here?

Oh. I completely forgot we have eventSender...
I'll try it before landing this.

[Shinya Kawanaka]

Created attachment 132999 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #10)
> (From update of attachment 132992 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132992&action=review
> 
> > ManualTests/meter-in-shadow.html:6
> > +var sr = new WebKitShadowRoot(container);
> 
> Please don't use abbreviations like sr. Also, can't you use event sender here?

Fixed for the abbreviation.

I've tried eventSender, but I couldn't reproduce the crash, maybe because chromium try to create selection by right click, but DRT won't. I'm not sure though...

[Radar WebKit Bug Importer]

<rdar://problem/11091895>

[Shinya Kawanaka]

I found that doubleclick also causes a crash and my previous patch didn't cover it. I'll update the patch soon.

[Shinya Kawanaka]

Hmm... in the current trunk this crash didn't happen as far as I tried on Linux and Mac.