Bug 37801 - [v8] Do not pass empty handle into SetHiddenValue which would crash
Summary: [v8] Do not pass empty handle into SetHiddenValue which would crash
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-19 07:51 PDT by anton muhin
Modified: 2010-04-22 03:41 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.32 KB, patch)
2010-04-19 07:55 PDT, anton muhin
no flags Details | Formatted Diff | Diff
Add an assert (1.60 KB, patch)
2010-04-21 05:41 PDT, anton muhin
no flags Details | Formatted Diff | Diff
Next iteration (2.10 KB, patch)
2010-04-21 10:53 PDT, anton muhin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description anton muhin 2010-04-19 07:51:29 PDT
[v8] Do not pass empty handle into SetHiddenPrototype which would crash event
Comment 1 anton muhin 2010-04-19 07:53:06 PDT
Sample stack trace:

0x025ec87a	 [chrome.dll	 - handles.cc:217]	v8::internal::SetProperty(v8::internal::Handle<v8::internal::JSObject>,v8::internal::Handle<v8::internal::String>,v8::internal::Handle<v8::internal::Object>,PropertyAttributes)
0x0265d733	 [chrome.dll	 - runtime.cc:3799]	v8::internal::Runtime::SetObjectProperty(v8::internal::Handle<v8::internal::Object>,v8::internal::Handle<v8::internal::Object>,v8::internal::Handle<v8::internal::Object>,PropertyAttributes)
0x025eb454	 [chrome.dll	 - objects.cc:6412]	v8::internal::JSObject::GetLocalPropertyPostInterceptor(v8::internal::JSObject *,v8::internal::String *,PropertyAttributes *)
0x025ec9fa	 [chrome.dll	 - handles.cc:226]	v8::internal::SetProperty(v8::internal::Handle<v8::internal::Object>,v8::internal::Handle<v8::internal::Object>,v8::internal::Handle<v8::internal::Object>,PropertyAttributes)
0x025c7cd2	 [chrome.dll	 - api.cc:2393]	v8::Object::SetHiddenValue(v8::Handle<v8::String>,v8::Handle<v8::Value>)
0x025ccbff	 [chrome.dll	 - api.cc:3069]	v8::Context::Global()
0x01e62d06	 [chrome.dll	 - v8abstracteventlistener.cpp:140]	WebCore::V8AbstractEventListener::invokeEventHandler(WebCore::ScriptExecutionContext *,WebCore::Event *,v8::Handle<v8::Value>)
0x01e62bc5	 [chrome.dll	 - v8abstracteventlistener.cpp:90]	WebCore::V8AbstractEventListener::handleEvent(WebCore::ScriptExecutionContext *,WebCore::Event *)
0x01d3d0af	 [chrome.dll	 - eventtarget.cpp:315]	WebCore::EventTarget::fireEventListeners(WebCore::Event *,WebCore::EventTargetData *,WTF::Vector<WebCore::RegisteredEventListener,1> &)
0x01d3cfa7	 [chrome.dll	 - eventtarget.cpp:276]	WebCore::EventTarget::fireEventListeners(WebCore::Event *)
0x01c9161d	 [chrome.dll	 - node.cpp:2665]	WebCore::Node::dispatchGenericEvent(WTF::PassRefPtr<WebCore::Event>)
0x01c913b4	 [chrome.dll	 - node.cpp:2567]	WebCore::Node::dispatchEvent(WTF::PassRefPtr<WebCore::Event>)
0x01c91c2f	 [chrome.dll	 - node.cpp:2859]	WebCore::Node::dispatchMouseEvent(WebCore::AtomicString const &,int,int,int,int,int,int,bool,bool,bool,bool,bool,WebCore::Node *,WTF::PassRefPtr<WebCore::Event>)
0x01c918ef	 [chrome.dll	 - node.cpp:2768]	WebCore::Node::dispatchMouseEvent(WebCore::PlatformMouseEvent const &,WebCore::AtomicString const &,int,WebCore::Node *)
0x01cc7f62	 [chrome.dll	 - eventhandler.cpp:1782]	WebCore::EventHandler::dispatchMouseEvent(WebCore::AtomicString const &,WebCore::Node *,bool,int,WebCore::PlatformMouseEvent const &,bool)
0x01cc758f	 [chrome.dll	 - eventhandler.cpp:1451]	WebCore::EventHandler::handleMouseMoveEvent(WebCore::PlatformMouseEvent const &,WebCore::HitTestResult *)
0x01cc71cc	 [chrome.dll	 - eventhandler.cpp:1338]	WebCore::EventHandler::mouseMoved(WebCore::PlatformMouseEvent const &)
0x01e91478	 [chrome.dll	 - webviewimpl.cpp:308]	WebKit::WebViewImpl::mouseMove(WebKit::WebMouseEvent const &)
0x01e9227c	 [chrome.dll	 - webviewimpl.cpp:1015]	WebKit::WebViewImpl::handleInputEvent(WebKit::WebInputEvent const &)
0x02012952	 [chrome.dll	 - render_widget.cc:315]	RenderWidget::OnHandleInputEvent(IPC::Message const &)
0x0201257c	 [chrome.dll	 - render_widget.cc:147]	RenderWidget::OnMessageReceived(IPC::Message const &)
0x01fe857c	 [chrome.dll	 - render_view.cc:643]	RenderView::OnMessageReceived(IPC::Message const &)
0x022687fa	 [chrome.dll	 - message_router.cc:40]	MessageRouter::RouteMessage(IPC::Message const &)
0x022687d4	 [chrome.dll	 - message_router.cc:31]	MessageRouter::OnMessageReceived(IPC::Message const &)
0x0226749a	 [chrome.dll	 - child_thread.cc:146]	ChildThread::OnMessageReceived(IPC::Message const &)
0x02153270	 [chrome.dll	 - task.h:296]	RunnableMethod<CancelableRequest<CallbackRunner<Tuple0> >,void ( CancelableRequest<CallbackRunner<Tuple0> >::*)(Tuple0 const &),Tuple1<Tuple0> >::Run()
0x01fc2b6f	 [chrome.dll	 - message_loop.cc:329]	MessageLoop::RunTask(Task *)
0x01fc2bac	 [chrome.dll	 - message_loop.cc:337]	MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const &)
0x01fc2d42	 [chrome.dll	 - message_loop.cc:444]	MessageLoop::DoWork()
0x01fd321f	 [chrome.dll	 - message_pump_default.cc:50]	base::MessagePumpDefault::Run(base::MessagePump::Delegate *)
...... (23 stack frames dropped.)
0x01d8cc4b	 [chrome.dll	 - v8proxy.cpp:492]	WebCore::V8Proxy::callFunction(v8::Handle<v8::Function>,v8::Handle<v8::Object>,int,v8::Handle<v8::Value> * const)
0x01d8afbb	 [chrome.dll	 - v8domwrapper.cpp:385]	WebCore::V8DOMWrapper::convertEventTargetToV8Object(WebCore::EventTarget *)
0x01d35272	 [chrome.dll	 - scriptcontrollerbase.cpp:41]	WebCore::ScriptController::canExecuteScripts(WebCore::ReasonForCallingCanExecuteScripts)
0x01e2e980	 [chrome.dll	 - v8lazyeventlistener.cpp:69]	WebCore::V8LazyEventListener::callListenerFunction(WebCore::ScriptExecutionContext *,v8::Handle<v8::Value>,WebCore::Event *)
0x01e62d24	 [chrome.dll	 - v8abstracteventlistener.cpp:144]	WebCore::V8AbstractEventListener::invokeEventHandler(WebCore::ScriptExecutionContext *,WebCore::Event *,v8::Handle<v8::Value>)
0x023577bd	 [chrome.dll	 - v8mouseevent.cpp:435]	WebCore::toV8(WebCore::MouseEvent *)
0x01e62bc5	 [chrome.dll	 - v8abstracteventlistener.cpp:90]	WebCore::V8AbstractEventListener::handleEvent(WebCore::ScriptExecutionContext *,WebCore::Event *)
0x01d9faff	 [chrome.dll	 - svgfilterbuilder.cpp:62]	WebCore::SVGFilterBuilder::getEffectById(WebCore::AtomicString const &)
0x01d3d0af	 [chrome.dll	 - eventtarget.cpp:315]	WebCore::EventTarget::fireEventListeners(WebCore::Event *,WebCore::EventTargetData *,WTF::Vector<WebCore::RegisteredEventListener,1> &)
0x01d3cfa7	 [chrome.dll	 - eventtarget.cpp:276]	WebCore::EventTarget::fireEventListeners(WebCore::Event *)
0x01c91297	 [chrome.dll	 - node.cpp:2504]	WebCore::Node::handleLocalEvents(WebCore::Event *)
0x01c915b1	 [chrome.dll	 - node.cpp:2647]	WebCore::Node::dispatchGenericEvent(WTF::PassRefPtr<WebCore::Event>)
0x01c913b4	 [chrome.dll	 - node.cpp:2567]	WebCore::Node::dispatchEvent(WTF::PassRefPtr<WebCore::Event>)
0x01c91c2f	 [chrome.dll	 - node.cpp:2859]	WebCore::Node::dispatchMouseEvent(WebCore::AtomicString const &,int,int,int,int,int,int,bool,bool,bool,bool,bool,WebCore::Node *,WTF::PassRefPtr<WebCore::Event>)
0x01c918ef	 [chrome.dll	 - node.cpp:2768]	WebCore::Node::dispatchMouseEvent(WebCore::PlatformMouseEvent const &,WebCore::AtomicString const &,int,WebCore::Node *)
0x01c5b1ff	 [chrome.dll	 - atomicstring.cpp:241]	WebCore::AtomicString::lower()
0x01c5b1ff	 [chrome.dll	 - atomicstring.cpp:241]	WebCore::AtomicString::lower()
0x01cc7f62	 [chrome.dll	 - eventhandler.cpp:1782]	WebCore::EventHandler::dispatchMouseEvent(WebCore::AtomicString const &,WebCore::Node *,bool,int,WebCore::PlatformMouseEvent const &,bool)
0x01cc7803	 [chrome.dll	 - eventhandler.cpp:1511]	WebCore::EventHandler::handleMouseReleaseEvent(WebCore::PlatformMouseEvent const &)
Comment 2 anton muhin 2010-04-19 07:55:15 PDT
Created attachment 53679 [details]
Patch
Comment 3 anton muhin 2010-04-19 07:57:06 PDT
Those crashes are relatively rare, but given https://bugs.webkit.org/show_bug.cgi?id=37661 I am starting to wonder if we could tolerate empty handles in v8?
Comment 4 Nate Chapin 2010-04-19 12:41:32 PDT
This change looks fine, but should we add a layout test for this?
Comment 5 Adam Barth 2010-04-19 14:13:48 PDT
Comment on attachment 53679 [details]
Patch

This is better than crashing, but do we understand how we end up with a null handle here? I'm wondering whether not invoking the handler is the right behavior.
Comment 6 anton muhin 2010-04-20 07:45:01 PDT
(In reply to comment #5)
> (From update of attachment 53679 [details])
> This is better than crashing, but do we understand how we end up with a null
> handle here? I'm wondering whether not invoking the handler is the right
> behavior.

Alas, I don't have an easy repro case for that and thus cannot add layout test, nor don't know for sure why we get null handle here.

Regarding if not invoking a handler is a right approach.  Quite agree, but once again that seems the best solution so far.

If you like, as an alternative we could try to do the following: this or similar bug is reproducible on bots.  So I could temporary patch the code to try to gather what goes on.

Thoughts?

Not submitting for now.
Comment 7 Adam Barth 2010-04-20 12:19:19 PDT
Maybe an assert so we can catch this in debug builds?  That also documents that were not sold on null handles being correct here.
Comment 8 anton muhin 2010-04-21 05:41:58 PDT
Created attachment 53945 [details]
Add an assert
Comment 9 anton muhin 2010-04-21 05:44:54 PDT
(In reply to comment #7)
> Maybe an assert so we can catch this in debug builds?  That also documents that
> were not sold on null handles being correct here.

I've attempted to add an assert.

My concerns:

1) if my analysis is correct, we'll just die in debug builds (but benefit is we could verify the hypothesis)

2) beyond verifying the hypothesis, it won't allow us to understand why it empty here.

If we want to understand what goes here, I'd inline toV8 method here and see why we take a branch returning empty handle.
Comment 10 Adam Barth 2010-04-21 08:18:28 PDT
Comment on attachment 53945 [details]
Add an assert

Ok.  The main goal is to figure out why we're getting a null handle if you don't think the assert is helpful, feel free to land the earlier version.  (Also, your change log is out of date.)
Comment 11 anton muhin 2010-04-21 10:53:27 PDT
Created attachment 53970 [details]
Next iteration
Comment 12 anton muhin 2010-04-21 10:55:25 PDT
(In reply to comment #10)
> (From update of attachment 53945 [details])
> Ok.  The main goal is to figure out why we're getting a null handle if you
> don't think the assert is helpful, feel free to land the earlier version. 
> (Also, your change log is out of date.)

Adam, no, I think asserts (almost) never hurt :)

I've added another one---if it triggers, we'll know for sure why we get empty handle here.

And, sorry, what's wrong with change log?  No reference to handleEvent method?  If yes, I've fixed that.
Comment 13 Adam Barth 2010-04-21 11:56:21 PDT
Comment on attachment 53970 [details]
Next iteration

Great.  Thanks for iterating on this patch.  Yeah, the change log was missing the line for the newer function you modified.
Comment 14 anton muhin 2010-04-21 11:57:34 PDT
Comment on attachment 53970 [details]
Next iteration

Thanks a lot for review, Adam, cq+'ing
Comment 15 WebKit Commit Bot 2010-04-22 03:41:14 PDT
Comment on attachment 53970 [details]
Next iteration

Clearing flags on attachment: 53970

Committed r58084: <http://trac.webkit.org/changeset/58084>
Comment 16 WebKit Commit Bot 2010-04-22 03:41:20 PDT
All reviewed patches have been landed.  Closing bug.