Bug 207073 - Regression(r248734) StorageAreaMap objects are getting leaked
Summary: Regression(r248734) StorageAreaMap objects are getting leaked
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 200373
  Show dependency treegraph
 
Reported: 2020-01-31 15:12 PST by Chris Dumez
Modified: 2020-02-05 20:25 PST (History)
9 users (show)

See Also:


Attachments
WIP Patch (7.43 KB, patch)
2020-01-31 15:13 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (9.63 KB, patch)
2020-02-03 16:53 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (13.25 KB, patch)
2020-02-05 09:49 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (4.78 KB, patch)
2020-02-05 10:16 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (15.18 KB, patch)
2020-02-05 14:41 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (15.23 KB, patch)
2020-02-05 15:26 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2020-01-31 15:12:23 PST
Partial revert of r248734 to fix leak of StorageAreaMaps in WebContent processes.
Comment 1 Chris Dumez 2020-01-31 15:13:14 PST
Created attachment 389423 [details]
WIP Patch

We are in the process of A/B testing this.
Comment 2 Sihui Liu 2020-01-31 17:54:35 PST
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.
Comment 3 Chris Dumez 2020-02-03 16:53:11 PST
Created attachment 389602 [details]
Patch
Comment 4 Alex Christensen 2020-02-04 10:24:04 PST
Comment on attachment 389602 [details]
Patch

How does keeping a strong reference to something that was weak fix a memory leak?
Comment 5 Chris Dumez 2020-02-04 10:30:37 PST
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.
Comment 6 Chris Dumez 2020-02-04 10:32:55 PST
(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 7 Alex Christensen 2020-02-04 14:09:05 PST
Comment on attachment 389602 [details]
Patch

Could you mention which radar this is from?
Comment 8 Chris Dumez 2020-02-04 15:22:48 PST
Comment on attachment 389602 [details]
Patch

Clearing flags on attachment: 389602

Committed r255706: <https://trac.webkit.org/changeset/255706>
Comment 9 Chris Dumez 2020-02-04 15:22:49 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2020-02-04 15:23:18 PST
<rdar://problem/59168065>
Comment 11 Chris Dumez 2020-02-05 08:39:18 PST
Reverted r255706 for reason:

Caused assertions in API tests

Committed r255815: <https://trac.webkit.org/changeset/255815>
Comment 12 Chris Dumez 2020-02-05 09:49:57 PST
Created attachment 389821 [details]
Patch
Comment 13 Chris Dumez 2020-02-05 10:16:31 PST
Created attachment 389826 [details]
Patch
Comment 14 Chris Dumez 2020-02-05 10:16:58 PST
Alternative patch which does not revert Sihui's change.
Comment 15 Darin Adler 2020-02-05 13:33:53 PST
Comment on attachment 389826 [details]
Patch

Should we come up with some way to test this?
Comment 16 Chris Dumez 2020-02-05 13:39:42 PST
(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.
Comment 17 Chris Dumez 2020-02-05 14:41:46 PST
Created attachment 389870 [details]
Patch
Comment 18 Chris Dumez 2020-02-05 14:42:17 PST
Requesting review again since I added a test and the test infrastructure for it.
Comment 19 Darin Adler 2020-02-05 15:18:49 PST
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".
Comment 20 Chris Dumez 2020-02-05 15:26:09 PST
Created attachment 389884 [details]
Patch
Comment 21 Chris Dumez 2020-02-05 15:26:58 PST
(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 22 WebKit Commit Bot 2020-02-05 16:11:27 PST
Comment on attachment 389884 [details]
Patch

Clearing flags on attachment: 389884

Committed r255875: <https://trac.webkit.org/changeset/255875>
Comment 23 WebKit Commit Bot 2020-02-05 16:11:29 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 Fujii Hironori 2020-02-05 20:25:10 PST
Committed r255898: <https://trac.webkit.org/changeset/255898>