Bug 57382 – [chromium] UnlockNonLocked condition reached in WorkerFileSystemsCallbackBridge::mayPostTaskToWorker

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

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

[Kinuko Yasuda]

Hmm, not sure how I could unlock an non-locked lock using a scoped locker?  I'll look into it...

[Adam Barth]

Memory corruption?

[David Levin]

Created attachment 87468 [details]
Patch

[David Levin]

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.

[Adam Barth]

Comment on attachment 87468 [details]
Patch

This patch is blowing my mind.

[Kinuko Yasuda]

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.

[Dmitry Titov]

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

[David Levin]

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.

[David Levin]

Created attachment 87636 [details]
Patch

[Kinuko Yasuda]

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

[David Levin]

Created attachment 87638 [details]
Patch

[Dmitry Titov]

Comment on attachment 87638 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 87638 [details]
Patch

Clearing flags on attachment: 87638

Committed r82540: <http://trac.webkit.org/changeset/82540>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.