Bug 81741 - [Crash] Crash by right click if <meter> is in a shadow subtree.
Summary: [Crash] Crash by right click if <meter> is in a shadow subtree.
Status: RESOLVED WORKSFORME
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shinya Kawanaka
URL:
Keywords: InRadar
Depends on: 82021
Blocks: 72352 82697
  Show dependency tree / graph
 
Reported: 2012-03-20 22:13 PDT by Shinya Kawanaka
Modified: 2012-05-10 00:33 PDT (History)
9 users (show)

See Also:

Attachments
Patch (2.73 KB, patch)
2012-03-21 01:28 PDT, Shinya Kawanaka 		no flags Details | Formatted Diff | Diff
Patch (2.67 KB, patch)
2012-03-21 01:44 PDT, Shinya Kawanaka 		no flags Details | Formatted Diff | Diff
Patch (2.69 KB, patch)
2012-03-21 02:39 PDT, Shinya Kawanaka 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2012-03-20 22:13:24 PDT 
I found Repro for Bug 81311 causes crash by right click.
Comment 1 Shinya Kawanaka 2012-03-20 22:35:29 PDT 
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
Comment 2 Shinya Kawanaka 2012-03-21 01:28:42 PDT 
Created attachment 132988 [details]
Patch
Comment 3 Shinya Kawanaka 2012-03-21 01:30:30 PDT 
I'm not sure VisibleSelection can return null if anchorNode is null...

I think rniwa can review this...
Comment 4 Ryosuke Niwa 2012-03-21 01:31:48 PDT 
Why is this a security bug if it's a clean null crash?
Comment 5 Ryosuke Niwa 2012-03-21 01:33:39 PDT 
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?
Comment 6 Shinya Kawanaka 2012-03-21 01:43:19 PDT 
(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...
Comment 7 Shinya Kawanaka 2012-03-21 01:44:51 PDT 
Created attachment 132992 [details]
Patch
Comment 8 Shinya Kawanaka 2012-03-21 01:45:48 PDT 
(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.
Comment 9 Ryosuke Niwa 2012-03-21 01:53:19 PDT 
This is not a security bug.
Comment 10 Ryosuke Niwa 2012-03-21 01:54:14 PDT 
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?
Comment 11 Shinya Kawanaka 2012-03-21 02:11:32 PDT 
(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.
Comment 12 Shinya Kawanaka 2012-03-21 02:39:10 PDT 
Created attachment 132999 [details]
Patch
Comment 13 Shinya Kawanaka 2012-03-21 02:40:50 PDT 
(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...
Comment 14 Radar WebKit Bug Importer 2012-03-21 11:14:03 PDT 
<rdar://problem/11091895>
Comment 15 Shinya Kawanaka 2012-03-21 23:35:32 PDT 
I found that doubleclick also causes a crash and my previous patch didn't cover it. I'll update the patch soon.
Comment 16 Shinya Kawanaka 2012-05-10 00:30:00 PDT 
Hmm... in the current trunk this crash didn't happen as far as I tried on Linux and Mac.