There is a crash in Chromium(http://crbug.com/31714) related to ScriptState retrieval logic. ScriptController::currentScriptState assumes that we can always get ScriptState for the current context. However, on page navigation, when the window object has already been cleared, some asynchronous code from the previous page may invoke console.log and despite its context is still valid it will fail on V8Proxy::retrieveFrame. One possible solution would be to remove Frame* pointer from ScriptState and cache ScriptState instance directly on the v8::Context(currently it's cached on ScriptController and V8IsolatedWorld but v8::Context lifetime matches better ScriptState lifetime). ---------------------------- * Crash Trace * ---------------------------- [OwnPtr.h:68] - WebCore::ScriptController::mainWorldScriptState() [ScriptController.cpp:381] - WebCore::ScriptController::currentScriptState() [ScriptCallStack.cpp:58] - WebCore::ScriptCallStack::create(v8::Arguments const&, unsigned int) [V8Console.cpp:75] - logCallback [builtins.cc:386] - Builtin_HandleApiCall [execution.cc:97] - Invoke [execution.cc:124] - v8::internal::Execution::TryCall(v8::internal::Handle, v8::internal::Handle, int, v8::internal::Object***, bool*) [api.cc:2426] - v8::Function::Call(v8::Handle, int, v8::Handle*) [V8Proxy.cpp:510] - WebCore::V8Proxy::callFunction(v8::Handle, v8::Handle, int, v8::Handle*) [V8CustomEventListener.cpp:75] - WebCore::V8EventListener::callListenerFunction(WebCore::ScriptExecutionContext*, v8::Handle, WebCore::Event*) [V8AbstractEventListener.cpp:145] - WebCore::V8AbstractEventListener::invokeEventHandler(WebCore::ScriptExecutionContext*, WebCore::Event*, v8::Handle) [V8AbstractEventListener.cpp:89] - WebCore::V8AbstractEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) [EventTarget.cpp:297] - WebCore::EventTarget::fireEventListeners(WebCore::Event*) [EventTarget.cpp:262] - WebCore::EventTarget::dispatchEvent(WTF::PassRefPtr) [XMLHttpRequest.cpp:276] - WebCore::XMLHttpRequest::callReadyStateChangeListener() [XMLHttpRequest.cpp:245] - WebCore::XMLHttpRequest::didFinishLoading(unsigned long) [SubresourceLoader.cpp:184] - WebCore::SubresourceLoader::didFinishLoading() [weburlloader_impl.cc:532] - webkit_glue::WebURLLoaderImpl::Context::OnCompletedRequest(URLRequestStatus const&, std::string const&) [resource_dispatcher.cc:448] - ResourceDispatcher::OnRequestComplete(int, URLRequestStatus const&, std::string const&) [tuple.h:435] - ResourceDispatcher::DispatchMessage(IPC::Message const&) [resource_dispatcher.cc:294] - ResourceDispatcher::OnMessageReceived(IPC::Message const&) [child_thread.cc:109] - ChildThread::OnMessageReceived(IPC::Message const&) [tuple.h:422] - RunnableMethod >::Run() [message_loop.cc:320] - MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) [message_loop.cc:435] - MessageLoop::DoWork() [message_pump_mac.mm:291] - base::MessagePumpCFRunLoopBase::RunWorkSource(void*) [CoreFoundation+0x0003e8ca] - __CFRunLoopDoSources0 [CoreFoundation+0x0003c38e] - __CFRunLoopRun [CoreFoundation+0x0003b863] - CFRunLoopRunSpecific [CoreFoundation+0x0003b690] - CFRunLoopRunInMode [HIToolbox+0x00034f0b] - RunCurrentEventLoopInMode [HIToolbox+0x00034cc2] - ReceiveNextEventCommon [HIToolbox+0x00034b47] - BlockUntilNextEventMatchingListInMode [AppKit+0x00048ac4] - _DPSNextEvent [AppKit+0x00048305] - -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] [AppKit+0x0000a49e] - -[NSApplication run] [message_pump_mac.mm:677] - base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*) [message_pump_mac.mm:213] - base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*) [message_loop.cc:205] - MessageLoop::Run() [renderer_main.cc:277] - RendererMain(MainFunctionParams const&) [chrome_dll_main.cc:668] - ChromeMain [Google Chrome Helper+0x00000fc5] - null
Created attachment 47621 [details] patch
Attachment 47621 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/bindings/v8/ScriptState.cpp:63: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 47621 [details] patch This looks great. A few comments: 1) Please use V8HiddenPropertyNames for the ScriptState hidden property name. 2) Can we make the ScriptState destructor private to ensure that it's only deleted via the weak callback? 3) Have you tested performance? Hidden properties are relatively slow, so we need to make sure this isn't used in any performance critical places. Also, please fix the style nit.
Created attachment 47641 [details] patch addressing reviewers comments
(In reply to comment #3) > (From update of attachment 47621 [details]) > This looks great. A few comments: > > 1) Please use V8HiddenPropertyNames for the ScriptState hidden property name. > Done. > 2) Can we make the ScriptState destructor private to ensure that it's only > deleted via the weak callback? > Good point. I've added FIXME comment since there is one call to the destructor on the Chromium side and we need to get rid of it first. > 3) Have you tested performance? Hidden properties are relatively slow, so we > need to make sure this isn't used in any performance critical places. > Non-empty ScriptState is used only for writing console messages and for developer tools so the code is not performance critical. > Also, please fix the style nit. Done.
Comment on attachment 47641 [details] patch addressing reviewers comments > > 2) Can we make the ScriptState destructor private to ensure that it's only > > deleted via the weak callback? > > > Good point. I've added FIXME comment since there is one call to the destructor > on the Chromium side and we need to get rid of it first. Is this call going to cause a double free? Please ensure that it's ok before landing. Thanks!
(In reply to comment #6) > (From update of attachment 47641 [details]) > > > 2) Can we make the ScriptState destructor private to ensure that it's only > > > deleted via the weak callback? > > > > > Good point. I've added FIXME comment since there is one call to the destructor > > on the Chromium side and we need to get rid of it first. > > Is this call going to cause a double free? Please ensure that it's ok before > landing. > > Thanks! You're right, it would cause double disposal. It's easy to fix by removing m_context.MakeWeak from the obsolete constructor.
Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/bindings/js/ScriptController.cpp M WebCore/bindings/js/ScriptController.h M WebCore/bindings/js/ScriptState.cpp M WebCore/bindings/js/ScriptState.h M WebCore/bindings/v8/ScriptCallStack.cpp M WebCore/bindings/v8/ScriptController.cpp M WebCore/bindings/v8/ScriptController.h M WebCore/bindings/v8/ScriptScope.cpp M WebCore/bindings/v8/ScriptScope.h M WebCore/bindings/v8/ScriptState.cpp M WebCore/bindings/v8/ScriptState.h M WebCore/bindings/v8/V8HiddenPropertyName.h M WebCore/bindings/v8/V8IsolatedContext.cpp M WebCore/bindings/v8/V8IsolatedContext.h M WebCore/bindings/v8/V8Utilities.cpp M WebCore/bindings/v8/custom/V8NodeIteratorCustom.cpp M WebCore/bindings/v8/custom/V8TreeWalkerCustom.cpp M WebCore/inspector/InspectorController.cpp Committed r54012
Created attachment 47695 [details] patch This patch can be landed once Chromium is updated to use the new ScriptState methods.
Comment on attachment 47695 [details] patch Needs chromium to be ready.
Comment on attachment 47695 [details] patch Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/bindings/v8/ScriptState.cpp M WebCore/bindings/v8/ScriptState.h Committed r54108