Bug 57382 - [chromium] UnlockNonLocked condition reached in WorkerFileSystemsCallbackBridge::mayPostTaskToWorker
Summary: [chromium] UnlockNonLocked condition reached in WorkerFileSystemsCallbackBrid...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-29 13:29 PDT by cbentzel
Modified: 2011-03-30 20:25 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.23 KB, patch)
2011-03-29 23:16 PDT, David Levin
no flags Details | Formatted Diff | Diff
Patch (7.33 KB, patch)
2011-03-30 16:18 PDT, David Levin
no flags Details | Formatted Diff | Diff
Patch (7.21 KB, patch)
2011-03-30 17:02 PDT, David Levin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description cbentzel 2011-03-29 13:29:12 PDT
Chromium Linux ThreadSanitizer bots started triggering this during a WebKit roll from 81970 to 82211.

I've added a valgrind suppression for it, but the core issue should be fixed. 

See also http://crbug.com/77792
Comment 1 Kinuko Yasuda 2011-03-29 18:38:18 PDT
Hmm, not sure how I could unlock an non-locked lock using a scoped locker?  I'll look into it...
Comment 2 Adam Barth 2011-03-29 18:52:04 PDT
Memory corruption?
Comment 3 David Levin 2011-03-29 23:16:56 PDT
Created attachment 87468 [details]
Patch
Comment 4 David Levin 2011-03-29 23:18:29 PDT
This got introduced recently when a race was being fixed for a ref counted object but in turn the ref counting basically got moved inside of the mutex lock which led to this issue.
Comment 5 Adam Barth 2011-03-29 23:25:48 PDT
Comment on attachment 87468 [details]
Patch

This patch is blowing my mind.
Comment 6 Kinuko Yasuda 2011-03-29 23:32:24 PDT
I have re-read the comment and it makes sense to me.  (At least now I'm sure that the previous code could cause a memory corruption.)   Thanks for looking into this one.
Comment 7 Dmitry Titov 2011-03-30 00:25:48 PDT
Comment on attachment 87468 [details]
Patch

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

> Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:366
> +        // Take the lock to ensure that it has been released on the other thread.

Does it actually mean "Take the lock to wait until the other thread releases it"?
If so, what ensures that the other thread grabbed the lock by that time?

I'm not sure I really understand what you are doing here though :-)
Comment 8 David Levin 2011-03-30 11:53:17 PDT
Comment on attachment 87468 [details]
Patch

Ok, this was just a quick fix and no one understands it.

I'm looking at this class and trying to solve this issue and the one that caused it. I think I can simplify a few things in it.
Comment 9 David Levin 2011-03-30 16:18:48 PDT
Created attachment 87636 [details]
Patch
Comment 10 Kinuko Yasuda 2011-03-30 16:53:55 PDT
Comment on attachment 87636 [details]
Patch

This change looks good to me now.  I wanted to get rid of m_selfRef but wasn't able to think of a solution.
Thanks very much!

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

> Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:345
> +    if (!bridge->m_worker) // Since this check is just an optimization, it doesn't matter that the usage isn't threadsafe.

I think we can just remove these lines?  (I'm afraid tsan will warn on this check.)
Comment 11 David Levin 2011-03-30 17:02:31 PDT
Created attachment 87638 [details]
Patch
Comment 12 Dmitry Titov 2011-03-30 17:09:19 PDT
Comment on attachment 87638 [details]
Patch

r=me
Comment 13 WebKit Commit Bot 2011-03-30 20:25:20 PDT
Comment on attachment 87638 [details]
Patch

Clearing flags on attachment: 87638

Committed r82540: <http://trac.webkit.org/changeset/82540>
Comment 14 WebKit Commit Bot 2011-03-30 20:25:26 PDT
All reviewed patches have been landed.  Closing bug.