Bug 143339 – [WK2] Storage areas should get torn down when there are no remaining references to them.

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

[Andreas Kling]

We shouldn't keep local storage areas open forever.

[Andreas Kling]

Created attachment 249989 [details]
Patch for EWS

[Andreas Kling]

Created attachment 249993 [details]
Proposed patch

[Andreas Kling]

Created attachment 249997 [details]
Patch

[Darin Adler]

Comment on attachment 249997 [details]
Patch

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

> Source/WebKit2/ChangeLog:15
> +        Practically speaking, this means that the garbage collector now decides when local
> +        storage databases can be closed, instead of us keeping them open for the lifetime
> +        of the web process.

When you put it that way, it does seem a bit lame. Is there no way this can be based on some better defined event other than “when the object is garbage collected”?

Further, can we add some extra weight to the objects tat are holding the databases open so they are more likely to be collected even when there is plenty of RAM still available?

> Source/WebKit2/WebProcess/Storage/StorageNamespaceImpl.cpp:86
> +    return StorageAreaImpl::create(map);

Could this be either map.release() or WTF::move(map) to avoid a little churn?

[Geoffrey Garen]

Comment on attachment 249997 [details]
Patch

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

>> Source/WebKit2/ChangeLog:15
>> +        of the web process.
> 
> When you put it that way, it does seem a bit lame. Is there no way this can be based on some better defined event other than “when the object is garbage collected”?
> 
> Further, can we add some extra weight to the objects tat are holding the databases open so they are more likely to be collected even when there is plenty of RAM still available?

Does the spec allow us to close a storage unconditionally after navigation (even if the storage is still referenced by another webpage)?

[Andreas Kling]

(In reply to comment #5)
> Comment on attachment 249997 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249997&action=review
> 
> >> Source/WebKit2/ChangeLog:15
> >> +        of the web process.
> > 
> > When you put it that way, it does seem a bit lame. Is there no way this can be based on some better defined event other than “when the object is garbage collected”?
> > 
> > Further, can we add some extra weight to the objects tat are holding the databases open so they are more likely to be collected even when there is plenty of RAM still available?
> 
> Does the spec allow us to close a storage unconditionally after navigation
> (even if the storage is still referenced by another webpage)?

The intent here is to make a change that's not observable from the web.

Anders pointed out that this change may break session storage, so I am currently looking into that.

[David Kilzer (:ddkilzer)]

<rdar://problem/20156436>

[Andreas Kling]

(In reply to comment #4)
> Comment on attachment 249997 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249997&action=review
> 
> > Source/WebKit2/ChangeLog:15
> > +        Practically speaking, this means that the garbage collector now decides when local
> > +        storage databases can be closed, instead of us keeping them open for the lifetime
> > +        of the web process.
> 
> When you put it that way, it does seem a bit lame. Is there no way this can
> be based on some better defined event other than “when the object is garbage
> collected”?
> 
> Further, can we add some extra weight to the objects tat are holding the
> databases open so they are more likely to be collected even when there is
> plenty of RAM still available?

That would be nice. StorageArea already has this:

    virtual size_t memoryBytesUsedByCache() = 0;

But everyone just implements it as { return 0; }. :-/

> > Source/WebKit2/WebProcess/Storage/StorageNamespaceImpl.cpp:86
> > +    return StorageAreaImpl::create(map);
> 
> Could this be either map.release() or WTF::move(map) to avoid a little churn?

Definitely.

[Andreas Kling]

Created attachment 253766 [details]
Better patch that's less wrong

[Andreas Kling]

Created attachment 253775 [details]
Patch

[Darin Adler]

Comment on attachment 253775 [details]
Patch

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

I think Anders gave this an informal thumbs up. Based on that plus my reading of this, review+.

> Source/WebKit2/UIProcess/Storage/StorageManager.cpp:754
> +        Ref<StorageArea> area = *it.value;
> +        if (!area->isSessionStorage())
> +            continue;
> +        if (!origin->isSameSchemeHostPort(area->securityOrigin()))
> +            continue;

Could restructure this and make it a bit uglier but remove a tiny bit of reference count churn like this:

    auto& area = it.value;
    if (...)
        ...
    Ref<StorageArea> movedArea = WTF::move(area);
    ...
    m_storageAreasByConnection.add({ &connection, storageMapID }, WTF::move(movedArea));

> Source/WebKit2/UIProcess/Storage/StorageManager.cpp:755
> +        m_storageAreasByConnection.remove(it.key);

This should be remove(it) rather than remove(it.key) to avoid re-hashing the key and doing equality comparisons just to find the hash table entry that "it" is already pointing at.

> Source/WebKit2/WebProcess/Storage/StorageAreaMap.cpp:61
>  StorageAreaMap::StorageAreaMap(StorageNamespaceImpl* storageNamespace, Ref<WebCore::SecurityOrigin>&& securityOrigin)

Someone should come back here and make StorageNamespaceImpl a reference.

[Andreas Kling]

Created attachment 253816 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 253816 [details]
Patch for landing

Clearing flags on attachment: 253816

Committed r184930: <http://trac.webkit.org/changeset/184930>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.