Bug 201074 – CacheStorageConnection::computeRealBodySize is not thread-safe

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[youenn fablet]

CacheStorageConnection::computeRealBodySize is not thread-safe

[youenn fablet]

Created attachment 377122 [details]
Patch

[Darin Adler]

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.

[youenn fablet]

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

[youenn fablet]

Created attachment 377236 [details]
Patch

[Chris Dumez]

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.

[youenn fablet]

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

[WebKit Commit Bot]

Comment on attachment 377236 [details]
Patch

Clearing flags on attachment: 377236

Committed r249105: <https://trac.webkit.org/changeset/249105>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54713221>