Bug 207073 – Regression(r248734) StorageAreaMap objects are getting leaked

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Partial revert of r248734 to fix leak of StorageAreaMaps in WebContent processes.

[Chris Dumez]

Created attachment 389423 [details]
WIP Patch

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

[Sihui Liu]

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.

[Chris Dumez]

Created attachment 389602 [details]
Patch

[Alex Christensen]

Comment on attachment 389602 [details]
Patch

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

[Chris Dumez]

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.

[Chris Dumez]

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

[Alex Christensen]

Comment on attachment 389602 [details]
Patch

Could you mention which radar this is from?

[Chris Dumez]

Comment on attachment 389602 [details]
Patch

Clearing flags on attachment: 389602

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

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59168065>

[Chris Dumez]

Reverted r255706 for reason:

Caused assertions in API tests

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

[Chris Dumez]

Created attachment 389821 [details]
Patch

[Chris Dumez]

Created attachment 389826 [details]
Patch

[Chris Dumez]

Alternative patch which does not revert Sihui's change.

[Darin Adler]

Comment on attachment 389826 [details]
Patch

Should we come up with some way to test this?

[Chris Dumez]

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

[Chris Dumez]

Created attachment 389870 [details]
Patch

[Chris Dumez]

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

[Darin Adler]

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

[Chris Dumez]

Created attachment 389884 [details]
Patch

[Chris Dumez]

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

[WebKit Commit Bot]

Comment on attachment 389884 [details]
Patch

Clearing flags on attachment: 389884

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

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Fujii Hironori]

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