...
Created attachment 375381 [details] Patch
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 on attachment 375381 [details] Patch Needs ChangeLog.
Created attachment 375446 [details] Patch
Created attachment 375449 [details] Patch
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.
(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.
(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.
Created attachment 375561 [details] Patch
Created attachment 375756 [details] Patch
Comment on attachment 375756 [details] Patch r=me
Created attachment 376320 [details] Patch for landing
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
Created attachment 376336 [details] Patch for landing
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
Created attachment 376379 [details] Patch for landing
Created attachment 376382 [details] Patch for landing
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.
Created attachment 376395 [details] Patch for landing
Comment on attachment 376395 [details] Patch for landing Clearing flags on attachment: 376395 Committed r248734: <https://trac.webkit.org/changeset/248734>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54355962>
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.
(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 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.
(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<>.
(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.
(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.
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.
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.
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>.
(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.