WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
197636
Move Web Storage to Network Process
https://bugs.webkit.org/show_bug.cgi?id=197636
Summary
Move Web Storage to Network Process
Sihui Liu
Reported
2019-05-06 16:32:26 PDT
To facilitate quota computation and database file management during process suspension.
Attachments
Patch
(264.51 KB, patch)
2019-05-06 16:50 PDT
,
Sihui Liu
no flags
Details
Formatted Diff
Diff
Patch
(265.40 KB, patch)
2019-05-06 17:41 PDT
,
Sihui Liu
no flags
Details
Formatted Diff
Diff
Patch
(266.31 KB, patch)
2019-05-06 18:14 PDT
,
Sihui Liu
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews104 for mac-highsierra-wk2
(2.68 MB, application/zip)
2019-05-06 19:22 PDT
,
EWS Watchlist
no flags
Details
Archive of layout-test-results from ews123 for ios-simulator-wk2
(2.59 MB, application/zip)
2019-05-06 20:33 PDT
,
EWS Watchlist
no flags
Details
Archive of layout-test-results from ews211 for win-future
(13.94 MB, application/zip)
2019-05-07 00:40 PDT
,
EWS Watchlist
no flags
Details
Patch
(270.34 KB, patch)
2019-05-08 12:03 PDT
,
Sihui Liu
no flags
Details
Formatted Diff
Diff
Patch
(270.27 KB, patch)
2019-05-13 09:05 PDT
,
Sihui Liu
no flags
Details
Formatted Diff
Diff
Patch
(270.24 KB, patch)
2019-05-13 09:52 PDT
,
Sihui Liu
no flags
Details
Formatted Diff
Diff
Patch
(287.83 KB, patch)
2019-05-15 11:55 PDT
,
Sihui Liu
no flags
Details
Formatted Diff
Diff
Patch
(286.71 KB, patch)
2019-05-15 12:13 PDT
,
Sihui Liu
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews107 for mac-highsierra-wk2
(2.86 MB, application/zip)
2019-05-15 13:22 PDT
,
EWS Watchlist
no flags
Details
Archive of layout-test-results from ews122 for ios-simulator-wk2
(19.92 MB, application/zip)
2019-05-15 14:18 PDT
,
EWS Watchlist
no flags
Details
Patch
(290.72 KB, patch)
2019-05-15 18:38 PDT
,
Sihui Liu
no flags
Details
Formatted Diff
Diff
Patch
(290.79 KB, patch)
2019-05-16 10:15 PDT
,
Sihui Liu
no flags
Details
Formatted Diff
Diff
Patch
(292.75 KB, patch)
2019-05-17 15:53 PDT
,
Sihui Liu
no flags
Details
Formatted Diff
Diff
Patch
(293.05 KB, patch)
2019-05-17 18:16 PDT
,
Sihui Liu
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews211 for win-future
(13.44 MB, application/zip)
2019-05-17 21:53 PDT
,
EWS Watchlist
no flags
Details
Patch for landing
(293.72 KB, patch)
2019-05-20 16:26 PDT
,
Sihui Liu
no flags
Details
Formatted Diff
Diff
Show Obsolete
(18)
View All
Add attachment
proposed patch, testcase, etc.
Sihui Liu
Comment 1
2019-05-06 16:50:50 PDT
Created
attachment 369206
[details]
Patch
Sihui Liu
Comment 2
2019-05-06 17:41:40 PDT
Created
attachment 369210
[details]
Patch
Sihui Liu
Comment 3
2019-05-06 18:14:56 PDT
Created
attachment 369215
[details]
Patch
EWS Watchlist
Comment 4
2019-05-06 19:21:59 PDT
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
Comment 5
2019-05-06 19:22:00 PDT
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 6
2019-05-06 20:33:40 PDT
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
Comment 7
2019-05-06 20:33:42 PDT
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 8
2019-05-07 00:40:53 PDT
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
Comment 9
2019-05-07 00:40:57 PDT
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 10
2019-05-08 09:26:55 PDT
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
Comment 11
2019-05-08 12:03:41 PDT
Created
attachment 369404
[details]
Patch
Sihui Liu
Comment 12
2019-05-13 09:05:36 PDT
Created
attachment 369737
[details]
Patch
Sihui Liu
Comment 13
2019-05-13 09:52:12 PDT
Created
attachment 369739
[details]
Patch
youenn fablet
Comment 14
2019-05-13 10:49:21 PDT
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 15
2019-05-13 18:37:53 PDT
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
Comment 16
2019-05-13 18:49:05 PDT
> > 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
Comment 17
2019-05-15 11:55:38 PDT
Created
attachment 369978
[details]
Patch
Sihui Liu
Comment 18
2019-05-15 12:13:54 PDT
Created
attachment 369982
[details]
Patch
EWS Watchlist
Comment 19
2019-05-15 13:22:52 PDT
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
Comment 20
2019-05-15 13:22:54 PDT
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 21
2019-05-15 14:18:17 PDT
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
Comment 22
2019-05-15 14:18:25 PDT
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
Comment 23
2019-05-15 18:38:03 PDT
Created
attachment 370015
[details]
Patch
Sihui Liu
Comment 24
2019-05-16 10:15:27 PDT
Created
attachment 370047
[details]
Patch
youenn fablet
Comment 25
2019-05-16 16:05:27 PDT
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
Comment 26
2019-05-17 15:53:18 PDT
Created
attachment 370166
[details]
Patch
Sihui Liu
Comment 27
2019-05-17 15:54:18 PDT
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
Comment 28
2019-05-17 18:14:41 PDT
> >> 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
Comment 29
2019-05-17 18:16:30 PDT
Created
attachment 370172
[details]
Patch
EWS Watchlist
Comment 30
2019-05-17 21:53:07 PDT
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
Comment 31
2019-05-17 21:53:11 PDT
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 32
2019-05-20 10:08:23 PDT
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 33
2019-05-20 16:26:24 PDT
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
Comment 34
2019-05-20 16:26:51 PDT
Created
attachment 370280
[details]
Patch for landing
WebKit Commit Bot
Comment 35
2019-05-20 17:07:46 PDT
Comment on
attachment 370280
[details]
Patch for landing Clearing flags on attachment: 370280 Committed
r245540
: <
https://trac.webkit.org/changeset/245540
>
WebKit Commit Bot
Comment 36
2019-05-20 17:07:48 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 37
2019-05-20 17:08:33 PDT
<
rdar://problem/50967284
>
Ryan Haddad
Comment 38
2019-05-21 08:48:14 PDT
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
Comment 39
2019-05-21 09:04:22 PDT
(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
Comment 40
2019-05-21 15:31:34 PDT
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
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug