Regression (r288298): NetworkStorageManager sends messages to wrong StorageAreaMap

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

...

[Sihui Liu]

Created attachment 449898 [details]
Patch

[Darin Adler]

Comment on attachment 449898 [details]
Patch

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

> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.messages.in:32
> +    SetItem(WebKit::StorageAreaIdentifier identifier, WebKit::StorageAreaImplIdentifier implIdentifier, uint64_t storageMapSeed, String key, String value, String urlString) -> (uint64_t storageMapSeed, String key, bool quotaException) Async WantsConnection
> +    RemoveItem(WebKit::StorageAreaIdentifier identifier, WebKit::StorageAreaImplIdentifier implIdentifier, uint64_t storageMapSeed, String key, String urlString) -> (uint64_t storageMapSeed, String key) Async WantsConnection

We don’t need these keys in the return values, can just capture them instead. See below ...

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:692
> +    completionHandler(storageMapSeed, key, hasQuotaError);

Why not use WTFMove(key) here to save a tiny bit of reference count churn? Except if you take the suggestion of not sending the key back in the return value, then you can use that WTFMove(key) above instead of String { key }.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:700
>          storageArea->removeItem(connection.uniqueID(), implIdentifier, key, WTFMove(urlString));

May be able to use WTFMove(key) here if we get rid of the key below. But if the removeItem function takes a const String&, then I guess that won’t be helpful.

> Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:108
> +    auto callback = [weakThis = WeakPtr { *this }](uint64_t storageMapSeed, String key, bool quotaException) mutable {
> +        if (weakThis)
> +            weakThis->didSetItem(storageMapSeed, key, quotaException);
> +    };

We could capture a copy of the key here and then we would not need the completion handler to pass the key back to us, saving some XPC.

> Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:134
> +    auto callback = [weakThis = WeakPtr { *this }](uint64_t storageMapSeed, String key) mutable {
> +        if (weakThis)
> +            weakThis->didRemoveItem(storageMapSeed, key);
> +    };

Ditto.

[Sihui Liu]

(In reply to Darin Adler from comment #2)
> Comment on attachment 449898 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449898&action=review

Thanks for the review.

> 
> > Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.messages.in:32
> > +    SetItem(WebKit::StorageAreaIdentifier identifier, WebKit::StorageAreaImplIdentifier implIdentifier, uint64_t storageMapSeed, String key, String value, String urlString) -> (uint64_t storageMapSeed, String key, bool quotaException) Async WantsConnection
> > +    RemoveItem(WebKit::StorageAreaIdentifier identifier, WebKit::StorageAreaImplIdentifier implIdentifier, uint64_t storageMapSeed, String key, String urlString) -> (uint64_t storageMapSeed, String key) Async WantsConnection
> 
> We don’t need these keys in the return values, can just capture them
> instead. See below ...

Ah right, will change.
> 
> > Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:692
> > +    completionHandler(storageMapSeed, key, hasQuotaError);
> 
> Why not use WTFMove(key) here to save a tiny bit of reference count churn?
> Except if you take the suggestion of not sending the key back in the return
> value, then you can use that WTFMove(key) above instead of String { key }.
> 
> > Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:700
> >          storageArea->removeItem(connection.uniqueID(), implIdentifier, key, WTFMove(urlString));
> 
> May be able to use WTFMove(key) here if we get rid of the key below. But if
> the removeItem function takes a const String&, then I guess that won’t be
> helpful.
> 
> > Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:108
> > +    auto callback = [weakThis = WeakPtr { *this }](uint64_t storageMapSeed, String key, bool quotaException) mutable {
> > +        if (weakThis)
> > +            weakThis->didSetItem(storageMapSeed, key, quotaException);
> > +    };
> 
> We could capture a copy of the key here and then we would not need the
> completion handler to pass the key back to us, saving some XPC.
> 
> > Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:134
> > +    auto callback = [weakThis = WeakPtr { *this }](uint64_t storageMapSeed, String key) mutable {
> > +        if (weakThis)
> > +            weakThis->didRemoveItem(storageMapSeed, key);
> > +    };
> 
> Ditto.

[Sihui Liu]

Created attachment 449902 [details]
Patch

[EWS]

Committed r288551 (246383@main): <https://commits.webkit.org/246383@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449902 [details].

[Radar WebKit Bug Importer]

<rdar://problem/88025071>