Bug 34266 – [v8] Crash - WebCore::ScriptController::mainWorldScriptState()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Yury Semikhatsky]

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

[Yury Semikhatsky]

Created attachment 47621 [details]
patch

[WebKit Review Bot]

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.

[Adam Barth]

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.

[Yury Semikhatsky]

Created attachment 47641 [details]
patch addressing reviewers comments

[Yury Semikhatsky]

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

[Adam Barth]

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!

[Yury Semikhatsky]

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

[Yury Semikhatsky]

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

[Yury Semikhatsky]

Created attachment 47695 [details]
patch

This patch can be landed once Chromium is updated to use the new ScriptState methods.

[Pavel Feldman]

Comment on attachment 47695 [details]
patch

Needs chromium to be ready.

[Yury Semikhatsky]

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