Bug 56138

Summary: Data race between ~WorkerFileSystemCallbacksBridge and runTasksOnWorkerThread
Product: WebKit Reporter: Adam Klein <adamk>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: kinuko, levin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch levin: review+

Description Adam Klein 2011-03-10 13:08:21 PST
Retain a reference to the WorkerFileSystemCallbacksBridge in runTaskOnWorkerThread to avoid a race condition
Comment 1 Adam Klein 2011-03-10 13:10:08 PST
Created attachment 85382 [details]
Patch
Comment 2 David Levin 2011-03-10 13:20:41 PST
Comment on attachment 85382 [details]
Patch

I don't understand how this changes a thing.  The previous passed in variable bridge would have help the ref count.

Also prpBridge looks like hungarian naming which is simply not used in WebKit.
Comment 3 Adam Klein 2011-03-10 13:22:41 PST
Working with Kinuko offline to get to the root of the issue, you're right that this doesn't solve the problem.

As for the naming, though, this style is recommended by http://www.webkit.org/coding/RefPtr.html and used frequently in WebKit (as a quick codesearch shows).
Comment 4 David Levin 2011-03-10 13:24:22 PST
(In reply to comment #3)
> As for the naming, though, this style is recommended by http://www.webkit.org/coding/RefPtr.html and used frequently in WebKit (as a quick codesearch shows).

Good point!
Comment 5 Adam Klein 2011-03-10 13:33:20 PST
Retitling to better capture the actual problem. Here's the TSAN trace:

WARNING: Possible data race during read of size 4 at 0xB16D3C4: {{{
   T0 (L{}):
    #0  WebCore::WorkerContext::Observer::~Observer() third_party/WebKit/Source/WebCore/workers/WorkerContext.cpp:393
    #1  WebKit::WorkerFileSystemCallbacksBridge::~WorkerFileSystemCallbacksBridge() third_party/WebKit/Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:311
    #2  WTF::ThreadSafeShared<WebKit::WorkerFileSystemCallbacksBridge>::deref() third_party/WebKit/Source/JavaScriptCore/wtf/ThreadSafeShared.h:135
    #3  WebKit::MainThreadFileSystemCallbacks::~MainThreadFileSystemCallbacks() third_party/WebKit/Source/JavaScriptCore/wtf/PassRefPtr.h:59
    #4  WebKit::MainThreadFileSystemCallbacks::didReadMetadata(WebKit::WebFileInfo const&) third_party/WebKit/Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:119
    #5  WebFileSystemCallbackDispatcher::DidReadMetadata(base::PlatformFileInfo const&) content/common/file_system/webfilesystem_callback_dispatcher.cc:44
    #6  FileSystemDispatcher::OnDidReadMetadata(int, base::PlatformFileInfo const&) content/common/file_system/file_system_dispatcher.cc:242
    ...
  Concurrent write(s) happened at (OR AFTER) these points:
   T2 (L{}):
    #0  WebCore::WorkerContext::Observer::stopObserving() third_party/WebKit/Source/WebCore/workers/WorkerContext.cpp:405
    #1  WebKit::WorkerFileSystemCallbacksBridge::runTaskOnWorkerThread(WebCore::ScriptExecutionContext*, WTF::PassRefPtr<WebKit::WorkerFileSystemCallbacksBridge>, WTF::PassOwnPtr<WebCore::ScriptExecutionContext::Task>) third_party/WebKit/Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:368
    #2  WebCore::CrossThreadTask2<WTF::PassRefPtr<WebKit::WorkerFileSystemCallbacksBridge>, WTF::PassRefPtr<WebKit::WorkerFileSystemCallbacksBridge>, WTF::PassOwnPtr<WebCore::ScriptExecutionContext::Task>, WTF::PassOwnPtr<WebCore::ScriptExecutionContext::Task> >::performTask(WebCore::ScriptExecutionContext*) third_party/WebKit/Source/WebCore/dom/CrossThreadTask.h:112
    #3  WebCore::WorkerRunLoop::Task::performTask(WebCore::ScriptExecutionContext*) third_party/WebKit/Source/WebCore/workers/WorkerRunLoop.cpp:199
    #4  WebCore::WorkerRunLoop::runInMode(WebCore::WorkerContext*, WebCore::ModePredicate const&) third_party/WebKit/Source/WebCore/workers/WorkerRunLoop.cpp:163
    #5  WebCore::WorkerRunLoop::run(WebCore::WorkerContext*) third_party/WebKit/Source/WebCore/workers/WorkerRunLoop.cpp:134
    #6  WebCore::WorkerThread::runEventLoop() third_party/WebKit/Source/WebCore/workers/WorkerThread.cpp:163
    #7  WebCore::DedicatedWorkerThread::runEventLoop() third_party/WebKit/Source/WebCore/workers/DedicatedWorkerThread.cpp:66
    #8  WebCore::WorkerThread::workerThread() third_party/WebKit/Source/WebCore/workers/WorkerThread.cpp:141
    #9  WebCore::WorkerThread::workerThreadStart(void*) third_party/WebKit/Source/WebCore/workers/WorkerThread.cpp:118
  Location 0xB16D3C4 is 4 bytes inside a block starting at 0xB16D3C0 of size 52 allocated by T2 from heap:
    #0  malloc /tmp/valgrind-src/tsan/tsan/ts_valgrind_intercepts.c:410
    #1  WTF::fastMalloc(unsigned int) third_party/WebKit/Source/JavaScriptCore/wtf/FastMalloc.cpp:250
    #2  WTF::ThreadSafeSharedBase::operator new(unsigned int) third_party/WebKit/Source/JavaScriptCore/wtf/ThreadSafeShared.h:70
    #3  WebKit::WorkerFileSystemCallbacksBridge::create(WebKit::WebWorkerBase*, WebCore::ScriptExecutionContext*, WebKit::WebFileSystemCallbacks*) third_party/WebKit/Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:83
    #4  WebCore::WorkerAsyncFileSystemChromium::createWorkerFileSystemCallbacksBridge(WTF::PassOwnPtr<WebCore::AsyncFileSystemCallbacks>) third_party/WebKit/Source/WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:219
    #5  WebCore::WorkerAsyncFileSystemChromium::readMetadata(WTF::String const&, WTF::PassOwnPtr<WebCore::AsyncFileSystemCallbacks>) third_party/WebKit/Source/WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:112
    #6  WebCore::DOMFileSystemBase::getMetadata(WebCore::EntryBase const*, WTF::PassRefPtr<WebCore::MetadataCallback>, WTF::PassRefPtr<WebCore::ErrorCallback>) third_party/WebKit/Source/WebCore/fileapi/DOMFileSystemBase.cpp:104
    #7  WebCore::Entry::getMetadata(WTF::PassRefPtr<WebCore::MetadataCallback>, WTF::PassRefPtr<WebCore::ErrorCallback>) third_party/WebKit/Source/WebCore/fileapi/Entry.cpp:57
    #8  WebCore::EntryInternal::getMetadataCallback(v8::Arguments const&) out/Release/obj/gen/webcore/bindings/V8Entry.cpp:107
    #9  v8::internal::MaybeObject* v8::internal::HandleApiCallHelper<false>(v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>) v8/src/builtins.cc:1069
   Race verifier data: 0x9AB7AF9,0x9AB763B
}}}

See http://crbug.com/75589 for details.
Comment 6 Kinuko Yasuda 2011-03-25 00:10:55 PDT
Created attachment 86901 [details]
Patch
Comment 7 David Levin 2011-03-25 13:05:48 PDT
Comment on attachment 86901 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86901&action=review

Just a few things but they may change the code a bit so I'd like to see it one more time.

> Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:89
>      static MainThreadFileSystemCallbacks* createLeakedPtr(PassRefPtr<WorkerFileSystemCallbacksBridge> bridge, const String& mode)

This should no longer be a PassRefPtr (which implies that it keeps the ref count in some way and that is clearly not the case anymore).

> Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:385
>      { // Let go of the mutex before possibly deleting this due to m_selfRef.clear().

This comment and the brace are now out of date.
Comment 8 Kinuko Yasuda 2011-03-25 15:23:34 PDT
Created attachment 86987 [details]
Patch
Comment 9 Kinuko Yasuda 2011-03-25 16:01:10 PDT
Committed r81999: <http://trac.webkit.org/changeset/81999>