[v8] Do not pass empty handle into SetHiddenPrototype which would crash event
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 &)
Created attachment 53679 [details] Patch
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?
This change looks fine, but should we add a layout test for this?
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.
(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.
Maybe an assert so we can catch this in debug builds? That also documents that were not sold on null handles being correct here.
Created attachment 53945 [details] Add an assert
(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 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.)
Created attachment 53970 [details] Next iteration
(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 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 on attachment 53970 [details] Next iteration Thanks a lot for review, Adam, cq+'ing
Comment on attachment 53970 [details] Next iteration Clearing flags on attachment: 53970 Committed r58084: <http://trac.webkit.org/changeset/58084>
All reviewed patches have been landed. Closing bug.