Bug 200895 - Use a strongly typed identifier for StorageNamespace's identifier
Summary: Use a strongly typed identifier for StorageNamespace's identifier
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:
 
Reported: 2019-08-19 13:33 PDT by Chris Dumez
Modified: 2021-08-20 15:47 PDT (History)
8 users (show)

See Also:


Attachments
Patch (48.27 KB, patch)
2019-08-19 13:36 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (49.89 KB, patch)
2019-08-20 09:03 PDT, 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 2019-08-19 13:33:09 PDT
Use a strongly typed identifier for StorageNamespace's identifier instead of uint64_t.
Comment 1 Chris Dumez 2019-08-19 13:36:44 PDT
Created attachment 376705 [details]
Patch
Comment 2 Chris Dumez 2019-08-20 08:25:15 PDT
ping review?
Comment 3 youenn fablet 2019-08-20 08:38:29 PDT
Comment on attachment 376705 [details]
Patch

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

> Source/WebCore/storage/StorageNamespace.h:45
> +    virtual Ref<StorageNamespace> copy(Page* newPage) = 0; // Only valid for session storage.

Comment is not really clear to me. I am not sure how it relates to the current patch.
If we want to precise the meaning of copy, maybe we should precise the name of the method?

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:195
> +StorageArea* StorageManager::createLocalStorageArea(StorageNamespaceIdentifier storageNamespaceID, WebCore::SecurityOriginData&& origin, Ref<WorkQueue>&& workQueue)

We can probably remove WebCore:: here.

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:205
> +StorageArea* StorageManager::createTransientLocalStorageArea(StorageNamespaceIdentifier storageNamespaceID, WebCore::SecurityOriginData&& topLevelOrigin, WebCore::SecurityOriginData&& origin, Ref<WorkQueue>&& workQueue)

Ditto.

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:216
> +StorageArea* StorageManager::createSessionStorageArea(StorageNamespaceIdentifier storageNamespaceID, WebCore::SecurityOriginData&& origin, Ref<WorkQueue>&& workQueue)

Ditto.

> Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.cpp:114
> +    return makeObjectIdentifier<PageIdentifierType>(m_storageNamespaceID.toUInt64());

This is preexisting code but seems a bit odd to me.

> Source/WebKit/WebProcess/WebStorage/WebStorageNamespaceProvider.cpp:54
>      return storageNamespaceProvider;

HashMap::ensure maybe
Comment 4 Chris Dumez 2019-08-20 08:45:58 PDT
Comment on attachment 376705 [details]
Patch

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

>> Source/WebCore/storage/StorageNamespace.h:45
>> +    virtual Ref<StorageNamespace> copy(Page* newPage) = 0; // Only valid for session storage.
> 
> Comment is not really clear to me. I am not sure how it relates to the current patch.
> If we want to precise the meaning of copy, maybe we should precise the name of the method?

I noticed while writing this patch (because it was made obvious by the new code factoring) that the implementation of this function is only correct if the StorageNamespace object is for session storage. If the StorageNamespace object is used for local storage, then it does not do the right thing. I think that ideally, this would be moved to a subclass (e.g. SessionStorageNamespace which we would need to create) but this is a bit out of scope of this patch, so I merely put in a comment.
Comment 5 youenn fablet 2019-08-20 08:47:47 PDT
Maybe a FIXME would be more adequate then.
Comment 6 Chris Dumez 2019-08-20 09:03:01 PDT
Created attachment 376773 [details]
Patch
Comment 7 WebKit Commit Bot 2019-08-20 09:35:02 PDT
Comment on attachment 376773 [details]
Patch

Clearing flags on attachment: 376773

Committed r248901: <https://trac.webkit.org/changeset/248901>
Comment 8 WebKit Commit Bot 2019-08-20 09:35:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-08-20 09:36:18 PDT
<rdar://problem/54514978>
Comment 10 Simon Fraser (smfr) 2021-08-20 15:46:27 PDT
Comment on attachment 376773 [details]
Patch

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

> Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.cpp:120
> +    ASSERT(m_storageType == StorageType::Local || m_storageType == StorageType::TransientLocal);
> +    return m_storageNamespaceID.toUInt64();

Why is this returning the m_storageNamespaceID as a page group identifier?
Comment 11 Alex Christensen 2021-08-20 15:47:20 PDT
Comment on attachment 376773 [details]
Patch

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

>> Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.cpp:120
>> +    return m_storageNamespaceID.toUInt64();
> 
> Why is this returning the m_storageNamespaceID as a page group identifier?

This is among the many reasons I've been working to remove page groups from WebKit2.