Bug 56138 – Data race between ~WorkerFileSystemCallbacksBridge and runTasksOnWorkerThread

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Klein]

Retain a reference to the WorkerFileSystemCallbacksBridge in runTaskOnWorkerThread to avoid a race condition

[Adam Klein]

Created attachment 85382 [details]
Patch

[David Levin]

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.

[Adam Klein]

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

[David Levin]

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

[Adam Klein]

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.

[Kinuko Yasuda]

Created attachment 86901 [details]
Patch

[David Levin]

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.

[Kinuko Yasuda]

Created attachment 86987 [details]
Patch

[Kinuko Yasuda]

Committed r81999: <http://trac.webkit.org/changeset/81999>