Bug 200817 – Clarify StorageManagerSet / StorageManager threading model after r248734

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Clarify StorageManagerSet / StorageManager threading model after r248734. StorageManager is now a background thread object but it still calls its completion handlers on the main thread, which is very error prone.

[Chris Dumez]

Created attachment 376503 [details]
Patch

[Chris Dumez]

Created attachment 376505 [details]
Patch

[Sihui Liu]

Comment on attachment 376505 [details]
Patch

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

> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp:188
>  void StorageManagerSet::getSessionStorageOrigins(PAL::SessionID sessionID, CompletionHandler<void(HashSet<WebCore::SecurityOriginData>&&)>&& completionHandler)

Probably not very related: CompletionHandler<void(HashSet<WebCore::SecurityOriginData>&&)>&& here can be changed to GetOriginsCallback&&.

> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp:203
>  void StorageManagerSet::deleteSessionStorage(PAL::SessionID sessionID, CompletionHandler<void()>&& completionHandler)

CompletionHandler<void()>&& => DeleteCallback&&

> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp:270
>  void StorageManagerSet::getLocalStorageOriginDetails(PAL::SessionID sessionID, CompletionHandler<void(Vector<LocalStorageDatabaseTracker::OriginDetails>&&)>&& completionHandler)

CompletionHandler<void(Vector<LocalStorageDatabaseTracker::OriginDetails>&&)>&& => GetOriginDetailsCallback&&

[Chris Dumez]

Created attachment 376508 [details]
Patch

[Geoffrey Garen]

Comment on attachment 376508 [details]
Patch

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

r=me

> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp:279
> +        auto originDetails = storageManager->getLocalStorageOriginDetails();
> +        RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler), originDetails = crossThreadCopy(originDetails)]() mutable {

Kind of unfortunate that this idiom creates a throw-away copy followed by a cross-thread copy.

Maybe we should consider renaming getLocalStorageOriginDetails() -> getLocalStorageOriginDetailsCrossThreadCopy(), and change the implementation to produce a crossThreadCopy.

[Chris Dumez]

Comment on attachment 376508 [details]
Patch

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

>> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp:279
>> +        RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler), originDetails = crossThreadCopy(originDetails)]() mutable {
> 
> Kind of unfortunate that this idiom creates a throw-away copy followed by a cross-thread copy.
> 
> Maybe we should consider renaming getLocalStorageOriginDetails() -> getLocalStorageOriginDetailsCrossThreadCopy(), and change the implementation to produce a crossThreadCopy.

Ok, I can do that.

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 376508 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376508&action=review
> 
> >> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp:279
> >> +        RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler), originDetails = crossThreadCopy(originDetails)]() mutable {
> > 
> > Kind of unfortunate that this idiom creates a throw-away copy followed by a cross-thread copy.
> > 
> > Maybe we should consider renaming getLocalStorageOriginDetails() -> getLocalStorageOriginDetailsCrossThreadCopy(), and change the implementation to produce a crossThreadCopy.
> 
> Ok, I can do that.

I would just like to point out that the previous code was already creating a throw-away copy since LocalStorageDatabaseTracker::originDetails() returns a new Vector. I merely moved the place where the Vector was crossThreadCopied.

[Chris Dumez]

Created attachment 376510 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #8)
> Created attachment 376510 [details]
> Patch

This latest iteration should have no perf impact as it always cross-thread-copies at the source.

[Geoffrey Garen]

Comment on attachment 376510 [details]
Patch

r=me

[Geoffrey Garen]

> I would just like to point out that the previous code was already creating a
> throw-away copy since LocalStorageDatabaseTracker::originDetails() returns a
> new Vector. I merely moved the place where the Vector was crossThreadCopied.

True! :P

I guess I only noticed this because the new code is clearer.

[Chris Dumez]

(In reply to Geoffrey Garen from comment #11)
> > I would just like to point out that the previous code was already creating a
> > throw-away copy since LocalStorageDatabaseTracker::originDetails() returns a
> > new Vector. I merely moved the place where the Vector was crossThreadCopied.
> 
> True! :P
> 
> I guess I only noticed this because the new code is clearer.

Your point still stands for the other 2 calls sites. You just happened to comment on the only one when there is no perf-hit :P

[WebKit Commit Bot]

Comment on attachment 376510 [details]
Patch

Clearing flags on attachment: 376510

Committed r248779: <https://trac.webkit.org/changeset/248779>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54409115>