Delay many service worker operations until after the database import completes This is so pages don't stomp over existing registrations, so fetches from service workers will happen, etc etc.
Created attachment 328794 [details] Patch Bedtime ZzZzzzzzz
Comment on attachment 328794 [details] Patch Attachment 328794 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5540326 New failing tests: storage/indexeddb/pending-version-change-on-exit-private.html http/tests/contentextensions/top-url.html storage/indexeddb/pending-version-change-stuck-works-with-terminate-private.html storage/indexeddb/pending-version-change-stuck-private.html http/tests/cache-storage/cache-persistency.https.html http/tests/workers/service/ServiceWorkerGlobalScope_getRegistration.html http/tests/workers/service/basic-register.html storage/indexeddb/open-bad-versions-private.html http/tests/cache-storage/cache-representation.https.html http/tests/security/cookies/third-party-cookie-blocking-redirect.html animations/3d/change-transform-in-end-event.html storage/domstorage/localstorage/private-browsing-affects-storage.html storage/indexeddb/dont-wedge-private.html storage/indexeddb/modern/transactions-stop-on-navigation-private.html
Created attachment 328796 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 328794 [details] Patch Attachment 328794 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5540312 New failing tests: http/tests/security/cookies/third-party-cookie-blocking.html http/tests/security/cookies/third-party-cookie-blocking-xslt.xml http/tests/cache-storage/cache-persistency.https.html http/tests/workers/service/ServiceWorkerGlobalScope_getRegistration.html http/tests/security/cookies/third-party-cookie-blocking-redirect.html http/tests/cache-storage/cache-representation.https.html http/tests/security/cookies/third-party-cookie-blocking-main-frame.html http/tests/workers/service/basic-register.html storage/domstorage/localstorage/private-browsing-affects-storage.html http/tests/security/cookies/first-party-cookie-allow-xslt.xml
Created attachment 328798 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
The Mac WK2 failures appear to be something systemically wrong with the bot that was running things - We were seeing Cache tests fail, IDB tests fail, and content blocker extensions failing to compile... all things that this patch didn't touch.
(In reply to Brady Eidson from comment #6) > The Mac WK2 failures appear to be something systemically wrong with the bot > that was running things - We were seeing Cache tests fail, IDB tests fail, > and content blocker extensions failing to compile... all things that this > patch didn't touch. The iOS-sim failures were suspiciously similar =/ Building and running these locally...
Created attachment 328852 [details] Patch
The failures were probably because of the "multiple WKTR's sharing the same database file" bug that was just resolved.
Comment on attachment 328852 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328852&action=review > Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.cpp:71 > + if (!m_isImported) As soon as you start adding origins to the store, WebSWOriginStore::didInvalidateSharedMemory() will be called and it will send the store handle to the WebProcesses, which I guess is not want you want.
Comment on attachment 328852 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328852&action=review > Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.cpp:79 > sendStoreHandle(connection); Also, what makes sure you send the store handle to the Webprocesses once the import is complete (since you exit early earlier when !m_isImported).
Comment on attachment 328852 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328852&action=review >> Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.cpp:71 >> + if (!m_isImported) > > As soon as you start adding origins to the store, WebSWOriginStore::didInvalidateSharedMemory() will be called and it will send the store handle to the WebProcesses, which I guess is not want you want. As commented offline, it seems fine after all to send the storeHandle() before the import is complete. However, there is still an issue when: 1. Import is in progress and we already have some data in the table 2. WebProcess A starts and WebSWOriginStore::registerSWServerConnection() is called -> Does not send the store handle 3. Import is now complete and importComplete() is called. -> We send SetSWOriginTableIsImported IPC to WebProcess A but it may not have gotten the store handle.
Created attachment 328867 [details] Patch
Note that your recent patch iteration had the same timeouts: Regressions: Unexpected timeouts (12) http/tests/cache-storage/cache-persistency.https.html [ Timeout ] http/tests/cache-storage/cache-representation.https.html [ Timeout ] http/tests/security/cookies/third-party-cookie-blocking-redirect.html [ Timeout ] http/tests/workers/service/ServiceWorkerGlobalScope_getRegistration.html [ Timeout ] http/tests/workers/service/basic-register.html [ Timeout ] storage/domstorage/localstorage/private-browsing-affects-storage.html [ Timeout ] storage/indexeddb/dont-wedge-private.html [ Timeout ] storage/indexeddb/modern/transactions-stop-on-navigation-private.html [ Timeout ] storage/indexeddb/open-bad-versions-private.html [ Timeout ] storage/indexeddb/pending-version-change-on-exit-private.html [ Timeout ] storage/indexeddb/pending-version-change-stuck-private.html [ Timeout ] storage/indexeddb/pending-version-change-stuck-works-with-terminate-private.html [ Timeout ] They may not be fixed after all?
Comment on attachment 328867 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328867&action=review > Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.cpp:79 > + connection.send(Messages::WebSWClientConnection::SetSWOriginTableIsImported()); If m_store.isEmpty() but m_isImported is true, I believe we want to send the SetSWOriginTableIsImported IPC instead of merely returning early. Otherwise, the WebProcess will keep querying the StorageProcess even though there are no service workers and the import is done. > Source/WebKit/WebProcess/Storage/WebSWClientConnection.h:93 > + void runOrDelayTask(WTF::Function<void()>&& task); Nit: It'd be nice if the name indicated the reason for delaying. Alternatively, it might look nicer to make mayHaveServiceWorkerRegisteredForOrigin() take in a lambda instead.
Created attachment 328873 [details] Patch
Didn't try to fix the tests in this new patch - Exploring if I can reproduce them locally
COOL: Just http/tests/cache-storage/cache-persistency.https.html by itself timeouts
Yup, private browsing vs the database! Nice. ServiceWorker RegistrationDatabase opening file /var/folders/py/zsbx8whs03ngn_1kynm1ngph0000gn/T/WebKitTestRunners-JLXrgN/ServiceWorkers/ServiceWorkerRegistrations.sqlite3 ServiceWorker RegistrationDatabase opening file /ServiceWorkerRegistrations.sqlite3 ERROR: SQLite database failed to load from /ServiceWorkerRegistrations.sqlite3 Cause - unable to open database file /Volumes/Data/git/OpenSource/Source/WebCore/platform/sql/SQLiteDatabase.cpp(90) : bool WebCore::SQLiteDatabase::open(const WTF::String &, bool) Wait on notifyDone timed out, process com.apple.WebKit.WebContent.Development pid = 17358#EOF
These private browsing things are already occurring... but now activity is delayed as a result.
Created attachment 328879 [details] Patch
Comment on attachment 328879 [details] Patch r=me if the bots are happy.
(In reply to Chris Dumez from comment #22) > Comment on attachment 328879 [details] > Patch > > r=me if the bots are happy. The bots are not happy, but I also can't successfully run tests locally, without my patch, without the same failures... somethings rotten
Comment on attachment 328879 [details] Patch Attachment 328879 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5552888 New failing tests: http/tests/workers/service/ServiceWorkerGlobalScope_getRegistration.html
Created attachment 328885 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
I think it's a mis-merge after https://trac.webkit.org/changeset/225711/webkit landed. ugh.
Comment on attachment 328879 [details] Patch Attachment 328879 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5552797 New failing tests: http/tests/workers/service/ServiceWorkerGlobalScope_getRegistration.html
Created attachment 328888 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
(In reply to Build Bot from comment #27) > Comment on attachment 328879 [details] > Patch > > Attachment 328879 [details] did not pass ios-sim-ews (ios-simulator-wk2): > Output: http://webkit-queues.webkit.org/results/5552797 > > New failing tests: > http/tests/workers/service/ServiceWorkerGlobalScope_getRegistration.html It is timing out.
(In reply to Chris Dumez from comment #29) > (In reply to Build Bot from comment #27) > > Comment on attachment 328879 [details] > > Patch > > > > Attachment 328879 [details] did not pass ios-sim-ews (ios-simulator-wk2): > > Output: http://webkit-queues.webkit.org/results/5552797 > > > > New failing tests: > > http/tests/workers/service/ServiceWorkerGlobalScope_getRegistration.html > > It is timing out. The test in question is calling getRegistration() from a ServiceWorker, which ends up calling matchRegistration() that you modified. Presumably, the request gets queued up due to import and never processed?
(In reply to Chris Dumez from comment #30) > (In reply to Chris Dumez from comment #29) > > (In reply to Build Bot from comment #27) > > > Comment on attachment 328879 [details] > > > Patch > > > > > > Attachment 328879 [details] did not pass ios-sim-ews (ios-simulator-wk2): > > > Output: http://webkit-queues.webkit.org/results/5552797 > > > > > > New failing tests: > > > http/tests/workers/service/ServiceWorkerGlobalScope_getRegistration.html > > > > It is timing out. > > The test in question is calling getRegistration() from a ServiceWorker, > which ends up calling matchRegistration() that you modified. Presumably, the > request gets queued up due to import and never processed? I can reproduce the time out locally: Tools/Scripts/run-webkit-tests --no-retry http/tests/workers/service/ServiceWorkerGlobalScope_getRegistration.html
Comment on attachment 328879 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328879&action=review > Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp:155 > + runOrDelayTaskForImport([this, callback = WTFMove(callback), topOrigin = SecurityOriginData::fromSecurityOrigin(topOrigin), clientURL]() mutable { I added some logging locally, I see us calling runOrDelayTaskForImport() here but the lambda is never called.
Comment on attachment 328879 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328879&action=review > Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.cpp:74 > + if (m_isImported) This is never set to true?
Created attachment 328893 [details] Patch
Comment on attachment 328893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328893&action=review > Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.cpp:63 > + m_isImported = true; Setting m_isImported to true here fixes the issue for me locally.
(In reply to Chris Dumez from comment #35) > Comment on attachment 328893 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=328893&action=review > > > Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.cpp:63 > > + m_isImported = true; > > Setting m_isImported to true here fixes the issue for me locally. Yup, I had just sprinkled logging around that before having to leave the office, didn't quite finish proving it. That fixes http/tests/workers/service/ServiceWorkerGlobalScope_getRegistration.html, does it fix the others?
(In reply to Brady Eidson from comment #36) > (In reply to Chris Dumez from comment #35) > > Comment on attachment 328893 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=328893&action=review > > > > > Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.cpp:63 > > > + m_isImported = true; > > > > Setting m_isImported to true here fixes the issue for me locally. > > Yup, I had just sprinkled logging around that before having to leave the > office, didn't quite finish proving it. > > That fixes > http/tests/workers/service/ServiceWorkerGlobalScope_getRegistration.html, > does it fix the others? Last EWS round had only http/tests/workers/service/ServiceWorkerGlobalScope_getRegistration.html failing.
Awesome. So thinking about that the symptom and fix totally fit with each other. Thanks for following up while I was stuck in traffic. :)
(In reply to Brady Eidson from comment #38) > Awesome. So thinking about that the symptom and fix totally fit with each > other. > > Thanks for following up while I was stuck in traffic. :) No problem. Please make sure the radar is updated.
Comment on attachment 328893 [details] Patch Clearing flags on attachment: 328893 Committed r225717: <https://trac.webkit.org/changeset/225717>
All reviewed patches have been landed. Closing bug.