Bug 207073

Created attachment 389423 [details]
WIP Patch

We are in the process of A/B testing this.

I agree that we should remove web storage in web process if no page is using them any more.

I don't quite remember the reason why I let StorageNamespaceImpl own StorageAreaMap. I can think of two:
1. To somehow support session change (we no longer do this in tests).
2. To keep the StorageAreaMap around for a while so we don't need the sync messages to connect and get values from network process next time we visit the same origins.
With PSON, I think it's probably Okay to do 2. It would be like: let StorageAreaMap know whether it's used, and start a cleanup cache timer when its last client goes away.

Current approach by recovering the old behavior should work too.

Created attachment 389602 [details]
Patch

Comment on attachment 389602 [details]
Patch

How does keeping a strong reference to something that was weak fix a memory leak?

Comment on attachment 389602 [details]
Patch

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

> Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:68
> +    m_namespace.didDestroyStorageAreaMap(*this);

This is the code that removes the StorageAreaMap raw pointer from the StorageNamespaceImpl::m_storageAreaMaps HashMap. Previously, didDestroyStorageAreaMap() was dead code.

> Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.h:-82
> -    HashMap<WebCore::SecurityOriginData, std::unique_ptr<StorageAreaMap>> m_storageAreaMaps;

This HashMap was keeping the StorageAreaMaps alive because nothing was ever removing the maps from m_storageAreaMaps.

> Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.h:82
> +    HashMap<WebCore::SecurityOriginData, StorageAreaMap*> m_storageAreaMaps;

Now it is a map of raw pointers so it does not keep the StorageAreaMap alive. Only WebCore's Storage object keeps its StorageAreaMap alive.

(In reply to Chris Dumez from comment #5)
> Comment on attachment 389602 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389602&action=review
> 
> > Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:68
> > +    m_namespace.didDestroyStorageAreaMap(*this);
> 
> This is the code that removes the StorageAreaMap raw pointer from the
> StorageNamespaceImpl::m_storageAreaMaps HashMap. Previously,
> didDestroyStorageAreaMap() was dead code.
> 
> > Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.h:-82
> > -    HashMap<WebCore::SecurityOriginData, std::unique_ptr<StorageAreaMap>> m_storageAreaMaps;
> 
> This HashMap was keeping the StorageAreaMaps alive because nothing was ever
> removing the maps from m_storageAreaMaps.
> 
> > Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.h:82
> > +    HashMap<WebCore::SecurityOriginData, StorageAreaMap*> m_storageAreaMaps;
> 
> Now it is a map of raw pointers so it does not keep the StorageAreaMap
> alive. Only WebCore's Storage object keeps its StorageAreaMap alive.

In other words, the ownership is reversed. WebCore now owns the StorageAreaMaps, not the StorageNamespaceImpl at the WebKit layer. WebCore objects do get destroyed so StorageAreaMap objects get destroyed too now.
Previously, they would leak because the StorageNamespaceImpl would never remove StorageAreaMap from its HashMap and it would be the owner. This restores pre-r248734 behavior.

Comment on attachment 389602 [details]
Patch

Could you mention which radar this is from?

Comment on attachment 389602 [details]
Patch

Clearing flags on attachment: 389602

Committed r255706: <https://trac.webkit.org/changeset/255706>

All reviewed patches have been landed.  Closing bug.

<rdar://problem/59168065>

Reverted r255706 for reason:

Caused assertions in API tests

Committed r255815: <https://trac.webkit.org/changeset/255815>

Created attachment 389821 [details]
Patch

Created attachment 389826 [details]
Patch

Alternative patch which does not revert Sihui's change.

Comment on attachment 389826 [details]
Patch

Should we come up with some way to test this?

(In reply to Darin Adler from comment #15)
> Comment on attachment 389826 [details]
> Patch
> 
> Should we come up with some way to test this?

I will give it a shot. I think it is in theory possible with some extra test infrastructure.

Created attachment 389870 [details]
Patch

Requesting review again since I added a test and the test infrastructure for it.

Comment on attachment 389870 [details]
Patch

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

Looks great.

> Source/WebCore/storage/StorageNamespaceProvider.h:62
> +    WEBCORE_EXPORT StorageNamespace& localStorageNamespace(PAL::SessionID);

Isn’t there a separate export macro just for internals? If so, we should ideally use it here. Or if the distinction doesn’t matter we should eventually get rid of it and just always use WEBCORE_EXPORT.

> LayoutTests/TestExpectations:830
> +# This test is only releveant for WebKit2.

Spelling error: "relevant". Some day I hope we can retire the term "WebKit2" and move more fully to "modern" and "legacy".

Created attachment 389884 [details]
Patch

(In reply to Darin Adler from comment #19)
> Comment on attachment 389870 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389870&action=review
> 
> Looks great.
> 
> > Source/WebCore/storage/StorageNamespaceProvider.h:62
> > +    WEBCORE_EXPORT StorageNamespace& localStorageNamespace(PAL::SessionID);
> 
> Isn’t there a separate export macro just for internals? If so, we should
> ideally use it here. Or if the distinction doesn’t matter we should
> eventually get rid of it and just always use WEBCORE_EXPORT.

Switched to WEBCORE_TESTSUPPORT_EXPORT.

> 
> > LayoutTests/TestExpectations:830
> > +# This test is only releveant for WebKit2.
> 
> Spelling error: "relevant". Some day I hope we can retire the term "WebKit2"
> and move more fully to "modern" and "legacy".

Fixed the typo.

Comment on attachment 389884 [details]
Patch

Clearing flags on attachment: 389884

Committed r255875: <https://trac.webkit.org/changeset/255875>

All reviewed patches have been landed.  Closing bug.

Committed r255898: <https://trac.webkit.org/changeset/255898>