Bug 190466 – [GTK][WPE] Add DeviceIdHashSaltStorage disk persistency

• Component: WebRTC
• Status: RESOLVED FIXED

[Alejandro G. Castro]

We need to store the hash salts in the disk to make the storage complete

[Alejandro G. Castro]

Created attachment 352036 [details]
WIP

This is basically the WIP we had implemented rebased after the last commit. The main missing part is adding lastTimeUsed persistency, as discussed we will use encoders and decoders.

[Alejandro G. Castro]

Created attachment 352568 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Eric Carlson]

Comment on attachment 352568 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/APIWebsiteDataStoreCocoa.mm:212
> +WTF::String WebsiteDataStore::defaultDeviceIdHashSaltsStorageDirectory()
> +{
> +    // Not implemented.
> +    return String();
> +}

Did you mean to call this "legacyDefaultDeviceIdHashSaltsStorageDirectory".

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:43
> +static constexpr unsigned hashSaltSize  { 48 };

Nit: extra space before the "{".

[youenn fablet]

Comment on attachment 352568 [details]
Patch

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

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:42
> +static constexpr unsigned deviceIdHashSaltStorageVersion { 2 };

How about starting with 1?

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:88
> +                continue;

What is done for NetworkCache is to create a folder with the version as a name.
This allows to more easily migrate data from one version to another if needs be.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:123
> +    m_queue->dispatch([this, protectedThis = makeRef(*this), &hashSaltForOrigin]() mutable {

If you take a ref here, you need to ensure to deref it in the same thread.
I believe this can be handled more easily by making DeviceIdHashSaltStorage ThreadSafeRefCounted< DeviceIdHashSaltStorage, WTF::DestructionThread::Main>

Probably need to capture hashSaltForOrigin by value.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:124
> +        writeEncoderToDisk(createEncoderFromData(hashSaltForOrigin), FileSystem::pathByAppendingComponent(m_deviceIdHashSaltStorageDirectory, hashSaltForOrigin.deviceIdHashSalt.utf8().data()));

s/writeEncoderToDisk/writeToDisk/ maybe?

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:153
> +        auto newHashSaltForOrigin = std::make_unique<HashSaltForOrigin>(documentOrigin.data().isolatedCopy(), WTFMove(deviceIdHashSalt));

Why should we isolatedCopy documentOrigin.data().
It might be best to do that in storeHashSaltToDisk, even though this is less efficient, this will be more robust.
Otherwise, we could inline storeHashSaltToDisk here.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:180
> +    m_queue->dispatch([this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)]() mutable {

Why do we need to go to the background thread to query m_deviceIdHashSaltForOrigins?
Can we try querying/modifying it in the main thread?

One problem might be that at start-up time, we will need to read from disk and then load the data.
One solution would be to always pass through an initialize method that takes a completion handler.
The initialize method would be responsible to load the data from the background thread and store the callbacks until this is done.
Once done, call the callbacks so that they can query/modify the data.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:184
> +            origins.add(hashSaltForOrigin.value->documentOrigin);

If we go this way, we probably need to isolate origin values.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:192
> +bool DeviceIdHashSaltStorage::deleteHashSaltFromDiskIfNeeded(HashSaltForOrigin& hashSaltForOrigin, bool needsRemoval)

Can we try having this method return void?

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:206
> +    Vector<SecurityOriginData> copiedOrigins = origins;

Ditto for isolated copies.
Can we try just writing:
m_deviceIdHashSaltForOrigins.removeIf([this, &copiedOrigins](auto& keyAndValue) {
            return this->deleteHashSaltFromDiskIfNeeded(*keyAndValue.value.get(), copiedOrigins.contains(keyAndValue.value->documentOrigin));
});
Or if we are concerned about too much writing, putting the removed entries in a vector and pass all entries to deleteHashSaltFromDiskIfNeeded at once.

[Alejandro G. Castro]

(In reply to Eric Carlson from comment #4)
> Comment on attachment 352568 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352568&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/APIWebsiteDataStoreCocoa.mm:212
> > +WTF::String WebsiteDataStore::defaultDeviceIdHashSaltsStorageDirectory()
> > +{
> > +    // Not implemented.
> > +    return String();
> > +}
> 
> Did you mean to call this "legacyDefaultDeviceIdHashSaltsStorageDirectory".
> 

Right.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:43
> > +static constexpr unsigned hashSaltSize  { 48 };
> 
> Nit: extra space before the "{".

Right.

Thanks for the review.

[Alejandro G. Castro]

(In reply to youenn fablet from comment #5)
> Comment on attachment 352568 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352568&action=review
> 
> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:42
> > +static constexpr unsigned deviceIdHashSaltStorageVersion { 2 };
> 
> How about starting with 1?
> 

Right, just a leftover.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:88
> > +                continue;
> 
> What is done for NetworkCache is to create a folder with the version as a
> name.
> This allows to more easily migrate data from one version to another if needs
> be.
> 

Rigth, JFTR I was doing what it is done for statistics. Anyway after the meeting we had in the TPAC I'll reimplement usind directories.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:123
> > +    m_queue->dispatch([this, protectedThis = makeRef(*this), &hashSaltForOrigin]() mutable {
> 
> If you take a ref here, you need to ensure to deref it in the same thread.
> I believe this can be handled more easily by making DeviceIdHashSaltStorage
> ThreadSafeRefCounted< DeviceIdHashSaltStorage, WTF::DestructionThread::Main>
> 
> Probably need to capture hashSaltForOrigin by value.
> 

Right.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:124
> > +        writeEncoderToDisk(createEncoderFromData(hashSaltForOrigin), FileSystem::pathByAppendingComponent(m_deviceIdHashSaltStorageDirectory, hashSaltForOrigin.deviceIdHashSalt.utf8().data()));
> 
> s/writeEncoderToDisk/writeToDisk/ maybe?
> 

Right, I think the style is inspired in the statistics persistency.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:153
> > +        auto newHashSaltForOrigin = std::make_unique<HashSaltForOrigin>(documentOrigin.data().isolatedCopy(), WTFMove(deviceIdHashSalt));
> 
> Why should we isolatedCopy documentOrigin.data().
> It might be best to do that in storeHashSaltToDisk, even though this is less
> efficient, this will be more robust.
> Otherwise, we could inline storeHashSaltToDisk here.
> 

Main reason for the isolatedCopy is that we are creating a new structure, but now reviewing the code I can see we are copying the structure already in the constructor so I think it is safe just to pass the original origin.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:180
> > +    m_queue->dispatch([this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)]() mutable {
> 
> Why do we need to go to the background thread to query
> m_deviceIdHashSaltForOrigins?
> Can we try querying/modifying it in the main thread?
> 

Those methods are used in the WebsiteDataStore, and this is how the all the implementations work for retrieve/modify, I guess it is for performance reasons because they could be a lot to get and modify.

> One problem might be that at start-up time, we will need to read from disk
> and then load the data.
> One solution would be to always pass through an initialize method that takes
> a completion handler.
> The initialize method would be responsible to load the data from the
> background thread and store the callbacks until this is done.
> Once done, call the callbacks so that they can query/modify the data.
> 
> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:184
> > +            origins.add(hashSaltForOrigin.value->documentOrigin);
> 
> If we go this way, we probably need to isolate origin values.
> 

Right.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:192
> > +bool DeviceIdHashSaltStorage::deleteHashSaltFromDiskIfNeeded(HashSaltForOrigin& hashSaltForOrigin, bool needsRemoval)
> 
> Can we try having this method return void?
> 

Right, I can add the extra variable in the two methods and call this one before the return if the code is clearer.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:206
> > +    Vector<SecurityOriginData> copiedOrigins = origins;
> 
> Ditto for isolated copies.

Right

> Can we try just writing:
> m_deviceIdHashSaltForOrigins.removeIf([this, &copiedOrigins](auto&
> keyAndValue) {
>             return
> this->deleteHashSaltFromDiskIfNeeded(*keyAndValue.value.get(),
> copiedOrigins.contains(keyAndValue.value->documentOrigin));
> });

Not sure about the difference with the current code, anyway, removing the boolean in the delete function will create 3 lines instead of that one.

Thanks for the review, I'll try to modify the patch.

[Alejandro G. Castro]

Created attachment 355092 [details]
Patch

[Alejandro G. Castro]

Created attachment 355097 [details]
Patch

[Alejandro G. Castro]

Created attachment 355099 [details]
Patch

[Alejandro G. Castro]

Created attachment 355103 [details]
Patch

[Alejandro G. Castro]

Created attachment 355260 [details]
Patch

[Alejandro G. Castro]

The compilation errors look like unified build issues ...

[Alejandro G. Castro]

Created attachment 355268 [details]
Patch

[Alejandro G. Castro]

Created attachment 355276 [details]
Patch

[youenn fablet]

Comment on attachment 355276 [details]
Patch

Some comments inline.
To simplify things, I think we should try to:
- Get/set the origin map in main thread only
- Go to background thread for file reading/writing

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

> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp:736
> +        types = static_cast<WebKitWebsiteDataTypes>(types | WEBKIT_WEBSITE_DATA_DEVICE_ID_HASH_SALT);

Looking at WebsiteDataStore::removeData, we are probably already doing deviceID removal when deleting cookies.
Do we still need this code?

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:62
> +    m_queue->dispatch([this, protectedThis = makeRef(*this)]() mutable {

It might be good to add some release logging if we fail storing persistently data.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:98
> +            setDeviceIdHashSaltForOrigin(securityOriginData.value(), WTFMove(deviceIdHashSalt), WallTime::fromRawSeconds(lastTimeUsed));

I think, it is simpler to have the HashMap be modified in the main thread.
That way, once loaded, we just have to get/set it in the main thread, then in case of set, post a background task.
I would thus tend to create a HashMap in the background, then go to the main thread to set it to the DeviceIdHashSaltStorage instance.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:111
> +void DeviceIdHashSaltStorage::storeHashSaltToDisk(HashSaltForOrigin& hashSaltForOrigin)

could be const&?

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:113
> +    m_queue->dispatch([this, protectedThis = makeRef(*this), hashSaltForOrigin]() mutable {

This hashSaltForOrigin should be isolatedCopied.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:114
> +        writeToDisk(createEncoderFromData(hashSaltForOrigin), FileSystem::pathByAppendingComponent(m_deviceIdHashSaltStorageDirectory, hashSaltForOrigin.deviceIdHashSalt.utf8().data()));

m_deviceIdHashSaltStorageDirectory could be changed at runtime in main thread, which would create some issues.
Since it is const, this is probably ok.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:120
> +    auto& deviceIdHashSalt = m_deviceIdHashSaltForOrigins.ensure(documentOrigin.toRawString(), [this, &documentOrigin] () {

This method is called from main thread but it is modified in background thread above.
I would tend to add a completion handler to this one in case we call this method and the persistent data is not fully loaded.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:131
> +        auto newHashSaltForOrigin = std::make_unique<HashSaltForOrigin>(documentOrigin.data().isolatedCopy(), WTFMove(deviceIdHashSalt));

I do not think we need this isolatedCopy here.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:147
> +        return std::make_unique<HashSaltForOrigin>(documentOrigin.isolatedCopy(), WTFMove(deviceIdHashSalt));

s/.isolatedCopy()//

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:153
> +        deviceIdHashSaltForOrigin.iterator->value->deviceIdHashSalt = WTFMove(deviceIdHashSalt);

It is not very clear whether it is called from a background thread or not.
I would remove that function and inline it wherever needed, maybe make it a free function if that helps.
Or add ASSERT(!RunLoop::isMain());

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:167
>      });

I would tend to change the model here so that we do not have this double hop.
Something like:
- Check whether we are loaded.
- If we are loaded, directly use m_deviceIdHashSaltForOrigins (which would be only accessed in the main thread).
- If we are not loaded, store the completion handler and execute it when we finished loading.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:170
> +void DeviceIdHashSaltStorage::deleteHashSaltFromDiskIfNeeded(HashSaltForOrigin& hashSaltForOrigin)

const&?

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:189
> +        RunLoop::main().dispatch(WTFMove(completionHandler));

I think we can do the following here:
- Remove the necessary fields from m_deviceIdHashSaltForOrigins.
- Schedule a task on the background thread to remove the necessary files.

> Source/WebKit/UIProcess/PersistencyUtils.cpp:40
> +    ASSERT(!RunLoop::isMain());

I wonder whether this file should be in WebCore, closer to KeyedDecoder?

> Source/WebKit/UIProcess/PersistencyUtils.cpp:66
> +    return KeyedDecoder::decoder(reinterpret_cast<const uint8_t*>(buffer.data()), buffer.size());

buffer will be freed after exiting this function so KeyedDecoder is probably copying buffer memory.
This is preexisting code, so maybe we should add a FIXME.
In the future, we could pass buffer.data() and buffer.size(), pass a Vector<uint8_t>&&.

> Source/WebKit/UIProcess/PersistencyUtils.cpp:69
> +void writeToDisk(std::unique_ptr<KeyedEncoder>&& encoder, String&& path)

KeyedEncoder& and const String&?
Or instead of KeyedEncoder, should we pass rawData directly?

[Alejandro G. Castro]

Thanks for the review!

(In reply to youenn fablet from comment #16)
> Comment on attachment 355276 [details]
> Patch
> 
> Some comments inline.
> To simplify things, I think we should try to:
> - Get/set the origin map in main thread only
> - Go to background thread for file reading/writing
> 

Right. Basically the main get/set method deviceIdHashSaltForOrigin is in the main thread, the other methods are get/set to disk, and get/delete for the WebsiteDataStore also in a background thread. What do you think about using always the background thread to access the origins map? That way we should be safe and the structure would be similar to other classes sending the origins to the WebsiteDataStore.

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355276&action=review
> 
> > Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp:736
> > +        types = static_cast<WebKitWebsiteDataTypes>(types | WEBKIT_WEBSITE_DATA_DEVICE_ID_HASH_SALT);
> 
> Looking at WebsiteDataStore::removeData, we are probably already doing
> deviceID removal when deleting cookies.
> Do we still need this code?
> 

Yep, at least in our case if the type is not listed the remove does not happen.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:62
> > +    m_queue->dispatch([this, protectedThis = makeRef(*this)]() mutable {
> 
> It might be good to add some release logging if we fail storing persistently
> data.
> 

There is one in the PersistencyUtils.cpp already that does that.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:98
> > +            setDeviceIdHashSaltForOrigin(securityOriginData.value(), WTFMove(deviceIdHashSalt), WallTime::fromRawSeconds(lastTimeUsed));
> 
> I think, it is simpler to have the HashMap be modified in the main thread.
> That way, once loaded, we just have to get/set it in the main thread, then
> in case of set, post a background task.
> I would thus tend to create a HashMap in the background, then go to the main
> thread to set it to the DeviceIdHashSaltStorage instance.
> 

Right, check the above comment.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:111
> > +void DeviceIdHashSaltStorage::storeHashSaltToDisk(HashSaltForOrigin& hashSaltForOrigin)
> 
> could be const&?
> 

Right.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:113
> > +    m_queue->dispatch([this, protectedThis = makeRef(*this), hashSaltForOrigin]() mutable {
> 
> This hashSaltForOrigin should be isolatedCopied.
> 

Right.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:114
> > +        writeToDisk(createEncoderFromData(hashSaltForOrigin), FileSystem::pathByAppendingComponent(m_deviceIdHashSaltStorageDirectory, hashSaltForOrigin.deviceIdHashSalt.utf8().data()));
> 
> m_deviceIdHashSaltStorageDirectory could be changed at runtime in main
> thread, which would create some issues.
> Since it is const, this is probably ok.
> 

Right, the directory should not change, we could get a copy of it to make it more robust, maybe too defensive?

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:120
> > +    auto& deviceIdHashSalt = m_deviceIdHashSaltForOrigins.ensure(documentOrigin.toRawString(), [this, &documentOrigin] () {
> 
> This method is called from main thread but it is modified in background
> thread above.
> I would tend to add a completion handler to this one in case we call this
> method and the persistent data is not fully loaded.
> 

Right, check the comments above.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:131
> > +        auto newHashSaltForOrigin = std::make_unique<HashSaltForOrigin>(documentOrigin.data().isolatedCopy(), WTFMove(deviceIdHashSalt));
> 
> I do not think we need this isolatedCopy here.
> 

We need a copy of the SecurityOriginsData for the new object.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:147
> > +        return std::make_unique<HashSaltForOrigin>(documentOrigin.isolatedCopy(), WTFMove(deviceIdHashSalt));
> 
> s/.isolatedCopy()//
> 

Same here, we are creating the new object with new SecurityOriginData objects, in this case we can Move the objects from the disk load.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:153
> > +        deviceIdHashSaltForOrigin.iterator->value->deviceIdHashSalt = WTFMove(deviceIdHashSalt);
> 
> It is not very clear whether it is called from a background thread or not.
> I would remove that function and inline it wherever needed, maybe make it a
> free function if that helps.
> Or add ASSERT(!RunLoop::isMain());
> 

Right, yeah, it is the load from disk in the background thread, I can move the code there.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:167
> >      });
> 
> I would tend to change the model here so that we do not have this double hop.
> Something like:
> - Check whether we are loaded.
> - If we are loaded, directly use m_deviceIdHashSaltForOrigins (which would
> be only accessed in the main thread).
> - If we are not loaded, store the completion handler and execute it when we
> finished loading.
> 

Right, I think your proposal makes sense and we should try it, but I'm not sure if we should change this way of implementing the callbackAggregator interfaces just for this one or create a new bug that modifies every implemenation that is doing basically the copy to use in the main thread.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:170
> > +void DeviceIdHashSaltStorage::deleteHashSaltFromDiskIfNeeded(HashSaltForOrigin& hashSaltForOrigin)
> 
> const&?
> 

Right.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:189
> > +        RunLoop::main().dispatch(WTFMove(completionHandler));
> 
> I think we can do the following here:
> - Remove the necessary fields from m_deviceIdHashSaltForOrigins.
> - Schedule a task on the background thread to remove the necessary files.
> 

Right, I can try that but it seems all the other WebsiteDataStores are basically using the background thread to handle the origins variable. What do you think about moving all the hashmap handling to the background thread.

> > Source/WebKit/UIProcess/PersistencyUtils.cpp:40
> > +    ASSERT(!RunLoop::isMain());
> 
> I wonder whether this file should be in WebCore, closer to KeyedDecoder?
>

We could try to propose a patch to move it and get some feedback about it.

> > Source/WebKit/UIProcess/PersistencyUtils.cpp:66
> > +    return KeyedDecoder::decoder(reinterpret_cast<const uint8_t*>(buffer.data()), buffer.size());
> 
> buffer will be freed after exiting this function so KeyedDecoder is probably
> copying buffer memory.
> This is preexisting code, so maybe we should add a FIXME.
> In the future, we could pass buffer.data() and buffer.size(), pass a
> Vector<uint8_t>&&.
> 

Right, we could add a bug for that.

> > Source/WebKit/UIProcess/PersistencyUtils.cpp:69
> > +void writeToDisk(std::unique_ptr<KeyedEncoder>&& encoder, String&& path)
> 
> KeyedEncoder& and const String&?
> Or instead of KeyedEncoder, should we pass rawData directly?

Right, I can try to adapt all the code calling it.

[Alejandro G. Castro]

Created attachment 357568 [details]
Patch

[Alejandro G. Castro]

After talking in more detail with Youenn we decided to move the get/set to the main thread as proposed. In the last patch just the disk operations happen in the background thread and we try to control if that thread is done when receiving the get operations.

Thanks everyone for the comments.

[youenn fablet]

Comment on attachment 357568 [details]
Patch

Seems mostly good to me.
Some improvement suggestions below.

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

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:59
> +    }

You might be able to do:
auto isolatedOrigins = crossThreadCopy(m_deviceIdHashSaltForOrigins).
Might inline in RunLoop::main().dispatch call as well.

completeHandler could be renamed,  maybe to dispatchOriginsToMainThread?

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:71
> +    loadStorageFromDisk([this, protectedThis = makeRef(*this)] (HashMap<String, std::unique_ptr<HashSaltForOrigin>>&& deviceIdHashSaltForOrigins) {

s/HashMap<String, std::unique_ptr<HashSaltForOrigin>>/auto

Since this is file disk, there might be some errors, in which case, the HashMap could sometimes be replaced by an error.
Could use Expected<HashMap, Error> as the result passed to the lambda.
Or just log the errors for now and continue processing.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:78
> +                completeHandler(WTFMove(completionHandler));

Maybe completeHandler execution will trigger m_pendingGetCompletionHandler changes.
Usually, we do:
auto handlers = WTFMove(m_pendingGetCompletionHandler);
for (auto& handler : handlers)
    ...
Since we are in the main loop, do we need to dispatch on the run loop for its handler?

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:85
> +                completeDeviceIdHashSaltForOriginCall(WTFMove(pendingCall.documentOrigin), WTFMove(pendingCall.parentOrigin), WTFMove(pendingCall.completionHandler));

Ditto here.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:138
> +                continue;

We should probably log errors for each case.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:150
> +            auto deviceIdHashSaltForOrigin = deviceIdHashSaltForOrigins.ensure(origins, [securityOriginData = WTFMove(securityOriginData), parentSecurityOriginData = WTFMove(parentSecurityOriginData), deviceIdHashSalt = WTFMove(deviceIdHashSalt)] () mutable {

Should we use add instead of ensure?
And if there is already a value, we could log an error.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:154
> +            deviceIdHashSaltForOrigin.iterator->value->lastTimeUsed = WallTime::fromRawSeconds(lastTimeUsed);

Do we need to set lastTimeUsed here? Can it be done when creating the HashSaltForOrigin.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:157
> +                deviceIdHashSaltForOrigin.iterator->value->deviceIdHashSalt = WTFMove(deviceIdHashSalt);

Similarly to previous comment, isn't it an error case?

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:166
> +std::unique_ptr<KeyedEncoder> DeviceIdHashSaltStorage::createEncoderFromData(const HashSaltForOrigin& hashSaltForOrigin) const

I would have a mirror function for decoding so that it is easy to check they are symmetric.
It could be a free function as well.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:179
> +        writeToDisk(WTFMove(encoder), FileSystem::pathByAppendingComponent(m_deviceIdHashSaltStorageDirectory, deviceIdHashSalt.utf8().data()));

I would tend to isolateCopy hashSaltForOrigin and pass it in dispatch lambda.
That way, we create the encoder in the background thread.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:216
> +        m_deviceIdHashSaltForOriginPendingCalls.append(WTFMove(newCall));

Can we try removing m_deviceIdHashSaltForOriginPendingCalls?
That way, we have an ordered list of completion handlers.

Something like: 

m_pendingGetCompletionHandler.append([documentOrigin = documentOrigin.data().isolatedCopy(), ..., completionHandler  = WTFMove(completionHandler](auto&& origins) {
    completeDeviceIdHashSaltForOriginCall(documentOrigin,..., WTFMove(completionHandler));
});

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:220
> +    completeDeviceIdHashSaltForOriginCall(documentOrigin.data().isolatedCopy(), parentOrigin.data().isolatedCopy(), WTFMove(completionHandler));

I don't think we need an isolatedCopy here.
If we need a SecurityOriginData&&, we can use the copy constructor.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:237
> +    m_queue->dispatch([this, protectedThis = makeRef(*this), deviceIdHashSalt = hashSaltForOrigin.deviceIdHashSalt]() mutable {

Should deviceIdHashSalt be isolatedCopy?
Should we assert RunLoop::isMain()?

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:250
> +        auto needsRemoval = origins.contains(keyAndValue.value->documentOrigin) || origins.contains(keyAndValue.value->parentOrigin);

Could use bool

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:187
> +    m_page.websiteDataStore().deviceIdHashSaltStorage()->deviceIdHashSaltForOrigin(request->userMediaDocumentSecurityOrigin(), request->topLevelDocumentSecurityOrigin(), [this, userMediaID, audioDevice = WTFMove(audioDevice), videoDevice = WTFMove(videoDevice), localRequest = request.copyRef()] (String&& deviceIDHashSalt) mutable {

Are we sure 'this' is valid in the lambda?
If not, we probably want to make UserMediaPermissionRequestManagerProxy a CanMakeWeakPtr<UserMediaPermissionRequestManagerProxy> and capture weakThis = createWeakPtr(this) in the lambda to check it.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:394
> +            RealtimeMediaSourceCenter::singleton().validateRequestConstraints(WTFMove(validHandler), WTFMove(invalidHandler), WTFMove(localUserRequest), WTFMove(deviceIDHashSalt));

It seems we do not need to capture 'this' there.

[Alejandro G. Castro]

Thanks for the review!

(In reply to youenn fablet from comment #20)
> Comment on attachment 357568 [details]
> Patch
> 
> Seems mostly good to me.
> Some improvement suggestions below.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357568&action=review
> 
> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:59
> > +    }
> 
> You might be able to do:
> auto isolatedOrigins = crossThreadCopy(m_deviceIdHashSaltForOrigins).
> Might inline in RunLoop::main().dispatch call as well.
> 

Yep but we still would need to create the HashSet, we are not returning the original HashMap. So I think this way we avoid the copy of all the object and later create the HashSet.

> completeHandler could be renamed,  maybe to dispatchOriginsToMainThread?
> 

Right.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:71
> > +    loadStorageFromDisk([this, protectedThis = makeRef(*this)] (HashMap<String, std::unique_ptr<HashSaltForOrigin>>&& deviceIdHashSaltForOrigins) {
> 
> s/HashMap<String, std::unique_ptr<HashSaltForOrigin>>/auto
> 

Right.

> Since this is file disk, there might be some errors, in which case, the
> HashMap could sometimes be replaced by an error.
> Could use Expected<HashMap, Error> as the result passed to the lambda.
> Or just log the errors for now and continue processing.
> 

Right. Good point, I'll add specific errors for the release log for each problem.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:78
> > +                completeHandler(WTFMove(completionHandler));
> 
> Maybe completeHandler execution will trigger m_pendingGetCompletionHandler
> changes.
> Usually, we do:
> auto handlers = WTFMove(m_pendingGetCompletionHandler);
> for (auto& handler : handlers)
>     ...

We set m_isLoaded before completing the handlers so any call to the original function is not going change the pendingCompletionHandlers Vector.

> Since we are in the main loop, do we need to dispatch on the run loop for
> its handler?
> 

Right, we can directly resolve in both cases, this is a left over of the refactor, I'll rename to completePendingHandler and avoid the dispatch.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:85
> > +                completeDeviceIdHashSaltForOriginCall(WTFMove(pendingCall.documentOrigin), WTFMove(pendingCall.parentOrigin), WTFMove(pendingCall.completionHandler));
> 
> Ditto here.
> 

Yep, it is the same situation m_isLoaded already changed so the pending can not be modified.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:138
> > +                continue;
> 
> We should probably log errors for each case.
> 

Right, done as explained before.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:150
> > +            auto deviceIdHashSaltForOrigin = deviceIdHashSaltForOrigins.ensure(origins, [securityOriginData = WTFMove(securityOriginData), parentSecurityOriginData = WTFMove(parentSecurityOriginData), deviceIdHashSalt = WTFMove(deviceIdHashSalt)] () mutable {
> 
> Should we use add instead of ensure?
> And if there is already a value, we could log an error.
> 

Right, good point, I can ensure and log and error if it is not new.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:154
> > +            deviceIdHashSaltForOrigin.iterator->value->lastTimeUsed = WallTime::fromRawSeconds(lastTimeUsed);
> 
> Do we need to set lastTimeUsed here? Can it be done when creating the
> HashSaltForOrigin.
>

This is the lastTimeUsed we restored from the file, we need to restore it here.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:157
> > +                deviceIdHashSaltForOrigin.iterator->value->deviceIdHashSalt = WTFMove(deviceIdHashSalt);
> 
> Similarly to previous comment, isn't it an error case?
> 

Right, I'll add and error instead of replacing the one already set.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:166
> > +std::unique_ptr<KeyedEncoder> DeviceIdHashSaltStorage::createEncoderFromData(const HashSaltForOrigin& hashSaltForOrigin) const
> 
> I would have a mirror function for decoding so that it is easy to check they
> are symmetric.
> It could be a free function as well.
> 

Right, I'll add a function with that code.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:179
> > +        writeToDisk(WTFMove(encoder), FileSystem::pathByAppendingComponent(m_deviceIdHashSaltStorageDirectory, deviceIdHashSalt.utf8().data()));
> 
> I would tend to isolateCopy hashSaltForOrigin and pass it in dispatch lambda.
> That way, we create the encoder in the background thread.
> 

Right.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:216
> > +        m_deviceIdHashSaltForOriginPendingCalls.append(WTFMove(newCall));
> 
> Can we try removing m_deviceIdHashSaltForOriginPendingCalls?
> That way, we have an ordered list of completion handlers.
> 
> Something like: 
> 
> m_pendingGetCompletionHandler.append([documentOrigin =
> documentOrigin.data().isolatedCopy(), ..., completionHandler  =
> WTFMove(completionHandler](auto&& origins) {
>     completeDeviceIdHashSaltForOriginCall(documentOrigin,...,
> WTFMove(completionHandler));
> });
> 

The problem is that one has a parameter and the other does not, we would be creating the HashSet that is not going to be used in this case. I'm going to take the suggestion to use the lambda instead of the struct :-).

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:220
> > +    completeDeviceIdHashSaltForOriginCall(documentOrigin.data().isolatedCopy(), parentOrigin.data().isolatedCopy(), WTFMove(completionHandler));
> 
> I don't think we need an isolatedCopy here.
> If we need a SecurityOriginData&&, we can use the copy constructor.
> 

Right.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:237
> > +    m_queue->dispatch([this, protectedThis = makeRef(*this), deviceIdHashSalt = hashSaltForOrigin.deviceIdHashSalt]() mutable {
> 
> Should deviceIdHashSalt be isolatedCopy?

Right.

> Should we assert RunLoop::isMain()?
> 

It is not required because we added the assert to the methods calling this one and it is private.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:250
> > +        auto needsRemoval = origins.contains(keyAndValue.value->documentOrigin) || origins.contains(keyAndValue.value->parentOrigin);
> 
> Could use bool
> 

Right.

> > Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:187
> > +    m_page.websiteDataStore().deviceIdHashSaltStorage()->deviceIdHashSaltForOrigin(request->userMediaDocumentSecurityOrigin(), request->topLevelDocumentSecurityOrigin(), [this, userMediaID, audioDevice = WTFMove(audioDevice), videoDevice = WTFMove(videoDevice), localRequest = request.copyRef()] (String&& deviceIDHashSalt) mutable {
> 
> Are we sure 'this' is valid in the lambda?
> If not, we probably want to make UserMediaPermissionRequestManagerProxy a
> CanMakeWeakPtr<UserMediaPermissionRequestManagerProxy> and capture weakThis
> = createWeakPtr(this) in the lambda to check it.
> 

I thought so, but I added later the pending calls and probably it is not safe. I'll add the control.

> > Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:394
> > +            RealtimeMediaSourceCenter::singleton().validateRequestConstraints(WTFMove(validHandler), WTFMove(invalidHandler), WTFMove(localUserRequest), WTFMove(deviceIDHashSalt));
> 
> It seems we do not need to capture 'this' there.

Right.

Thanks again for the comments!

[Alejandro G. Castro]

Created attachment 357672 [details]
Patch

[Alejandro G. Castro]

Created attachment 357673 [details]
Patch

[Alejandro G. Castro]

Created attachment 357674 [details]
Patch

[Alejandro G. Castro]

Created attachment 357675 [details]
Patch

[Alejandro G. Castro]

Created attachment 357677 [details]
Patch

[youenn fablet]

Comment on attachment 357677 [details]
Patch

LGTM with proposed changes below.
Unit tests covering private browsing might be good too, now or as a follow-up.

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

> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp:733
> +    // We have to remove the hash salts when cookies are removed.

I would add a FIXME comment then, since this should be dealt with in platform generic code.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:58
> +        origins.add(hashSaltForOrigin.value->parentOrigin.isolatedCopy());

Do we need to isolate copy these?
It seems there is no threading issue, it is all main thread.
Maybe we should add an ASSERT here for main thread.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:64
> +DeviceIdHashSaltStorage::DeviceIdHashSaltStorage(const String& deviceIdHashSaltStorageDirectory, bool isPersistent)

We probably do not need isPersistent.
If deviceIdHashSaltStorageDirectory, isPersistent is false and we do not need to compute m_deviceIdHashSaltStorageDirectory.
The persistency check can be done as m_deviceIdHashSaltStorageDirectory.isEmpty().

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:69
> +    loadStorageFromDisk([this, protectedThis = makeRef(*this)] (auto deviceIdHashSaltForOrigins) {

s/auto/auto&&/ might be better.
No need for protectedThis here since loadStorageFromDisk is already refing 'this'.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:79
> +        }

I would write it this way:
auto m_pendingGetCompletionHandlers = WTFMove(m_pendingGetCompletionHandler);
for (auto& completionHandler : m_pendingGetCompletionHandler)
    this->completePendingHandler(WTFMove(completionHandler));
No need for clear.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:86
> +        }

Ditto here.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:157
> +std::unique_ptr<DeviceIdHashSaltStorage::HashSaltForOrigin> DeviceIdHashSaltStorage::getDataFromDecoder(KeyedDecoder* decoder, String&& deviceIdHashSalt) const

Can be free function.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:184
> +std::unique_ptr<KeyedEncoder> DeviceIdHashSaltStorage::createEncoderFromData(const HashSaltForOrigin& hashSaltForOrigin) const

Ditto.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:204
>      auto& deviceIdHashSalt = m_deviceIdHashSaltForOrigins.ensure(origins, [&documentOrigin, &parentOrigin] () {

It is fine as is.
I wonder though whether it would be clearer to do:
[documentOrigin = WTFMove(documentOrigin), parentOrigin= WTFMove(parentOrigin)]() mutable

That way, it is clearer that documentOrigin is moved at that point.
But this has a penalty cost as we would call the move constructor.
Let's keep it like this then, given the method is not long.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:223
> +        storeHashSaltToDisk(*deviceIdHashSalt.get());

I would tend to move the if check in storeHashSaltToDisk and make it exit early if m_isPersistent is false.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:247
> +        m_pendingGetCompletionHandler.append(WTFMove(completionHandler));

I think we can make m_pendingGetCompletionHandler and this completionHandler be a CompletionHandler<void()>.
This way, m_pendingGetCompletionHandler and m_pendingDeviceIdHashSaltForOrigin can be merged as one Vector.

This can be done by writing:
m_pendingGetCompletionHandlers.append([this, completionHandler  = WTFMove(completionHandler)] {
    this->completePendingHandler(WTFMove(completionHandler));
});

Note that we probably need to iterate over m_pendingGetCompletionHandlers in DeviceIdHashSaltStorage destructor.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:285
> +            this->deleteHashSaltFromDisk(*keyAndValue.value.get());

If none persistent, this call is not needed. Let's have deleteHashSaltFromDisk exit early if !m_isPersistent.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.h:42
> +class UserMediaPermissionRequestManagerProxy : public CanMakeWeakPtr<UserMediaPermissionRequestManagerProxy> {

I wonder whether it can be a private CanMakeWeakPtr<>

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:95
> +    , m_deviceIdHashSaltStorage(DeviceIdHashSaltStorage::create(API::WebsiteDataStore::defaultDeviceIdHashSaltsStorageDirectory(), isPersistent()))

Should we use m_configuration->deviceIdHashSaltsStorageDirectory() instead of the default one?

[Alejandro G. Castro]

Thanks again for the review.

(In reply to youenn fablet from comment #27)
> Comment on attachment 357677 [details]
> Patch
> 
> LGTM with proposed changes below.
> Unit tests covering private browsing might be good too, now or as a
> follow-up.
> 

I'll add it.

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357677&action=review
> 
> > Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp:733
> > +    // We have to remove the hash salts when cookies are removed.
> 
> I would add a FIXME comment then, since this should be dealt with in
> platform generic code.
> 

I agree, in the original patch we did it here because we were the only ones supporting this but I'm not sure if there was another reason. I'll try to move it.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:58
> > +        origins.add(hashSaltForOrigin.value->parentOrigin.isolatedCopy());
> 
> Do we need to isolate copy these?
> It seems there is no threading issue, it is all main thread.
> Maybe we should add an ASSERT here for main thread.
> 

The assert is in the method calling this one, I can add another one here to make sure no one uses this one from other thread. Yep, the isolatedCopy was added before the last refactor of the patch in the background.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:64
> > +DeviceIdHashSaltStorage::DeviceIdHashSaltStorage(const String& deviceIdHashSaltStorageDirectory, bool isPersistent)
> 
> We probably do not need isPersistent.
> If deviceIdHashSaltStorageDirectory, isPersistent is false and we do not
> need to compute m_deviceIdHashSaltStorageDirectory.
> The persistency check can be done as
> m_deviceIdHashSaltStorageDirectory.isEmpty().
> 

Right, I can add this modification.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:69
> > +    loadStorageFromDisk([this, protectedThis = makeRef(*this)] (auto deviceIdHashSaltForOrigins) {
> 
> s/auto/auto&&/ might be better.
> No need for protectedThis here since loadStorageFromDisk is already refing
> 'this'.
> 

Hrm, but the completion handler of loadStorageFromDisk is executed in a different thread with a dispatch.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:79
> > +        }
> 
> I would write it this way:
> auto m_pendingGetCompletionHandlers = WTFMove(m_pendingGetCompletionHandler);
> for (auto& completionHandler : m_pendingGetCompletionHandler)
>     this->completePendingHandler(WTFMove(completionHandler));
> No need for clear.
> 

Right.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:86
> > +        }
> 
> Ditto here.
> 

Right.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:157
> > +std::unique_ptr<DeviceIdHashSaltStorage::HashSaltForOrigin> DeviceIdHashSaltStorage::getDataFromDecoder(KeyedDecoder* decoder, String&& deviceIdHashSalt) const
> 
> Can be free function.
> 

I'm afraid it can't unless we make the HashSaltForOrigin public because it is private.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:184
> > +std::unique_ptr<KeyedEncoder> DeviceIdHashSaltStorage::createEncoderFromData(const HashSaltForOrigin& hashSaltForOrigin) const
> 
> Ditto.
> 

Ditto.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:204
> >      auto& deviceIdHashSalt = m_deviceIdHashSaltForOrigins.ensure(origins, [&documentOrigin, &parentOrigin] () {
> 
> It is fine as is.
> I wonder though whether it would be clearer to do:
> [documentOrigin = WTFMove(documentOrigin), parentOrigin=
> WTFMove(parentOrigin)]() mutable
> 
> That way, it is clearer that documentOrigin is moved at that point.
> But this has a penalty cost as we would call the move constructor.
> Let's keep it like this then, given the method is not long.
> 

Right. I can do it, I like the way you are proposing, again this was because in the multiple refactors of the patch at some point they were copies.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:223
> > +        storeHashSaltToDisk(*deviceIdHashSalt.get());
> 
> I would tend to move the if check in storeHashSaltToDisk and make it exit
> early if m_isPersistent is false.
> 

Right.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:247
> > +        m_pendingGetCompletionHandler.append(WTFMove(completionHandler));
> 
> I think we can make m_pendingGetCompletionHandler and this completionHandler
> be a CompletionHandler<void()>.
> This way, m_pendingGetCompletionHandler and
> m_pendingDeviceIdHashSaltForOrigin can be merged as one Vector.
> 
> This can be done by writing:
> m_pendingGetCompletionHandlers.append([this, completionHandler  =
> WTFMove(completionHandler)] {
>     this->completePendingHandler(WTFMove(completionHandler));
> });
> 
> Note that we probably need to iterate over m_pendingGetCompletionHandlers in
> DeviceIdHashSaltStorage destructor.
> 

Right, I'll merge both.

> > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:285
> > +            this->deleteHashSaltFromDisk(*keyAndValue.value.get());
> 
> If none persistent, this call is not needed. Let's have
> deleteHashSaltFromDisk exit early if !m_isPersistent.
> 

Right.

> > Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.h:42
> > +class UserMediaPermissionRequestManagerProxy : public CanMakeWeakPtr<UserMediaPermissionRequestManagerProxy> {
> 
> I wonder whether it can be a private CanMakeWeakPtr<>
> 

Just confirmed it can't, apparently the factory needs to get to use the API.

> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:95
> > +    , m_deviceIdHashSaltStorage(DeviceIdHashSaltStorage::create(API::WebsiteDataStore::defaultDeviceIdHashSaltsStorageDirectory(), isPersistent()))
> 
> Should we use m_configuration->deviceIdHashSaltsStorageDirectory() instead
> of the default one?

Right.

Thanks again for the review.

[Alejandro G. Castro]

(In reply to Alejandro G. Castro from comment #28)
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=357677&action=review
> > 
> > > Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp:733
> > > +    // We have to remove the hash salts when cookies are removed.
> > 
> > I would add a FIXME comment then, since this should be dealt with in
> > platform generic code.
> > 
> 
> I agree, in the original patch we did it here because we were the only ones
> supporting this but I'm not sure if there was another reason. I'll try to
> move it.
> 

Just recalled we need to do this here because we are removing the types from the list if the website does not contain that data, for instance cookies :-). This is specific to the glib implementation.

[Alejandro G. Castro]

(In reply to youenn fablet from comment #20)
> Maybe completeHandler execution will trigger m_pendingGetCompletionHandler
> changes.
> Usually, we do:
> auto handlers = WTFMove(m_pendingGetCompletionHandler);
> for (auto& handler : handlers)
>     ...
> Since we are in the main loop, do we need to dispatch on the run loop for
> its handler?
> 

I had to rollback also this proposal, I recalled I initially did the same test when creating the patch. The WebsiteDataStore does not work if we call directly to the completion handler, not sure if it is by design. Basically the callIfNeeded consumes the apply completionHandler the first time and we can not call it the second time. I think it is a bug in the design of the WebsiteDataStore, because I agree with youenn's proposal to avoid the dispatch, just that we are the first ones we try to run everything in the main thread without dispatching. We should check that in a different bug and probably move this back to call difectly the completionHandler.

Anyway, I'm uploading for a next review considering these changes.

[Alejandro G. Castro]

Created attachment 357949 [details]
Patch

[youenn fablet]

Comment on attachment 357949 [details]
Patch

LGTM.
Some improvements below that can either be done now or as follow-ups.

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

> Source/WebKit/UIProcess/API/APIProcessPoolConfiguration.h:100
> +    void setDeviceIdHashSaltsStorageDirectory(const WTF::String& directory) { m_deviceIdHashSaltsStorageDirectory = directory; }

Could be String&&

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:63
> +    RunLoop::main().dispatch([origins = WTFMove(origins), completionHandler = WTFMove(completionHandler)]() mutable {

Let's add a FIXME then.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:72
> +    if (m_deviceIdHashSaltStorageDirectory.isEmpty()) {

In the future, I would tend to lazily load the storage when we first use the device storage.
Can we add a FIXME?

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:77
> +    loadStorageFromDisk([this, protectedThis = makeRef(*this)] (auto&& deviceIdHashSaltForOrigins) {

Agreed that we currently need to protectedThis here.
The alternative would be to just call loadStorageFromDisk without any lambda.
In loadStorageFromDisk, we would take a ref when going to background thread and moving the ref when going back to main thread.
When going back to main thread, we could then call the lambda code as a method of 68DeviceIdHashSaltStorage for instance.

Not needed to land this change though.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:110
> +{

Maybe ASSERT(!m_deviceIdHashSaltStorageDirectory.isEmpty());

Should we also ASSERT(!m_isLoaded), or return early in that case as well.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:183
> +    hashSaltForOrigin->lastTimeUsed = WallTime::fromRawSeconds(lastTimeUsed);

We could change HashSaltForOrigin constructor so that lastTimeUsed is given in the constructor
That way, we could write it:
return std::make_unique<HashSaltForOrigin>(WTFMove(securityOriginData.value()), WTFMove(parentSecurityOriginData.value()), WTFMove(deviceIdHashSalt), WallTime::fromRawSeconds(lastTimeUsed));

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:224
> +        return newHashSaltForOrigin;

Write it as:
return std::make_unique<HashSaltForOrigin>(WTFMove(documentOrigin), WTFMove(parentOrigin), WTFMove(deviceIdHashSalt));

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:239
> +        m_pendingCompletionHandlers.append([this, documentOrigin = documentOrigin.data().isolatedCopy(), parentOrigin = parentOrigin.data().isolatedCopy(), completionHandler = WTFMove(completionHandler)]() mutable {

No need to isolatedCopy these, the pending completion handler should always be destroyed in main thread anyway.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:264
> +    m_queue->dispatch([this, protectedThis = makeRef(*this), deviceIdHashSalt = hashSaltForOrigin.deviceIdHashSalt.isolatedCopy()]() mutable {

To not require refing protectedThis, we could compute fileFullPath in the lambda.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:278
> +        if (m_deviceIdHashSaltStorageDirectory.isEmpty())

I would move this check inside deleteHashSaltFromDisk.
That would make it clearer.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:62
> +            isolatedCopy.lastTimeUsed = lastTimeUsed;

Adding lastTimeUsed in constructor would ease writing this line.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:72
> +    DeviceIdHashSaltStorage(const String& deviceIdHashSaltStorageDirectory);

Add explicit.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:75
> +    void deleteHashSaltFromDisk(const HashSaltForOrigin&);

I would make it consistent for all three methods and return early whenever m_deviceIdHashSaltStorageDirectory is empty.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:77
> +    std::unique_ptr<HashSaltForOrigin> getDataFromDecoder(WebCore::KeyedDecoder*, String&& deviceIdHashSalt) const;

Should be KeyedDecoder& instead of KeyedDecoder* probably.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:79
> +    void completeDeviceIdHashSaltForOriginCall(WebCore::SecurityOriginData&& documentOrigin, WebCore::SecurityOriginData&& parentOrigin, CompletionHandler<void(String&&)>&&);

Maybe we should make these two names (completePendingHandler and completeDeviceIdHashSaltForOriginCall) consistent.
Something like:
completePendingHandler -> completeOriginsRetrievalHandler
completeDeviceIdHashSaltForOriginCall -> completeDeviceIdHashSaltRetrievalHandler

[EWS Watchlist]

Comment on attachment 357949 [details]
Patch

Attachment 357949 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10508075

New failing tests:
webgpu/simple-triangle-strip.html

[EWS Watchlist]

Created attachment 357956 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[WebKit Commit Bot]

Comment on attachment 357949 [details]
Patch

Clearing flags on attachment: 357949

Committed r239513: <https://trac.webkit.org/changeset/239513>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46906016>