Bug 200373 – Some improvements on web storage

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sihui Liu]

...

[Sihui Liu]

Created attachment 375381 [details]
Patch

[Chris Dumez]

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 on attachment 375381 [details]
Patch

Needs ChangeLog.

[Sihui Liu]

Created attachment 375446 [details]
Patch

[Sihui Liu]

Created attachment 375449 [details]
Patch

[Geoffrey Garen]

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]

(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]

(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]

Created attachment 375561 [details]
Patch

[Sihui Liu]

Created attachment 375756 [details]
Patch

[Geoffrey Garen]

Comment on attachment 375756 [details]
Patch

r=me

[Sihui Liu]

Created attachment 376320 [details]
Patch for landing

[WebKit Commit Bot]

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]

Created attachment 376336 [details]
Patch for landing

[WebKit Commit Bot]

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]

Created attachment 376379 [details]
Patch for landing

[Sihui Liu]

Created attachment 376382 [details]
Patch for landing

[EWS Watchlist]

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]

Created attachment 376395 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 376395 [details]
Patch for landing

Clearing flags on attachment: 376395

Committed r248734: <https://trac.webkit.org/changeset/248734>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54355962>

[Chris Dumez]

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]

(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 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]

(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]

(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]

(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 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 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 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]

(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.