Network process crashes in WebKit::StorageManagerSet::deleteSessionStorageForOrigins

[Sihui Liu]

...

[Sihui Liu]

rdar://problem/64853929

[Sihui Liu]

Created attachment 403706 [details]
Patch

[John Wilander]

Comment on attachment 403706 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1929
> +                    return;

I don't think early return is correct here. The fact that the storage manager set doesn't contain the session ID doesn't mean there aren't other means of storage to delete. The function is for deleting all existing and future website data types.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1939
> +                    return;

Ditto.

[Sihui Liu]

Comment on attachment 403706 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1929
>> +                    return;
> 
> I don't think early return is correct here. The fact that the storage manager set doesn't contain the session ID doesn't mean there aren't other means of storage to delete. The function is for deleting all existing and future website data types.

This early return is in the callback function of getSessionStorageOrigins, so this change only affects deletion of session storage..  DiskCache below does the same thing..

[Chris Dumez]

Comment on attachment 403706 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1932
>                  m_storageManagerSet->deleteSessionStorageForOrigins(sessionID, originsToDelete, [callbackAggregator] { });

Why are we not doing the sessionID check inside deleteSessionStorageForOrigins() ?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1942
>                  m_storageManagerSet->deleteLocalStorageForOrigins(sessionID, originsToDelete, [callbackAggregator] { });

ditto.

[Sihui Liu]

Comment on attachment 403706 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1932
>>                  m_storageManagerSet->deleteSessionStorageForOrigins(sessionID, originsToDelete, [callbackAggregator] { });
> 
> Why are we not doing the sessionID check inside deleteSessionStorageForOrigins() ?

Currently all functions in StorageManagerSet asserts session exists when they are called, so it looks more consistent to not change there... And in NetworkProcess, we normally get/delete websitedata only when session exists.

[Chris Dumez]

Comment on attachment 403706 [details]
Patch

Ok

[EWS]

Committed r264036: <https://trac.webkit.org/changeset/264036>

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

[Sihui Liu]

<rdar://problem/64750825>