[v8] Do not pass empty handle into SetHiddenValue which would crash

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[anton muhin]

[v8] Do not pass empty handle into SetHiddenPrototype which would crash event

[anton muhin]

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 &)

[anton muhin]

Created attachment 53679 [details]
Patch

[anton muhin]

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?

[Nate Chapin]

This change looks fine, but should we add a layout test for this?

[Adam Barth]

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.

[anton muhin]

(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.

[Adam Barth]

Maybe an assert so we can catch this in debug builds?  That also documents that were not sold on null handles being correct here.

[anton muhin]

Created attachment 53945 [details]
Add an assert

[anton muhin]

(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.

[Adam Barth]

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.)

[anton muhin]

Created attachment 53970 [details]
Next iteration

[anton muhin]

(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.

[Adam Barth]

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.

[anton muhin]

Comment on attachment 53970 [details]
Next iteration

Thanks a lot for review, Adam, cq+'ing

[WebKit Commit Bot]

Comment on attachment 53970 [details]
Next iteration

Clearing flags on attachment: 53970

Committed r58084: <http://trac.webkit.org/changeset/58084>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.