Bug 200373 - Some improvements on web storage
Summary: Some improvements on web storage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on: 200817
Blocks:
  Show dependency treegraph
 
Reported: 2019-08-01 18:54 PDT by Sihui Liu
Modified: 2019-08-16 10:38 PDT (History)
10 users (show)

See Also:


Attachments
Patch (206.73 KB, patch)
2019-08-01 19:32 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (185.97 KB, patch)
2019-08-02 13:20 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (210.10 KB, patch)
2019-08-02 13:39 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (222.35 KB, patch)
2019-08-05 14:48 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (247.38 KB, patch)
2019-08-07 14:53 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (247.42 KB, patch)
2019-08-14 15:15 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (248.06 KB, patch)
2019-08-14 17:12 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (247.74 KB, patch)
2019-08-15 07:55 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (247.91 KB, patch)
2019-08-15 09:05 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (246.53 KB, patch)
2019-08-15 11:03 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sihui Liu 2019-08-01 18:54:40 PDT
...
Comment 1 Sihui Liu 2019-08-01 19:32:42 PDT
Created attachment 375381 [details]
Patch
Comment 2 Chris Dumez 2019-08-01 20:43:59 PDT
Comment on attachment 375381 [details]
Patch

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

> Source/WebKit/ChangeLog:7
> +

Please write a ChangeLog.
Comment 3 Sam Weinig 2019-08-02 12:48:30 PDT
Comment on attachment 375381 [details]
Patch

Needs ChangeLog.
Comment 4 Sihui Liu 2019-08-02 13:20:02 PDT
Created attachment 375446 [details]
Patch
Comment 5 Sihui Liu 2019-08-02 13:39:57 PDT
Created attachment 375449 [details]
Patch
Comment 6 Geoffrey Garen 2019-08-02 17:15:07 PDT
Under what conditions will (SessionID, SecurityOrigin) be the same, but (StorageNamespace) be different?

I’m trying to understand the feature that StorageNamespace provides. I can’t recall a web api that would distinguish a StorageNamespace.
Comment 7 Sihui Liu 2019-08-02 19:07:28 PDT
(In reply to Geoffrey Garen from comment #6)
> Under what conditions will (SessionID, SecurityOrigin) be the same, but
> (StorageNamespace) be different?
> 
> I’m trying to understand the feature that StorageNamespace provides. I can’t
> recall a web api that would distinguish a StorageNamespace.

sessionStorage from different web pages on the same session. sessionStorage is isolated per tab by spec: https://www.w3.org/TR/webstorage/#the-sessionstorage-attribute.
Comment 8 Sihui Liu 2019-08-02 19:13:58 PDT
(In reply to Sihui Liu from comment #7)
> (In reply to Geoffrey Garen from comment #6)
> > Under what conditions will (SessionID, SecurityOrigin) be the same, but
> > (StorageNamespace) be different?
> > 
> > I’m trying to understand the feature that StorageNamespace provides. I can’t
> > recall a web api that would distinguish a StorageNamespace.
> 
> sessionStorage from different web pages on the same session. sessionStorage
> is isolated per tab by spec:
> https://www.w3.org/TR/webstorage/#the-sessionstorage-attribute.

The StorageNameSpace was our implementation choice to separate storage between different tabs and different web page groups(this does not matter now). 

For SessionStorageNamespace, the ID equals to WebPageID. For LocalStorageNamespace, the ID is WebPageGroupID.
Comment 9 Sihui Liu 2019-08-05 14:48:11 PDT
Created attachment 375561 [details]
Patch
Comment 10 Sihui Liu 2019-08-07 14:53:47 PDT
Created attachment 375756 [details]
Patch
Comment 11 Geoffrey Garen 2019-08-09 15:20:27 PDT
Comment on attachment 375756 [details]
Patch

r=me
Comment 12 Sihui Liu 2019-08-14 15:15:20 PDT
Created attachment 376320 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2019-08-14 16:11:38 PDT
Comment on attachment 376320 [details]
Patch for landing

Rejecting attachment 376320 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 376320, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 5000 characters of output:
orageDatabase.cpp
patching file Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.h
patching file Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.cpp
patching file Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.h
patching file Source/WebKit/NetworkProcess/WebStorage/LocalStorageNamespace.cpp
patching file Source/WebKit/NetworkProcess/WebStorage/LocalStorageNamespace.h
patching file Source/WebKit/NetworkProcess/WebStorage/SessionStorageNamespace.cpp
patching file Source/WebKit/NetworkProcess/WebStorage/SessionStorageNamespace.h
patching file Source/WebKit/NetworkProcess/WebStorage/StorageArea.cpp
patching file Source/WebKit/NetworkProcess/WebStorage/StorageArea.h
patching file Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp
patching file Source/WebKit/NetworkProcess/WebStorage/StorageManager.h
patching file Source/WebKit/NetworkProcess/WebStorage/StorageManager.messages.in
rm 'Source/WebKit/NetworkProcess/WebStorage/StorageManager.messages.in'
patching file Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp
patching file Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.h
patching file Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.messages.in
patching file Source/WebKit/Shared/WebsiteDataStoreParameters.cpp
patching file Source/WebKit/Shared/WebsiteDataStoreParameters.h
patching file Source/WebKit/Sources.txt
patching file Source/WebKit/UIProcess/API/C/WKContext.cpp
patching file Source/WebKit/UIProcess/API/C/WKContextPrivate.h
patching file Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp
Hunk #1 succeeded at 531 (offset 11 lines).
patching file Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.h
Hunk #1 succeeded at 118 (offset 2 lines).
patching file Source/WebKit/UIProcess/WebProcessPool.cpp
Hunk #2 succeeded at 1802 (offset -2 lines).
patching file Source/WebKit/UIProcess/WebProcessPool.h
patching file Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm
Hunk #1 succeeded at 109 (offset 2 lines).
Hunk #2 succeeded at 138 (offset 3 lines).
Hunk #3 succeeded at 173 (offset 3 lines).
patching file Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp
Hunk #1 succeeded at 1969 (offset 12 lines).
patching file Source/WebKit/WebKit.xcodeproj/project.pbxproj
patching file Source/WebKit/WebProcess/InjectedBundle/InjectedBundle.cpp
patching file Source/WebKit/WebProcess/WebProcess.cpp
patching file Source/WebKit/WebProcess/WebProcess.h
patching file Source/WebKit/WebProcess/WebStorage/StorageAreaImpl.cpp
patching file Source/WebKit/WebProcess/WebStorage/StorageAreaImpl.h
patching file Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp
patching file Source/WebKit/WebProcess/WebStorage/StorageAreaMap.h
patching file Source/WebKit/WebProcess/WebStorage/StorageAreaMap.messages.in
patching file Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.cpp
patching file Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.h
patching file Source/WebKit/WebProcess/WebStorage/WebStorageNamespaceProvider.cpp
patching file Source/WebKit/WebProcess/WebStorage/WebStorageNamespaceProvider.h
patching file Source/WebKitLegacy/Storage/StorageAreaImpl.cpp
patching file Source/WebKitLegacy/Storage/StorageAreaImpl.h
patching file Source/WebKitLegacy/Storage/StorageAreaSync.h
patching file Source/WebKitLegacy/Storage/StorageNamespaceImpl.cpp
patching file Source/WebKitLegacy/Storage/StorageNamespaceImpl.h
patching file Source/WebKitLegacy/Storage/WebStorageNamespaceProvider.cpp
patching file Source/WebKitLegacy/Storage/WebStorageNamespaceProvider.h
patching file Source/WebKitLegacy/mac/WebView/WebView.mm
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm
patching file Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl
patching file Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp
patching file Tools/WebKitTestRunner/InjectedBundle/TestRunner.h
patching file Tools/WebKitTestRunner/TestController.cpp
patching file Tools/WebKitTestRunner/TestController.h
Hunk #1 succeeded at 262 (offset 1 line).
patching file Tools/WebKitTestRunner/TestInvocation.cpp
Hunk #1 succeeded at 1659 (offset 9 lines).
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/platform/ios-simulator-wk2/TestExpectations
patching file LayoutTests/platform/mac-wk2/TestExpectations
Hunk #1 succeeded at 922 (offset 2 lines).
patching file LayoutTests/storage/domstorage/localstorage/private-browsing-affects-storage-expected.txt
patching file LayoutTests/storage/indexeddb/IDBObject-leak.html
patching file LayoutTests/storage/indexeddb/modern/opendatabase-after-storage-crash-expected.txt
patching file LayoutTests/storage/indexeddb/modern/opendatabase-after-storage-crash.html

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/12913883
Comment 14 Sihui Liu 2019-08-14 17:12:09 PDT
Created attachment 376336 [details]
Patch for landing
Comment 15 WebKit Commit Bot 2019-08-14 18:58:58 PDT
Comment on attachment 376336 [details]
Patch for landing

Rejecting attachment 376336 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 376336, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 5000 characters of output:
Process/WebStorage/LocalStorageDatabase.h
patching file Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.cpp
patching file Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.h
patching file Source/WebKit/NetworkProcess/WebStorage/LocalStorageNamespace.cpp
patching file Source/WebKit/NetworkProcess/WebStorage/LocalStorageNamespace.h
patching file Source/WebKit/NetworkProcess/WebStorage/SessionStorageNamespace.cpp
patching file Source/WebKit/NetworkProcess/WebStorage/SessionStorageNamespace.h
patching file Source/WebKit/NetworkProcess/WebStorage/StorageArea.cpp
patching file Source/WebKit/NetworkProcess/WebStorage/StorageArea.h
patching file Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp
patching file Source/WebKit/NetworkProcess/WebStorage/StorageManager.h
patching file Source/WebKit/NetworkProcess/WebStorage/StorageManager.messages.in
rm 'Source/WebKit/NetworkProcess/WebStorage/StorageManager.messages.in'
patching file Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp
patching file Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.h
patching file Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.messages.in
patching file Source/WebKit/Shared/WebsiteDataStoreParameters.cpp
patching file Source/WebKit/Shared/WebsiteDataStoreParameters.h
patching file Source/WebKit/Sources.txt
patching file Source/WebKit/UIProcess/API/C/WKContext.cpp
Hunk #1 succeeded at 669 (offset 5 lines).
patching file Source/WebKit/UIProcess/API/C/WKContextPrivate.h
Hunk #1 succeeded at 120 (offset 2 lines).
patching file Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp
patching file Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.h
patching file Source/WebKit/UIProcess/WebProcessPool.cpp
Hunk #2 succeeded at 1809 with fuzz 1 (offset 7 lines).
patching file Source/WebKit/UIProcess/WebProcessPool.h
Hunk #1 succeeded at 317 with fuzz 2.
patching file Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm
patching file Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp
patching file Source/WebKit/WebKit.xcodeproj/project.pbxproj
patching file Source/WebKit/WebProcess/InjectedBundle/InjectedBundle.cpp
patching file Source/WebKit/WebProcess/WebProcess.cpp
patching file Source/WebKit/WebProcess/WebProcess.h
patching file Source/WebKit/WebProcess/WebStorage/StorageAreaImpl.cpp
patching file Source/WebKit/WebProcess/WebStorage/StorageAreaImpl.h
patching file Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp
patching file Source/WebKit/WebProcess/WebStorage/StorageAreaMap.h
patching file Source/WebKit/WebProcess/WebStorage/StorageAreaMap.messages.in
patching file Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.cpp
patching file Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.h
patching file Source/WebKit/WebProcess/WebStorage/WebStorageNamespaceProvider.cpp
patching file Source/WebKit/WebProcess/WebStorage/WebStorageNamespaceProvider.h
patching file Source/WebKitLegacy/Storage/StorageAreaImpl.cpp
patching file Source/WebKitLegacy/Storage/StorageAreaImpl.h
patching file Source/WebKitLegacy/Storage/StorageAreaSync.h
patching file Source/WebKitLegacy/Storage/StorageNamespaceImpl.cpp
patching file Source/WebKitLegacy/Storage/StorageNamespaceImpl.h
patching file Source/WebKitLegacy/Storage/WebStorageNamespaceProvider.cpp
patching file Source/WebKitLegacy/Storage/WebStorageNamespaceProvider.h
patching file Source/WebKitLegacy/mac/WebView/WebView.mm
Hunk #1 succeeded at 2888 (offset -5 lines).
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm
patching file Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl
patching file Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp
patching file Tools/WebKitTestRunner/InjectedBundle/TestRunner.h
patching file Tools/WebKitTestRunner/TestController.cpp
Hunk #1 FAILED at 939.
Hunk #2 succeeded at 3070 with fuzz 1 (offset 6 lines).
Hunk #3 succeeded at 3080 (offset 6 lines).
1 out of 3 hunks FAILED -- saving rejects to file Tools/WebKitTestRunner/TestController.cpp.rej
patching file Tools/WebKitTestRunner/TestController.h
patching file Tools/WebKitTestRunner/TestInvocation.cpp
Hunk #1 succeeded at 1666 (offset 7 lines).
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/platform/ios-simulator-wk2/TestExpectations
patching file LayoutTests/platform/mac-wk2/TestExpectations
patching file LayoutTests/storage/domstorage/localstorage/private-browsing-affects-storage-expected.txt
patching file LayoutTests/storage/indexeddb/IDBObject-leak.html
patching file LayoutTests/storage/indexeddb/modern/opendatabase-after-storage-crash-expected.txt
patching file LayoutTests/storage/indexeddb/modern/opendatabase-after-storage-crash.html

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/12914618
Comment 16 Sihui Liu 2019-08-15 07:55:01 PDT
Created attachment 376379 [details]
Patch for landing
Comment 17 Sihui Liu 2019-08-15 09:05:39 PDT
Created attachment 376382 [details]
Patch for landing
Comment 18 Build Bot 2019-08-15 10:28:34 PDT
Attachment 376382 [details] did not pass style-queue:


ERROR: Source/WebCore/storage/StorageNamespace.h:48:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/WebKit/WebProcess/WebStorage/WebStorageNamespaceProvider.h:42:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:69:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:262:  Missing space before {  [whitespace/braces] [5]
Total errors found: 4 in 77 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Sihui Liu 2019-08-15 11:03:30 PDT
Created attachment 376395 [details]
Patch for landing
Comment 20 WebKit Commit Bot 2019-08-15 11:42:02 PDT
Comment on attachment 376395 [details]
Patch for landing

Clearing flags on attachment: 376395

Committed r248734: <https://trac.webkit.org/changeset/248734>
Comment 21 WebKit Commit Bot 2019-08-15 11:42:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Radar WebKit Bug Importer 2019-08-15 11:43:20 PDT
<rdar://problem/54355962>
Comment 23 Chris Dumez 2019-08-16 09:16:57 PDT
Comment on attachment 376395 [details]
Patch for landing

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

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:105
> +    RunLoop::main().dispatch([origins = WTFMove(origins), completionHandler = WTFMove(completionHandler)]() mutable {

Please don't do this, this is exactly how we end up with thread-safety bug. A method that takes a completion handler MUST call its completion handler on the thread is was called on. Here getSessionStorageOrigins() gets called on a background queue (as asserted at the beginning of this method) and then it calls its completion handler on the main thread. This is super error prone.
Comment 24 Chris Dumez 2019-08-16 09:22:05 PDT
(In reply to Chris Dumez from comment #23)
> Comment on attachment 376395 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376395&action=review
> 
> > Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:105
> > +    RunLoop::main().dispatch([origins = WTFMove(origins), completionHandler = WTFMove(completionHandler)]() mutable {
> 
> Please don't do this, this is exactly how we end up with thread-safety bug.
> A method that takes a completion handler MUST call its completion handler on
> the thread is was called on. Here getSessionStorageOrigins() gets called on
> a background queue (as asserted at the beginning of this method) and then it
> calls its completion handler on the main thread. This is super error prone.

I will write a follow-up. No need to do anything.
Comment 25 Chris Dumez 2019-08-16 09:42:47 PDT
Comment on attachment 376395 [details]
Patch for landing

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

>>> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:105
>>> +    RunLoop::main().dispatch([origins = WTFMove(origins), completionHandler = WTFMove(completionHandler)]() mutable {
>> 
>> Please don't do this, this is exactly how we end up with thread-safety bug. A method that takes a completion handler MUST call its completion handler on the thread is was called on. Here getSessionStorageOrigins() gets called on a background queue (as asserted at the beginning of this method) and then it calls its completion handler on the main thread. This is super error prone.
> 
> I will write a follow-up. No need to do anything.

If those methods were taking proper CompletionHandler data types for their completion handler (instead of the generic Function type), you'd have hit assertions telling you this is unsafe.
Comment 26 Sihui Liu 2019-08-16 10:07:57 PDT
(In reply to Chris Dumez from comment #25)
> Comment on attachment 376395 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376395&action=review
> 
> >>> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:105
> >>> +    RunLoop::main().dispatch([origins = WTFMove(origins), completionHandler = WTFMove(completionHandler)]() mutable {
> >> 
> >> Please don't do this, this is exactly how we end up with thread-safety bug. A method that takes a completion handler MUST call its completion handler on the thread is was called on. Here getSessionStorageOrigins() gets called on a background queue (as asserted at the beginning of this method) and then it calls its completion handler on the main thread. This is super error prone.
> > 
> > I will write a follow-up. No need to do anything.
> 
> If those methods were taking proper CompletionHandler data types for their
> completion handler (instead of the generic Function type), you'd have hit
> assertions telling you this is unsafe.

Yes, may be a nice thing to do to change Function<> ...completionHandler to CompletionHandler<>.
Comment 27 Chris Dumez 2019-08-16 10:13:53 PDT
(In reply to Sihui Liu from comment #26)
> (In reply to Chris Dumez from comment #25)
> > Comment on attachment 376395 [details]
> > Patch for landing
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=376395&action=review
> > 
> > >>> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:105
> > >>> +    RunLoop::main().dispatch([origins = WTFMove(origins), completionHandler = WTFMove(completionHandler)]() mutable {
> > >> 
> > >> Please don't do this, this is exactly how we end up with thread-safety bug. A method that takes a completion handler MUST call its completion handler on the thread is was called on. Here getSessionStorageOrigins() gets called on a background queue (as asserted at the beginning of this method) and then it calls its completion handler on the main thread. This is super error prone.
> > > 
> > > I will write a follow-up. No need to do anything.
> > 
> > If those methods were taking proper CompletionHandler data types for their
> > completion handler (instead of the generic Function type), you'd have hit
> > assertions telling you this is unsafe.
> 
> Yes, may be a nice thing to do to change Function<> ...completionHandler to
> CompletionHandler<>.

Not really, if you look at my patch, you'll see that they are gone entirely since those operations are synchronous.
Comment 28 Sihui Liu 2019-08-16 10:38:42 PDT
(In reply to Chris Dumez from comment #27)
> (In reply to Sihui Liu from comment #26)
> > (In reply to Chris Dumez from comment #25)
> > > Comment on attachment 376395 [details]
> > > Patch for landing
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=376395&action=review
> > > 
> > > >>> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:105
> > > >>> +    RunLoop::main().dispatch([origins = WTFMove(origins), completionHandler = WTFMove(completionHandler)]() mutable {
> > > >> 
> > > >> Please don't do this, this is exactly how we end up with thread-safety bug. A method that takes a completion handler MUST call its completion handler on the thread is was called on. Here getSessionStorageOrigins() gets called on a background queue (as asserted at the beginning of this method) and then it calls its completion handler on the main thread. This is super error prone.
> > > > 
> > > > I will write a follow-up. No need to do anything.
> > > 
> > > If those methods were taking proper CompletionHandler data types for their
> > > completion handler (instead of the generic Function type), you'd have hit
> > > assertions telling you this is unsafe.
> > 
> > Yes, may be a nice thing to do to change Function<> ...completionHandler to
> > CompletionHandler<>.
> 
> Not really, if you look at my patch, you'll see that they are gone entirely
> since those operations are synchronous.

I saw your patch. I mean generally replacing Function<> ...completionHandler with CompletionHandler<> in the code base, so we will hit the assertion and notice the bad practice.