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.
Created attachment 376503 [details] Patch
Created attachment 376505 [details] Patch
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&&
Created attachment 376508 [details] Patch
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.
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.
(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.
Created attachment 376510 [details] Patch
(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.
Comment on attachment 376510 [details] Patch r=me
> 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.
(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
Comment on attachment 376510 [details] Patch Clearing flags on attachment: 376510 Committed r248779: <https://trac.webkit.org/changeset/248779>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54409115>