Bug 34266 - [v8] Crash - WebCore::ScriptController::mainWorldScriptState()
Summary: [v8] Crash - WebCore::ScriptController::mainWorldScriptState()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-28 08:08 PST by Yury Semikhatsky
Modified: 2010-01-30 23:40 PST (History)
2 users (show)

See Also:


Attachments
patch (17.18 KB, patch)
2010-01-28 08:35 PST, Yury Semikhatsky
abarth: review-
Details | Formatted Diff | Diff
patch addressing reviewers comments (23.84 KB, patch)
2010-01-28 12:00 PST, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
patch (1.95 KB, patch)
2010-01-29 03:22 PST, Yury Semikhatsky
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2010-01-28 08:08:08 PST
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
Comment 1 Yury Semikhatsky 2010-01-28 08:35:07 PST
Created attachment 47621 [details]
patch
Comment 2 WebKit Review Bot 2010-01-28 08:35:47 PST
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 3 Adam Barth 2010-01-28 08:43:41 PST
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.
Comment 4 Yury Semikhatsky 2010-01-28 12:00:02 PST
Created attachment 47641 [details]
patch addressing reviewers comments
Comment 5 Yury Semikhatsky 2010-01-28 12:02:43 PST
(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 6 Adam Barth 2010-01-28 12:22:25 PST
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!
Comment 7 Yury Semikhatsky 2010-01-28 12:33:01 PST
(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.
Comment 8 Yury Semikhatsky 2010-01-28 13:06:22 PST
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
Comment 9 Yury Semikhatsky 2010-01-29 03:22:58 PST
Created attachment 47695 [details]
patch

This patch can be landed once Chromium is updated to use the new ScriptState methods.
Comment 10 Pavel Feldman 2010-01-29 07:13:51 PST
Comment on attachment 47695 [details]
patch

Needs chromium to be ready.
Comment 11 Yury Semikhatsky 2010-01-30 23:40:09 PST
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