Bug 180573 – Delay some service worker operations until after the database import completes

• Component: Service Workers
• Status: RESOLVED FIXED

[Brady Eidson]

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.

[Brady Eidson]

Created attachment 328794 [details]
Patch

Bedtime ZzZzzzzzz

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Brady Eidson]

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.

[Brady Eidson]

(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...

[Brady Eidson]

Created attachment 328852 [details]
Patch

[Brady Eidson]

The failures were probably because of the "multiple WKTR's sharing the same database file" bug that was just resolved.

[Chris Dumez]

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.

[Chris Dumez]

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).

[Chris Dumez]

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.

[Brady Eidson]

Created attachment 328867 [details]
Patch

[Chris Dumez]

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?

[Chris Dumez]

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.

[Brady Eidson]

Created attachment 328873 [details]
Patch

[Brady Eidson]

Didn't try to fix the tests in this new patch - Exploring if I can reproduce them locally

[Brady Eidson]

COOL: Just http/tests/cache-storage/cache-persistency.https.html by itself timeouts

[Brady Eidson]

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

[Brady Eidson]

These private browsing things are already occurring... but now activity is delayed as a result.

[Brady Eidson]

Created attachment 328879 [details]
Patch

[Chris Dumez]

Comment on attachment 328879 [details]
Patch

r=me if the bots are happy.

[Brady Eidson]

(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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Brady Eidson]

I think it's a mis-merge after https://trac.webkit.org/changeset/225711/webkit landed. ugh.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Chris Dumez]

(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.

[Chris Dumez]

(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?

[Chris Dumez]

(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

[Chris Dumez]

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.

[Chris Dumez]

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?

[Chris Dumez]

Created attachment 328893 [details]
Patch

[Chris Dumez]

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.

[Brady Eidson]

(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?

[Chris Dumez]

(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.

[Brady Eidson]

Awesome. So thinking about that the symptom and fix totally fit with each other.

Thanks for following up while I was stuck in traffic. :)

[Chris Dumez]

(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.

[WebKit Commit Bot]

Comment on attachment 328893 [details]
Patch

Clearing flags on attachment: 328893

Committed r225717: <https://trac.webkit.org/changeset/225717>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.