CacheStorageConnection::computeRealBodySize is not thread-safe
Created attachment 377122 [details] Patch
Comment on attachment 377122 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377122&action=review > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:48 > + uint64_t resultSize; > + BinarySemaphore semaphore; > + callOnMainThread([sessionID, formData = formData.isolatedCopy(), &semaphore, &resultSize] { > + resultSize = formData->lengthInBytes(sessionID); > + semaphore.signal(); > + }); > + semaphore.wait(); Why is the semaphore needed? Doesn’t callOnMainThread, which is synchronous, already contain enough synchronization to guarantee the correct value for resultSize? If not, it seems really hard to program with callOnMainThread correctly.
(In reply to Darin Adler from comment #2) > Comment on attachment 377122 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=377122&action=review > > > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:48 > > + uint64_t resultSize; > > + BinarySemaphore semaphore; > > + callOnMainThread([sessionID, formData = formData.isolatedCopy(), &semaphore, &resultSize] { > > + resultSize = formData->lengthInBytes(sessionID); > > + semaphore.signal(); > > + }); > > + semaphore.wait(); > > Why is the semaphore needed? Doesn’t callOnMainThread, which is synchronous, > already contain enough synchronization to guarantee the correct value for > resultSize? If not, it seems really hard to program with callOnMainThread > correctly. Using callOnMainThread does not wait, but I should replace the semaphore with a call to callOnMainThreadAndWait.
Created attachment 377236 [details] Patch
Comment on attachment 377236 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377236&action=review > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:38 > + if (isMainThread()) This branch is not really needed, callOnMainThreadAndWait() already does the right thing for you. Or are you doing this as an optimization to avoid the isolatedCopy() of the FormData? I guess that's fine.
(In reply to Chris Dumez from comment #5) > Comment on attachment 377236 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=377236&action=review > > > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:38 > > + if (isMainThread()) > > This branch is not really needed, callOnMainThreadAndWait() already does the > right thing for you. Or are you doing this as an optimization to avoid the > isolatedCopy() of the FormData? I guess that's fine. Yes, there is no way we could optimise this.
Comment on attachment 377236 [details] Patch Clearing flags on attachment: 377236 Committed r249105: <https://trac.webkit.org/changeset/249105>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54713221>