Clearing all Website Data should remove service worker registrations on disk.
Created attachment 328758 [details] WIP Patch
Created attachment 328762 [details] Patch
Comment on attachment 328762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328762&action=review > Source/WebCore/workers/service/server/SWServer.h:121 > WEBCORE_EXPORT void clear(const SecurityOrigin&); I guess a follow-up patch should do the same for clear(). > Source/WebKit/StorageProcess/StorageProcess.cpp:299 > + RefPtr<CallbackAggregator> callbackAggregator = CallbackAggregator::create([this, callbackID] { I got some comments in the past to use auto there, and use copyRef() below as a way to more explicitly state that a ref is taken. > Source/WebKit/StorageProcess/StorageProcess.cpp:312 > + idbServer(sessionID).closeAndDeleteDatabasesModifiedSince(modifiedSince, [callbackAggregator] { }); Could move callbackAggregator.
Comment on attachment 328762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328762&action=review >> Source/WebCore/workers/service/server/SWServer.h:121 >> WEBCORE_EXPORT void clear(const SecurityOrigin&); > > I guess a follow-up patch should do the same for clear(). Yes, will look into the clear(origin) version tomorrow.
Created attachment 328765 [details] Patch
Comment on attachment 328765 [details] Patch We usually strive for file deletion when we're about to leave a database empty. This also only implements "clear all" but doesn't implement clearing individual origins when requested.
(In reply to Chris Dumez from comment #4) > Comment on attachment 328762 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=328762&action=review > > >> Source/WebCore/workers/service/server/SWServer.h:121 > >> WEBCORE_EXPORT void clear(const SecurityOrigin&); > > > > I guess a follow-up patch should do the same for clear(). > > Yes, will look into the clear(origin) version tomorrow. I see this came up. Fine to do that separately. But we need to have the file be gone completely instead of empty (possibly with a journal on disk with non-empty contents)
(In reply to Brady Eidson from comment #7) > (In reply to Chris Dumez from comment #4) > > Comment on attachment 328762 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=328762&action=review > > > > >> Source/WebCore/workers/service/server/SWServer.h:121 > > >> WEBCORE_EXPORT void clear(const SecurityOrigin&); > > > > > > I guess a follow-up patch should do the same for clear(). > > > > Yes, will look into the clear(origin) version tomorrow. > > I see this came up. > > Fine to do that separately. > > But we need to have the file be gone completely instead of empty (possibly > with a journal on disk with non-empty contents) As far as I can tell though, you create the database file eagerly, even if there are no registrations, as soon as we construct a SWServer, no? If I do what you suggest, I need to: 1. Close the database 2. Unlink the database file 3. Then what? The RegistrationDatabase should be in a state where it can process write requests, which currently means creating the database, opening it and creating the table. Or should we start doing this initialization lazily?
Created attachment 328788 [details] Patch
Ok, I now remove the database files entirely. The database file is now also created lazily upon writing the first records.
Comment on attachment 328788 [details] Patch Attachment 328788 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5539540 Number of test failures exceeded the failure limit.
Created attachment 328792 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 328819 [details] Patch
Created attachment 328820 [details] Patch
Created attachment 328822 [details] Patch
Created attachment 328826 [details] Patch
Now also clears service workers for a given origin.
Created attachment 328828 [details] Patch
Created attachment 328835 [details] Patch
Created attachment 328836 [details] Patch
Created attachment 328841 [details] Patch
<rdar://problem/34866077>
Created attachment 328848 [details] Patch
Comment on attachment 328848 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328848&action=review > Source/WebCore/workers/service/server/RegistrationDatabase.cpp:268 > + SQLiteStatement deleteStatement(*m_database, "DELETE FROM Records where topOrigin=?"); topOrigin isn't right, is it? a.com embedded in b.com, a request comes in to delete records for a.com - b.com is the top origin, but we want to clear a.com records. We need "DELETE FROM Records WHERE origin=?" Arguably we could *also* do topOrigin. (Also please not "WHERE" is caps) > Source/WebCore/workers/service/server/SWServer.cpp:-163 > - // FIXME: We should clear entries in m_registrations, m_jobQueues and m_workersByID. This FIXME was removed but do we do this?
Comment on attachment 328848 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328848&action=review >> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:268 >> + SQLiteStatement deleteStatement(*m_database, "DELETE FROM Records where topOrigin=?"); > > topOrigin isn't right, is it? > > a.com embedded in b.com, a request comes in to delete records for a.com - b.com is the top origin, but we want to clear a.com records. > > We need "DELETE FROM Records WHERE origin=?" > Arguably we could *also* do topOrigin. > > (Also please not "WHERE" is caps) Just had this discussion with Youenn. He thinks we should do both topOrigin and scopeOrigin. >> Source/WebCore/workers/service/server/SWServer.cpp:-163 >> - // FIXME: We should clear entries in m_registrations, m_jobQueues and m_workersByID. > > This FIXME was removed but do we do this? This is what I added a few lines above.
Created attachment 328862 [details] Patch
Comment on attachment 328862 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328862&action=review > Source/WebCore/workers/service/ServiceWorkerRegistrationKey.cpp:86 > + return scopeOrigin->isSameOriginAs(origin); This seems unfortunate to create a security origin to check for same origin which is basically scheme/host/port in our case since we only have https URLs. Maybe we can use protocolHostAndPortAreEqual instead? > Source/WebCore/workers/service/server/RegistrationDatabase.cpp:92 > + return FileSystem::pathByAppendingComponent(m_databaseDirectory.isolatedCopy(), databaseFilename()); Why an isolatedCopy() here? If we are to call this numerous time, should we have it as a member of RegistrationDatabase? > Source/WebCore/workers/service/server/RegistrationDatabase.cpp:141 > + openSQLiteDatabase(databasePath); Do we need makeScopeExit, it seems there is only one if statement below. > Source/WebCore/workers/service/server/RegistrationDatabase.cpp:251 > + SQLiteFileSystem::deleteEmptyDatabaseDirectory(m_databaseDirectory.isolatedCopy()); Why do we have to pass an isolated copy? > Source/WebCore/workers/service/server/RegistrationDatabase.h:30 > +#include "SecurityOriginData.h" Is this include needed?
Comment on attachment 328862 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328862&action=review >> Source/WebCore/workers/service/ServiceWorkerRegistrationKey.cpp:86 >> + return scopeOrigin->isSameOriginAs(origin); > > This seems unfortunate to create a security origin to check for same origin which is basically scheme/host/port in our case since we only have https URLs. > Maybe we can use protocolHostAndPortAreEqual instead? Will look into how to optimize this in a follow-up since protocolHostAndPortAreEqual() is not directly reusable (it takes 2 URLs).
Created attachment 328874 [details] Patch
Comment on attachment 328874 [details] Patch Clearing flags on attachment: 328874 Committed r225711: <https://trac.webkit.org/changeset/225711>
All reviewed patches have been landed. Closing bug.