Bug 56138 - Data race between ~WorkerFileSystemCallbacksBridge and runTasksOnWorkerThread
Summary: Data race between ~WorkerFileSystemCallbacksBridge and runTasksOnWorkerThread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-10 13:08 PST by Adam Klein
Modified: 2011-03-25 16:01 PDT (History)
2 users (show)

See Also:


Attachments
Patch (2.23 KB, patch)
2011-03-10 13:10 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (4.08 KB, patch)
2011-03-25 00:10 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (4.36 KB, patch)
2011-03-25 15:23 PDT, Kinuko Yasuda
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>