RESOLVED FIXED 200373
Some improvements on web storage
https://bugs.webkit.org/show_bug.cgi?id=200373
Summary Some improvements on web storage
Sihui Liu
Reported 2019-08-01 18:54:40 PDT
...
Attachments
Patch (206.73 KB, patch)
2019-08-01 19:32 PDT, Sihui Liu
no flags
Patch (185.97 KB, patch)
2019-08-02 13:20 PDT, Sihui Liu
no flags
Patch (210.10 KB, patch)
2019-08-02 13:39 PDT, Sihui Liu
no flags
Patch (222.35 KB, patch)
2019-08-05 14:48 PDT, Sihui Liu
no flags
Patch (247.38 KB, patch)
2019-08-07 14:53 PDT, Sihui Liu
no flags
Patch for landing (247.42 KB, patch)
2019-08-14 15:15 PDT, Sihui Liu
no flags
Patch for landing (248.06 KB, patch)
2019-08-14 17:12 PDT, Sihui Liu
no flags
Patch for landing (247.74 KB, patch)
2019-08-15 07:55 PDT, Sihui Liu
no flags
Patch for landing (247.91 KB, patch)
2019-08-15 09:05 PDT, Sihui Liu
no flags
Patch for landing (246.53 KB, patch)
2019-08-15 11:03 PDT, Sihui Liu
no flags
Sihui Liu
Comment 1 2019-08-01 19:32:42 PDT
Chris Dumez
Comment 2 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.
Sam Weinig
Comment 3 2019-08-02 12:48:30 PDT
Comment on attachment 375381 [details] Patch Needs ChangeLog.
Sihui Liu
Comment 4 2019-08-02 13:20:02 PDT
Sihui Liu
Comment 5 2019-08-02 13:39:57 PDT
Geoffrey Garen
Comment 6 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.
Sihui Liu
Comment 7 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.
Sihui Liu
Comment 8 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.
Sihui Liu
Comment 9 2019-08-05 14:48:11 PDT
Sihui Liu
Comment 10 2019-08-07 14:53:47 PDT
Geoffrey Garen
Comment 11 2019-08-09 15:20:27 PDT
Comment on attachment 375756 [details] Patch r=me
Sihui Liu
Comment 12 2019-08-14 15:15:20 PDT
Created attachment 376320 [details] Patch for landing
WebKit Commit Bot
Comment 13 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
Sihui Liu
Comment 14 2019-08-14 17:12:09 PDT
Created attachment 376336 [details] Patch for landing
WebKit Commit Bot
Comment 15 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
Sihui Liu
Comment 16 2019-08-15 07:55:01 PDT
Created attachment 376379 [details] Patch for landing
Sihui Liu
Comment 17 2019-08-15 09:05:39 PDT
Created attachment 376382 [details] Patch for landing
EWS Watchlist
Comment 18 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.
Sihui Liu
Comment 19 2019-08-15 11:03:30 PDT
Created attachment 376395 [details] Patch for landing
WebKit Commit Bot
Comment 20 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>
WebKit Commit Bot
Comment 21 2019-08-15 11:42:04 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 22 2019-08-15 11:43:20 PDT
Chris Dumez
Comment 23 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.
Chris Dumez
Comment 24 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.
Chris Dumez
Comment 25 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.
Sihui Liu
Comment 26 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<>.
Chris Dumez
Comment 27 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.
Sihui Liu
Comment 28 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.
Chris Dumez
Comment 29 2020-01-17 08:14:15 PST
Comment on attachment 376395 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=376395&action=review > Source/WebCore/ChangeLog:25 > + (WebCore::DOMWindow::prewarmLocalStorageIfNecessary): localStorage will be prewarmed in network process in the Could you point me to the code that does pre-warming now? I cannot find it and we are seeing hangs in StorageManagerSet::GetValues again.
Sihui Liu
Comment 30 2020-01-21 09:32:32 PST
Comment on attachment 376395 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=376395&action=review >> Source/WebCore/ChangeLog:25 >> + (WebCore::DOMWindow::prewarmLocalStorageIfNecessary): localStorage will be prewarmed in network process in the > > Could you point me to the code that does pre-warming now? I cannot find it and we are seeing hangs in StorageManagerSet::GetValues again. Sorry I missed see this comment.. I did not change the logic of prewarming in web process(we still need to call prewarmLocalStorageIfNecessary in DocumentLodaer), but removed the prewarm message and merged it into ConnectToLocalStorageArea. (so network process imports database when receiving ConnectToLocalStorageArea) Therefore I think https://trac.webkit.org/changeset/254753/webkit is not right.
Chris Dumez
Comment 31 2020-01-31 14:26:57 PST
Comment on attachment 376395 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=376395&action=review > Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.h:54 > void didDestroyStorageAreaMap(StorageAreaMap&); This is never called anymore. > Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.h:74 > + HashMap<WebCore::SecurityOriginData, RefPtr<StorageAreaMap>> m_storageAreaMaps; Nobody is ever removing entries from this map so I suspect we have a leak here. The only method that removes things from this map is didDestroyStorageAreaMap() but it is dead code after this change. We found this after investigating a LocalStorage leak for <rdar://problem/58550164>.
Chris Dumez
Comment 32 2020-01-31 14:42:44 PST
(In reply to Chris Dumez from comment #31) > Comment on attachment 376395 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=376395&action=review > > > Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.h:54 > > void didDestroyStorageAreaMap(StorageAreaMap&); > > This is never called anymore. > > > Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.h:74 > > + HashMap<WebCore::SecurityOriginData, RefPtr<StorageAreaMap>> m_storageAreaMaps; > > Nobody is ever removing entries from this map so I suspect we have a leak > here. The only method that removes things from this map is > didDestroyStorageAreaMap() but it is dead code after this change. We found > this after investigating a LocalStorage leak for <rdar://problem/58550164>. This patch seems to have reverted http://trac.webkit.org/changeset/184930, which was a fix for jetsams on iOS. I suspect this re-introduced the leak that http://trac.webkit.org/changeset/184930 fixed.
Note You need to log in before you can comment on or make changes to this bug.