Bug 197636 – Move Web Storage to Network Process

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

To facilitate quota computation and database file management during process suspension.

[Sihui Liu]

Created attachment 369206 [details]
Patch

[Sihui Liu]

Created attachment 369210 [details]
Patch

[Sihui Liu]

Created attachment 369215 [details]
Patch

[EWS Watchlist]

Comment on attachment 369215 [details]
Patch

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

New failing tests:
http/tests/adClickAttribution/store-disabled-in-ephemeral-session.html

[EWS Watchlist]

Created attachment 369220 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

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

[EWS Watchlist]

Comment on attachment 369215 [details]
Patch

Attachment 369215 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12118744

New failing tests:
http/tests/adClickAttribution/store-disabled-in-ephemeral-session.html
storage/domstorage/localstorage/private-browsing-affects-storage.html

[EWS Watchlist]

Created attachment 369223 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.4

[EWS Watchlist]

Comment on attachment 369215 [details]
Patch

Attachment 369215 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12120616

New failing tests:
legacy-animation-engine/compositing/reflections/load-video-in-reflection.html

[EWS Watchlist]

Created attachment 369252 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Alex Christensen]

Comment on attachment 369215 [details]
Patch

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

This is exciting.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2707
> +    auto storageManager = m_storageManagers.get(sessionID);

It looks like this needs to be null checked.  Probably others, too.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:4994
> -    [webView evaluateJavaScript:@"window.sessionStorage.setItem('b,'a')" completionHandler:^(id, NSError *) {
> +    [webView evaluateJavaScript:@"window.sessionStorage.setItem('b','a')" completionHandler:^(id, NSError *) {

🤦

[Sihui Liu]

Created attachment 369404 [details]
Patch

[Sihui Liu]

Created attachment 369737 [details]
Patch

[Sihui Liu]

Created attachment 369739 [details]
Patch

[youenn fablet]

Comment on attachment 369739 [details]
Patch

Looks good to have this in network process!
Some comments below.

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

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:892
> +void NetworkConnectionToWebProcess::webProcessSessionChanged(PAL::SessionID newSessionID, Vector<uint64_t> pages)

Vector<>&& or const Vector<>& probably.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1025
> +    auto storageManager = m_storageManagers.get(sessionID);

We can put storageManager in the NetworkSession which are already hashmaped by session ID.
This will make sure storageManager is cleared on destruction of session.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1029
> +    storageManager->getLocalStorageOrigins([domain, completionHandler = WTFMove(completionHandler)](HashSet<WebCore::SecurityOriginData>&& origins) mutable {

Could use auto&& here.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1032
> +            if (originDomain == domain) {

Can we call something like if (domain.matches(origin)) instead?
I am adding the same functionality in another patch.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1038
> +        completionHandler(false);

You could maybe write it as:
completionHandler(WTF::anyOf(origins, [&domain](auto& origin) { return domain.matches(origin) }))

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1538
> +static Vector<WebCore::SecurityOriginData> filterForRegistrableDomains(HashSet<WebCore::SecurityOriginData> origins, Vector<RegistrableDomain> domainsToDelete, HashSet<RegistrableDomain>& domainsDeleted)

Use const references?
Also, it might be better to not use out parameters if possible.
Can we return domainsDeleted as return value of the function?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2627
> +    ASSERT(m_storageManagers.contains(sessionID));

Since this is coming from IPC, we usually do not trust it.
We might want to return early if m_storageManagers.contains(sessionID) is false.

Ditto below.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2634
> +    });

What is the purpose of m_sessionByConnection.
Are we sure there is a one-to-one mapping from connection to sessionID?
I would have thought that a connection could support multiple sessionIDs.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2660
> +void NetworkProcess::webProcessSessionChanged(IPC::Connection& connection, PAL::SessionID newSessionID, Vector<uint64_t> pageIDs)

const Vector<>& or Vector<>&&

> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:2
> + * Copyright (C) 2008, 2009, 2010, 2013 Apple Inc. All rights reserved.

2019 as well?

> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:56
> +    , m_securityOrigin(securityOrigin)

Ideally we would pass a SecurityOriginData&&
If you are just moving the file, probably not worth changing this now to limit the size of the patch.

> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:86
> +{

Can we assert that we are not on the main thread?

> Source/WebKit/WebProcess/WebProcess.h:209
> +    StorageAreaMap* storageAreaMap(uint64_t identifier) const;

Is StorageAreaMap be able to recover if NetworkProcess is crashing.
If not, fixing this could be done in a follow-up if needed.

Ditto for removeWebPage and so on.
Is there a risk of the WebProcess being no longer synchronized with NetworkProcess?

[Sihui Liu]

Comment on attachment 369739 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:892
>> +void NetworkConnectionToWebProcess::webProcessSessionChanged(PAL::SessionID newSessionID, Vector<uint64_t> pages)
> 
> Vector<>&& or const Vector<>& probably.

Okay.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1025
>> +    auto storageManager = m_storageManagers.get(sessionID);
> 
> We can put storageManager in the NetworkSession which are already hashmaped by session ID.
> This will make sure storageManager is cleared on destruction of session.

Okay I will try moving this to NetworkSession.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1029
>> +    storageManager->getLocalStorageOrigins([domain, completionHandler = WTFMove(completionHandler)](HashSet<WebCore::SecurityOriginData>&& origins) mutable {
> 
> Could use auto&& here.

Okay.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1032
>> +            if (originDomain == domain) {
> 
> Can we call something like if (domain.matches(origin)) instead?
> I am adding the same functionality in another patch.

Okay, then I will use your newly added function later.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1038
>> +        completionHandler(false);
> 
> You could maybe write it as:
> completionHandler(WTF::anyOf(origins, [&domain](auto& origin) { return domain.matches(origin) }))

Nice.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1538
>> +static Vector<WebCore::SecurityOriginData> filterForRegistrableDomains(HashSet<WebCore::SecurityOriginData> origins, Vector<RegistrableDomain> domainsToDelete, HashSet<RegistrableDomain>& domainsDeleted)
> 
> Use const references?
> Also, it might be better to not use out parameters if possible.
> Can we return domainsDeleted as return value of the function?

Will use const references. 

Returning either Vector<SecurityOriginData> or HashSet<RegistrableDomain> would require second traverse of the collection to convert it into another type. I could remove the out parameter if you think it's better not use.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2627
>> +    ASSERT(m_storageManagers.contains(sessionID));
> 
> Since this is coming from IPC, we usually do not trust it.
> We might want to return early if m_storageManagers.contains(sessionID) is false.
> 
> Ditto below.

Okay.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2634
>> +    });
> 
> What is the purpose of m_sessionByConnection.
> Are we sure there is a one-to-one mapping from connection to sessionID?
> I would have thought that a connection could support multiple sessionIDs.

Yes, this one is tricky. 
In our previous implementation, we did only support one session per connection. See StorageManager::processWillOpenConnection: it dispatches the StorageManager message of connection to one StorageManager.
This patch tries to keep old behavior. We may change that if we start to support multiple sessions in one web process.

The purpose of m_sessionByConnection is to support setPrivateBrowsingEnabled for tests. setPrivateBrowsingEnabled can change session of a web process after connection to some StorageManager is established. If this happens, we should stop dispatching message to StorageManager with old sessionID.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2660
>> +void NetworkProcess::webProcessSessionChanged(IPC::Connection& connection, PAL::SessionID newSessionID, Vector<uint64_t> pageIDs)
> 
> const Vector<>& or Vector<>&&

Okay.

>> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:2
>> + * Copyright (C) 2008, 2009, 2010, 2013 Apple Inc. All rights reserved.
> 
> 2019 as well?

Sure.

>> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:56
>> +    , m_securityOrigin(securityOrigin)
> 
> Ideally we would pass a SecurityOriginData&&
> If you are just moving the file, probably not worth changing this now to limit the size of the patch.

That's true.

>> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:86
>> +{
> 
> Can we assert that we are not on the main thread?

Okay.

>> Source/WebKit/WebProcess/WebProcess.h:209
>> +    StorageAreaMap* storageAreaMap(uint64_t identifier) const;
> 
> Is StorageAreaMap be able to recover if NetworkProcess is crashing.
> If not, fixing this could be done in a follow-up if needed.
> 
> Ditto for removeWebPage and so on.
> Is there a risk of the WebProcess being no longer synchronized with NetworkProcess?

Right, this patch doesn't cover crash recovery. I should add it and upload later!

[youenn fablet]

> > What is the purpose of m_sessionByConnection.
> > Are we sure there is a one-to-one mapping from connection to sessionID?
> > I would have thought that a connection could support multiple sessionIDs.
> 
> Yes, this one is tricky. 
> In our previous implementation, we did only support one session per
> connection. See StorageManager::processWillOpenConnection: it dispatches the
> StorageManager message of connection to one StorageManager.
> This patch tries to keep old behavior. We may change that if we start to
> support multiple sessions in one web process.
> 
> The purpose of m_sessionByConnection is to support setPrivateBrowsingEnabled
> for tests. setPrivateBrowsingEnabled can change session of a web process
> after connection to some StorageManager is established. If this happens, we
> should stop dispatching message to StorageManager with old sessionID.

If this is only for testing, we could try to remove this.
To cover private browsing mode, we could rely on API tests.
And/or WTR could be beefed up to switch to ephemeral sessions on navigation or something like that.
Again, it might be easier in a follow-up patch.

[Sihui Liu]

Created attachment 369978 [details]
Patch

[Sihui Liu]

Created attachment 369982 [details]
Patch

[EWS Watchlist]

Comment on attachment 369982 [details]
Patch

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

New failing tests:
storage/indexeddb/IDBObject-leak.html
storage/indexeddb/modern/opendatabase-after-storage-crash.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 369982 [details]
Patch

Attachment 369982 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12200196

New failing tests:
storage/indexeddb/IDBObject-leak.html
storage/indexeddb/modern/opendatabase-after-storage-crash.html

[EWS Watchlist]

Created attachment 369990 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.4

[Sihui Liu]

Created attachment 370015 [details]
Patch

[Sihui Liu]

Created attachment 370047 [details]
Patch

[youenn fablet]

Comment on attachment 370047 [details]
Patch


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

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:307
> +    m_networkProcess->webProcessWasDisconnected(connection);

Are we making sure we unregister all pages of the web process if it crashes?
Is there a race condition in case of process swapping, between the old page that should remove itself and the new page in the new process that should register itself?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2641
> +    storageManager.addAllowedSessionStorageNamespaceConnection(pageID, connection);

Since this is IPC parameters, we might want to check that oldPageID/pageID are not 0.

> Source/WebKit/NetworkProcess/NetworkSession.h:123
> +    RefPtr<StorageManager> m_storageManager;

Can it be a Ref<>?

> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:86
> +{

Can we assert that it is not main thread?
If I understand correctly, this is called from IPC but probably IPC is dispatched to a work queue so this is probably fine.

> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.h:74
> +    RefPtr<WorkQueue> m_queue;

Can it be a Ref<>

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:101
> +    explicit LocalStorageNamespace(StorageManager*, uint64_t storageManagerID);

No need for explicit.

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:378
> +    m_storageManager->m_localStorageNamespaces.remove(m_storageNamespaceID);

Can we be sure m_storageManager is not null?
Should we change it to a reference?

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:718
> +    // FIXME: This should be a message check.

Seems nice to fix.
We might also want to ASSERT that we are on the right run loop as m_queue for this method and others that are called from IPC directly.

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:769
> +    TransientLocalStorageNamespace* transientLocalStorageNamespace = getOrCreateTransientLocalStorageNamespace(storageNamespaceID, WTFMove(topLevelOriginData));

auto

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:834
> +    StorageArea* storageArea = findStorageArea(connection, storageMapID);

auto here and below

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.h:35
> +#include <wtf/ThreadSafeRefCounted.h>

Do we need this one?

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:208
>      dataStoresWithStorageManagers().removeFirst(this);

Can we remove dataStoresWithStorageManagers()?
Are we handling the case of network process termination/suspension and making sure it is writing all necessary things?

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:244
>          processAccessType = std::max(processAccessType, ProcessAccessType::Launch);

Unrelated to this patch, but we are not consistent with our handling for IDB/SW and LocalStorage.
I guess we should return OnlyIfLaunched for isNonPersistentStore for IDB, SW and DOMCache.

> Source/WebKit/WebProcess/WebProcess.cpp:1253
> +        // To recover web storage.

I am not sure I like this.
Usually, web process is reacting upon crash in WebProcess::networkProcessConnectionClosed.
Maybe web pages should be told that the network process is crashed and they would re-register themselves.

> Source/WebKit/WebProcess/WebProcess.cpp:1690
> +    m_privateBrowsingEnabled = enabled;

Can we try to do without m_privateBrowsingEnabled? We do not want to have private browsing at the web process scope.
It seems used for crash recovery, which is probably not needed for testing.
It seems used when registering new pages/removing pages, but shouldn't pages sessionID be already changed to legacyPrivateSessionID?

[Sihui Liu]

Created attachment 370166 [details]
Patch

[Sihui Liu]

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

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:307
>> +    m_networkProcess->webProcessWasDisconnected(connection);
> 
> Are we making sure we unregister all pages of the web process if it crashes?
> Is there a race condition in case of process swapping, between the old page that should remove itself and the new page in the new process that should register itself?

No we are not, I will add this in StorageManager.

What kind of race condition? In process swapping, we should create a new provisional page in the new process and then remove the page from old process when commit the swap, right?

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2641
>> +    storageManager.addAllowedSessionStorageNamespaceConnection(pageID, connection);
> 
> Since this is IPC parameters, we might want to check that oldPageID/pageID are not 0.

Sure.

>> Source/WebKit/NetworkProcess/NetworkSession.h:123
>> +    RefPtr<StorageManager> m_storageManager;
> 
> Can it be a Ref<>?

Yes. Will change.

>> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:86
>> +{
> 
> Can we assert that it is not main thread?
> If I understand correctly, this is called from IPC but probably IPC is dispatched to a work queue so this is probably fine.

Right, I forgot to make this change.

>> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.h:74
>> +    RefPtr<WorkQueue> m_queue;
> 
> Can it be a Ref<>

Yes.

>> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:101
>> +    explicit LocalStorageNamespace(StorageManager*, uint64_t storageManagerID);
> 
> No need for explicit.

Removed.

>> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:378
>> +    m_storageManager->m_localStorageNamespaces.remove(m_storageNamespaceID);
> 
> Can we be sure m_storageManager is not null?
> Should we change it to a reference?

LocalStorageNameSpace is ref'ed by StorageManager. Reference to m_storageManager would create a cycle.

>> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:718
>> +    // FIXME: This should be a message check.
> 
> Seems nice to fix.
> We might also want to ASSERT that we are on the right run loop as m_queue for this method and others that are called from IPC directly.

Are you saying that we should check whether this is not on the main run loop?

>> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:769
>> +    TransientLocalStorageNamespace* transientLocalStorageNamespace = getOrCreateTransientLocalStorageNamespace(storageNamespaceID, WTFMove(topLevelOriginData));
> 
> auto

Okay.

>> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:834
>> +    StorageArea* storageArea = findStorageArea(connection, storageMapID);
> 
> auto here and below

Okay.

>> Source/WebKit/NetworkProcess/WebStorage/StorageManager.h:35
>> +#include <wtf/ThreadSafeRefCounted.h>
> 
> Do we need this one?

No we don't.

>> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:208
>>      dataStoresWithStorageManagers().removeFirst(this);
> 
> Can we remove dataStoresWithStorageManagers()?
> Are we handling the case of network process termination/suspension and making sure it is writing all necessary things?

I think we should just rename dataStoresWithStorageManagers to dataStores as it's used by resourceLoadStatistics.

This patch doesn't handle the network process termination/suspension. I try to focus on just moving the storage to network process.

>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:244
>>          processAccessType = std::max(processAccessType, ProcessAccessType::Launch);
> 
> Unrelated to this patch, but we are not consistent with our handling for IDB/SW and LocalStorage.
> I guess we should return OnlyIfLaunched for isNonPersistentStore for IDB, SW and DOMCache.

Yes, I will add a FIXME here.

>> Source/WebKit/WebProcess/WebProcess.cpp:1253
>> +        // To recover web storage.
> 
> I am not sure I like this.
> Usually, web process is reacting upon crash in WebProcess::networkProcessConnectionClosed.
> Maybe web pages should be told that the network process is crashed and they would re-register themselves.

Web process knows when the network connection is closed and re-established; webpage does not know when they should re-register. Storage operation is done through StorageAreaMap.

>> Source/WebKit/WebProcess/WebProcess.cpp:1690
>> +    m_privateBrowsingEnabled = enabled;
> 
> Can we try to do without m_privateBrowsingEnabled? We do not want to have private browsing at the web process scope.
> It seems used for crash recovery, which is probably not needed for testing.
> It seems used when registering new pages/removing pages, but shouldn't pages sessionID be already changed to legacyPrivateSessionID?

Discussed offline. Will remove this boolean!

[Sihui Liu]

> >> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:378
> >> +    m_storageManager->m_localStorageNamespaces.remove(m_storageNamespaceID);
> > 
> > Can we be sure m_storageManager is not null?
> > Should we change it to a reference?
> 
> LocalStorageNameSpace is ref'ed by StorageManager. Reference to
> m_storageManager would create a cycle.
> 
I misunderstood. Changed to a StorageManager&

> >> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:718
> >> +    // FIXME: This should be a message check.
> > 
> > Seems nice to fix.
> > We might also want to ASSERT that we are on the right run loop as m_queue for this method and others that are called from IPC directly.
> 
> Are you saying that we should check whether this is not on the main run loop?
> 
It seems we don't have the function to get RunLoop of WorkQueue on cocoa? I just use !RunLoop::isMain().

> >> Source/WebKit/WebProcess/WebProcess.cpp:1253
> >> +        // To recover web storage.
> > 
> > I am not sure I like this.
> > Usually, web process is reacting upon crash in WebProcess::networkProcessConnectionClosed.
> > Maybe web pages should be told that the network process is crashed and they would re-register themselves.
> 
> Web process knows when the network connection is closed and re-established;
> webpage does not know when they should re-register. Storage operation is
> done through StorageAreaMap.
> 
I tried to do this through StorageAreaMap, but that makes things more complicated as StorageAreaMap could be on the same page. In that case we may register the page multiple times.

[Sihui Liu]

Created attachment 370172 [details]
Patch

[EWS Watchlist]

Comment on attachment 370172 [details]
Patch

Attachment 370172 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12221617

New failing tests:
storage/indexeddb/modern/get-keyrange.html
storage/indexeddb/modern/gc-closes-database.html
webanimations/accelerated-animation-with-delay-and-seek.html

[EWS Watchlist]

Created attachment 370187 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[youenn fablet]

Comment on attachment 370172 [details]
Patch

LGTM, a few more comments.


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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1029
> +                return RegistrableDomain::uncheckedCreateFromHost(origin.host) == domain;

return domain.matches(origin);

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2692
> +void NetworkProcess::getLocalStorageOriginDetails(PAL::SessionID sessionID, CompletionHandler<void(Vector<LocalStorageDatabaseTracker::OriginDetails>)>&& completionHandler)

CompletionHandler should probably take a Vector<>&&

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2702
> +        completionHandler(details);

WTFMove(details).

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:366
> +    return storageArea;

We could use HashMap.ensure instead.

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:384
> +        if (originAndStorageArea.key == securityOrigin)

Shouldn't we just use m_storageAraeMap.find(securityOrigin)?

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:426
> +                storageArea->clear();

Ditto, m_storageAreaMap is keyed by SecurityOriginData

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:472
> +    return *m_storageAreaMap.ensure(securityOrigin, [this, securityOrigin]() mutable {

s/[this, securityOrigin]/[this, &securityOrigin]/
s/*m_storageAreaMap/m_storageAreaMap/

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:727
> +    ASSERT(!RunLoop::isMain());

This is good for now.
In the future, we could do something like:
ASSERT(m_queue.isCurrent())
On GLib, it would check the run loop.
On Cocoa, it would use dispatch_assert_queue.
Can we file a bugzilla for that and add a FIXME.

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:731
> +    // FIXME: This should be a message check.

Can we file a bugzilla for these?

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:970
> +        slot = LocalStorageNamespace::create(*this, storageNamespaceID);

We could use ensure() here and below

> Source/WebKit/UIProcess/API/C/WKKeyValueStorageManager.cpp:67
> +    websiteDataStore.fetchData({ WebKit::WebsiteDataType::LocalStorage, WebKit::WebsiteDataType::SessionStorage }, { }, [context, callback](auto dataRecords) {

Is WebKit:: needed, here and below?

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:208
> +    dataStores().removeFirst(this);

This is fragile, dataStores() should probably be a HashSet if we want to be sure to only include it once and order is not important.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:240
> +    // FIXME: make the handling for isNonPersistentStore be consistent with data types above.

Bugzilla maybe.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:-1812
> -    });

There is a risk that completionHandler is not called.
Let's make sure this is not the case even if this is only testing code.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2082
> +void WebsiteDataStore::getLocalStorageDetails(Function<void(Vector<LocalStorageDatabaseTracker::OriginDetails>)>&& completionHandler)

Ideally, this would take a Vector<>&&, if not too much work here or as a follow-up

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2091
> +        processPool->networkProcess()->getLocalStorageDetails(m_sessionID, [completionHandler = WTFMove(completionHandler)](auto details) {

We should only move completionHandler once.
If we need to support multiple pools, we need to consolidate the results ourselves here.
Please fix this before landing.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1406
> +    PAL::SessionID session(sessionID());

auto sessionID = this->sessionID();

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1434
> +    WebProcess::singleton().removeWebPage(session, m_pageID);

s/session/sessionID/

> Source/WebKit/WebProcess/WebProcess.cpp:1253
> +        // To recover web storage.

This is not great but is better than nothing.
Let's file a bugzilla and improve that as soon as possible after landing.

> Source/WebKit/WebProcess/WebProcess.cpp:1686
> +        if (page.value)

Do we need that check?
I would believe enablePrivateBrowsingForTesting to not be called in the middle of a WebPage creation.

[Sihui Liu]

Comment on attachment 370172 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1029
>> +                return RegistrableDomain::uncheckedCreateFromHost(origin.host) == domain;
> 
> return domain.matches(origin);

Okay.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2692
>> +void NetworkProcess::getLocalStorageOriginDetails(PAL::SessionID sessionID, CompletionHandler<void(Vector<LocalStorageDatabaseTracker::OriginDetails>)>&& completionHandler)
> 
> CompletionHandler should probably take a Vector<>&&

Okay.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2702
>> +        completionHandler(details);
> 
> WTFMove(details).

Okay.

>> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:366
>> +    return storageArea;
> 
> We could use HashMap.ensure instead.

Okay.

>> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:384
>> +        if (originAndStorageArea.key == securityOrigin)
> 
> Shouldn't we just use m_storageAraeMap.find(securityOrigin)?

Yes, changed.

>> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:426
>> +                storageArea->clear();
> 
> Ditto, m_storageAreaMap is keyed by SecurityOriginData

Changed.

>> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:472
>> +    return *m_storageAreaMap.ensure(securityOrigin, [this, securityOrigin]() mutable {
> 
> s/[this, securityOrigin]/[this, &securityOrigin]/
> s/*m_storageAreaMap/m_storageAreaMap/

Okay.

>> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:727
>> +    ASSERT(!RunLoop::isMain());
> 
> This is good for now.
> In the future, we could do something like:
> ASSERT(m_queue.isCurrent())
> On GLib, it would check the run loop.
> On Cocoa, it would use dispatch_assert_queue.
> Can we file a bugzilla for that and add a FIXME.

FIXME added.

>> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:731
>> +    // FIXME: This should be a message check.
> 
> Can we file a bugzilla for these?

Filed.

>> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:970
>> +        slot = LocalStorageNamespace::create(*this, storageNamespaceID);
> 
> We could use ensure() here and below

Okay.

>> Source/WebKit/UIProcess/API/C/WKKeyValueStorageManager.cpp:67
>> +    websiteDataStore.fetchData({ WebKit::WebsiteDataType::LocalStorage, WebKit::WebsiteDataType::SessionStorage }, { }, [context, callback](auto dataRecords) {
> 
> Is WebKit:: needed, here and below?

Not needed.

>> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:208
>> +    dataStores().removeFirst(this);
> 
> This is fragile, dataStores() should probably be a HashSet if we want to be sure to only include it once and order is not important.

Changed to HashSet.

>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:240
>> +    // FIXME: make the handling for isNonPersistentStore be consistent with data types above.
> 
> Bugzilla maybe.

Filed.

>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:-1812
>> -    });
> 
> There is a risk that completionHandler is not called.
> Let's make sure this is not the case even if this is only testing code.

Assert added.

>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2082
>> +void WebsiteDataStore::getLocalStorageDetails(Function<void(Vector<LocalStorageDatabaseTracker::OriginDetails>)>&& completionHandler)
> 
> Ideally, this would take a Vector<>&&, if not too much work here or as a follow-up

Changed.

>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2091
>> +        processPool->networkProcess()->getLocalStorageDetails(m_sessionID, [completionHandler = WTFMove(completionHandler)](auto details) {
> 
> We should only move completionHandler once.
> If we need to support multiple pools, we need to consolidate the results ourselves here.
> Please fix this before landing.

Added break after WTFMove.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1406
>> +    PAL::SessionID session(sessionID());
> 
> auto sessionID = this->sessionID();

Okay.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1434
>> +    WebProcess::singleton().removeWebPage(session, m_pageID);
> 
> s/session/sessionID/

Okay.

>> Source/WebKit/WebProcess/WebProcess.cpp:1253
>> +        // To recover web storage.
> 
> This is not great but is better than nothing.
> Let's file a bugzilla and improve that as soon as possible after landing.

Okay. Filed.

>> Source/WebKit/WebProcess/WebProcess.cpp:1686
>> +        if (page.value)
> 
> Do we need that check?
> I would believe enablePrivateBrowsingForTesting to not be called in the middle of a WebPage creation.

Yes, WebPage::create -> InjectedBundle::didCreatePage -> InjectedBundle::beginTesting, which invokes enablePrivateBrowsingForTesting to reset PrivateBrowsing.

[Sihui Liu]

Created attachment 370280 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 370280 [details]
Patch for landing

Clearing flags on attachment: 370280

Committed r245540: <https://trac.webkit.org/changeset/245540>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50967284>

[Ryan Haddad]

This change caused iOS Debug layout tests to exit early with the following assertion:

ASSERTION FAILED: identifier.isNull() || RunLoop::isMain()
./platform/cocoa/RuntimeApplicationChecksCocoa.mm(58) : WTF::String WebCore::applicationBundleIdentifier()
1   0x122d106d9 WTFCrash
2   0x1274c683b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x127b70630 WebCore::applicationBundleIdentifier()
4   0x127b70a39 WebCore::applicationBundleIsEqualTo(WTF::String const&)
5   0x127b70ad9 WebCore::IOSApplication::isMobileSafari()
6   0x117b130f1 WebKit::linkedOnOrAfter(WebKit::SDKVersion, WebKit::AssumeSafariIsAlwaysLinkedOnAfter)::$_17::operator()() const
7   0x117b1309d decltype(std::__1::forward<WebKit::linkedOnOrAfter(WebKit::SDKVersion, WebKit::AssumeSafariIsAlwaysLinkedOnAfter)::$_17>(fp)()) std::__1::__invoke<WebKit::linkedOnOrAfter(WebKit::SDKVersion, WebKit::AssumeSafariIsAlwaysLinkedOnAfter)::$_17>(WebKit::linkedOnOrAfter(WebKit::SDKVersion, WebKit::AssumeSafariIsAlwaysLinkedOnAfter)::$_17&&)
8   0x117b13078 void std::__1::__call_once_param<std::__1::tuple<WebKit::linkedOnOrAfter(WebKit::SDKVersion, WebKit::AssumeSafariIsAlwaysLinkedOnAfter)::$_17&&> >::__execute<>(std::__1::__tuple_indices<>)
9   0x117b13045 std::__1::__call_once_param<std::__1::tuple<WebKit::linkedOnOrAfter(WebKit::SDKVersion, WebKit::AssumeSafariIsAlwaysLinkedOnAfter)::$_17&&> >::operator()()
10  0x117b12f1d void std::__1::__call_once_proxy<std::__1::tuple<WebKit::linkedOnOrAfter(WebKit::SDKVersion, WebKit::AssumeSafariIsAlwaysLinkedOnAfter)::$_17&&> >(void*)
11  0x110584be8 std::__1::__call_once(unsigned long volatile&, void*, void (*)(void*))
12  0x117afeecc void std::__1::call_once<WebKit::linkedOnOrAfter(WebKit::SDKVersion, WebKit::AssumeSafariIsAlwaysLinkedOnAfter)::$_17>(std::__1::once_flag&, WebKit::linkedOnOrAfter(WebKit::SDKVersion, WebKit::AssumeSafariIsAlwaysLinkedOnAfter)::$_17&&)
13  0x117afede3 WebKit::linkedOnOrAfter(WebKit::SDKVersion, WebKit::AssumeSafariIsAlwaysLinkedOnAfter)
14  0x11747af9a WebKit::LocalStorageDatabaseTracker::platformMaybeExcludeFromBackup() const
15  0x1176a3c88 WebKit::LocalStorageDatabaseTracker::databasePath(WTF::String const&) const
16  0x1176d3a2a WebKit::LocalStorageDatabaseTracker::LocalStorageDatabaseTracker(WTF::Ref<WTF::WorkQueue, WTF::DumbPtrTraits<WTF::WorkQueue> >&&, WTF::String const&)::$_3::operator()()
17  0x1176d3969 WTF::Detail::CallableWrapper<WebKit::LocalStorageDatabaseTracker::LocalStorageDatabaseTracker(WTF::Ref<WTF::WorkQueue, WTF::DumbPtrTraits<WTF::WorkQueue> >&&, WTF::String const&)::$_3, void>::call()
18  0x122d3d28a WTF::Function<void ()>::operator()() const
19  0x122e2dc39 WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0::operator()() const
20  0x122e2de20 WTF::BlockPtr<void ()> WTF::BlockPtr<void ()>::fromCallable<WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0>(WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0)::'lambda'(void*)::operator()(void*) const
21  0x122e2ddf5 WTF::BlockPtr<void ()> WTF::BlockPtr<void ()>::fromCallable<WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0>(WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0)::'lambda'(void*)::__invoke(void*)
22  0x111eddccf _dispatch_call_block_and_release
23  0x111eded02 _dispatch_client_callout
24  0x111ee5720 _dispatch_lane_serial_drain
25  0x111ee6261 _dispatch_lane_invoke
26  0x111eeefcb _dispatch_workloop_worker_thread
27  0x1122c0611 _pthread_wqthread
28  0x1122c03fd start_wqthread
LEAK: 1 WebPageProxy

https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Debug%20WK2%20%28Tests%29/builds/3792

[Sihui Liu]

(In reply to Ryan Haddad from comment #38)
> This change caused iOS Debug layout tests to exit early with the following
> assertion:
> 
> ASSERTION FAILED: identifier.isNull() || RunLoop::isMain()
> ./platform/cocoa/RuntimeApplicationChecksCocoa.mm(58) : WTF::String
> WebCore::applicationBundleIdentifier()
> 1   0x122d106d9 WTFCrash
> 2   0x1274c683b WTFCrashWithInfo(int, char const*, char const*, int)
> 3   0x127b70630 WebCore::applicationBundleIdentifier()
> 4   0x127b70a39 WebCore::applicationBundleIsEqualTo(WTF::String const&)
> 5   0x127b70ad9 WebCore::IOSApplication::isMobileSafari()
> 6   0x117b130f1 WebKit::linkedOnOrAfter(WebKit::SDKVersion,
> WebKit::AssumeSafariIsAlwaysLinkedOnAfter)::$_17::operator()() const
> 7   0x117b1309d
> decltype(std::__1::forward<WebKit::linkedOnOrAfter(WebKit::SDKVersion,
> WebKit::AssumeSafariIsAlwaysLinkedOnAfter)::$_17>(fp)())
> std::__1::__invoke<WebKit::linkedOnOrAfter(WebKit::SDKVersion,
> WebKit::AssumeSafariIsAlwaysLinkedOnAfter)::$_17>(WebKit::
> linkedOnOrAfter(WebKit::SDKVersion,
> WebKit::AssumeSafariIsAlwaysLinkedOnAfter)::$_17&&)
> 8   0x117b13078 void
> std::__1::__call_once_param<std::__1::tuple<WebKit::linkedOnOrAfter(WebKit::
> SDKVersion, WebKit::AssumeSafariIsAlwaysLinkedOnAfter)::$_17&&>
> >::__execute<>(std::__1::__tuple_indices<>)
> 9   0x117b13045
> std::__1::__call_once_param<std::__1::tuple<WebKit::linkedOnOrAfter(WebKit::
> SDKVersion, WebKit::AssumeSafariIsAlwaysLinkedOnAfter)::$_17&&>
> >::operator()()
> 10  0x117b12f1d void
> std::__1::__call_once_proxy<std::__1::tuple<WebKit::linkedOnOrAfter(WebKit::
> SDKVersion, WebKit::AssumeSafariIsAlwaysLinkedOnAfter)::$_17&&> >(void*)
> 11  0x110584be8 std::__1::__call_once(unsigned long volatile&, void*, void
> (*)(void*))
> 12  0x117afeecc void
> std::__1::call_once<WebKit::linkedOnOrAfter(WebKit::SDKVersion,
> WebKit::AssumeSafariIsAlwaysLinkedOnAfter)::$_17>(std::__1::once_flag&,
> WebKit::linkedOnOrAfter(WebKit::SDKVersion,
> WebKit::AssumeSafariIsAlwaysLinkedOnAfter)::$_17&&)
> 13  0x117afede3 WebKit::linkedOnOrAfter(WebKit::SDKVersion,
> WebKit::AssumeSafariIsAlwaysLinkedOnAfter)
> 14  0x11747af9a
> WebKit::LocalStorageDatabaseTracker::platformMaybeExcludeFromBackup() const
> 15  0x1176a3c88
> WebKit::LocalStorageDatabaseTracker::databasePath(WTF::String const&) const
> 16  0x1176d3a2a
> WebKit::LocalStorageDatabaseTracker::LocalStorageDatabaseTracker(WTF::
> Ref<WTF::WorkQueue, WTF::DumbPtrTraits<WTF::WorkQueue> >&&, WTF::String
> const&)::$_3::operator()()
> 17  0x1176d3969
> WTF::Detail::CallableWrapper<WebKit::LocalStorageDatabaseTracker::
> LocalStorageDatabaseTracker(WTF::Ref<WTF::WorkQueue,
> WTF::DumbPtrTraits<WTF::WorkQueue> >&&, WTF::String const&)::$_3,
> void>::call()
> 18  0x122d3d28a WTF::Function<void ()>::operator()() const
> 19  0x122e2dc39 WTF::WorkQueue::dispatch(WTF::Function<void
> ()>&&)::$_0::operator()() const
> 20  0x122e2de20 WTF::BlockPtr<void ()> WTF::BlockPtr<void
> ()>::fromCallable<WTF::WorkQueue::dispatch(WTF::Function<void
> ()>&&)::$_0>(WTF::WorkQueue::dispatch(WTF::Function<void
> ()>&&)::$_0)::'lambda'(void*)::operator()(void*) const
> 21  0x122e2ddf5 WTF::BlockPtr<void ()> WTF::BlockPtr<void
> ()>::fromCallable<WTF::WorkQueue::dispatch(WTF::Function<void
> ()>&&)::$_0>(WTF::WorkQueue::dispatch(WTF::Function<void
> ()>&&)::$_0)::'lambda'(void*)::__invoke(void*)
> 22  0x111eddccf _dispatch_call_block_and_release
> 23  0x111eded02 _dispatch_client_callout
> 24  0x111ee5720 _dispatch_lane_serial_drain
> 25  0x111ee6261 _dispatch_lane_invoke
> 26  0x111eeefcb _dispatch_workloop_worker_thread
> 27  0x1122c0611 _pthread_wqthread
> 28  0x1122c03fd start_wqthread
> LEAK: 1 WebPageProxy
> 
> https://build.webkit.org/builders/
> Apple%20iOS%2012%20Simulator%20Debug%20WK2%20%28Tests%29/builds/3792

We should dispatch the check to runloop::main(). Working on a fix.

[Shawn Roberts]

New API test added in 

TestWebKitAPI.WKWebView.LocalStorageProcessCrashes 

is a flaky failure on Mac Release WK2 testers

Reproduced with:

run-api-tests TestWebKitAPI.WKWebView.LocalStorageProcessCrashes --debug --iter30

    TestWebKitAPI.WKWebView.LocalStorageProcessCrashes
        _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
        
        /Volumes/Data/slave/mojave-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm:91
        Value of: [@"storage" isEqualToString:result]
          Actual: false
        Expected: true