WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
180558
Clearing all Website Data should remove service worker registrations on disk
https://bugs.webkit.org/show_bug.cgi?id=180558
Summary
Clearing all Website Data should remove service worker registrations on disk
Chris Dumez
Reported
2017-12-07 17:05:17 PST
Clearing all Website Data should remove service worker registrations on disk.
Attachments
WIP Patch
(6.85 KB, patch)
2017-12-07 17:06 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(8.92 KB, patch)
2017-12-07 17:18 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(8.96 KB, patch)
2017-12-07 17:56 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(13.90 KB, patch)
2017-12-07 21:57 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
(2.87 MB, application/zip)
2017-12-07 23:04 PST
,
EWS Watchlist
no flags
Details
Patch
(13.86 KB, patch)
2017-12-08 08:53 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(13.89 KB, patch)
2017-12-08 08:55 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(13.81 KB, patch)
2017-12-08 09:03 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(18.16 KB, patch)
2017-12-08 09:58 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(18.63 KB, patch)
2017-12-08 10:23 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(18.63 KB, patch)
2017-12-08 11:04 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(18.55 KB, patch)
2017-12-08 11:06 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(18.57 KB, patch)
2017-12-08 11:37 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(18.55 KB, patch)
2017-12-08 12:08 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(20.70 KB, patch)
2017-12-08 13:33 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(20.59 KB, patch)
2017-12-08 15:08 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Show Obsolete
(15)
View All
Add attachment
proposed patch, testcase, etc.
Chris Dumez
Comment 1
2017-12-07 17:06:19 PST
Created
attachment 328758
[details]
WIP Patch
Chris Dumez
Comment 2
2017-12-07 17:18:47 PST
Created
attachment 328762
[details]
Patch
youenn fablet
Comment 3
2017-12-07 17:52:30 PST
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.
Chris Dumez
Comment 4
2017-12-07 17:55:34 PST
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.
Chris Dumez
Comment 5
2017-12-07 17:56:37 PST
Created
attachment 328765
[details]
Patch
Brady Eidson
Comment 6
2017-12-07 18:51:01 PST
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.
Brady Eidson
Comment 7
2017-12-07 18:51:51 PST
(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)
Chris Dumez
Comment 8
2017-12-07 20:38:51 PST
(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?
Chris Dumez
Comment 9
2017-12-07 21:57:58 PST
Created
attachment 328788
[details]
Patch
Chris Dumez
Comment 10
2017-12-07 21:58:59 PST
Ok, I now remove the database files entirely. The database file is now also created lazily upon writing the first records.
EWS Watchlist
Comment 11
2017-12-07 23:04:22 PST
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.
EWS Watchlist
Comment 12
2017-12-07 23:04:23 PST
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
Chris Dumez
Comment 13
2017-12-08 08:53:17 PST
Created
attachment 328819
[details]
Patch
Chris Dumez
Comment 14
2017-12-08 08:55:57 PST
Created
attachment 328820
[details]
Patch
Chris Dumez
Comment 15
2017-12-08 09:03:19 PST
Created
attachment 328822
[details]
Patch
Chris Dumez
Comment 16
2017-12-08 09:58:06 PST
Created
attachment 328826
[details]
Patch
Chris Dumez
Comment 17
2017-12-08 09:59:47 PST
Now also clears service workers for a given origin.
Chris Dumez
Comment 18
2017-12-08 10:23:21 PST
Created
attachment 328828
[details]
Patch
Chris Dumez
Comment 19
2017-12-08 11:04:24 PST
Created
attachment 328835
[details]
Patch
Chris Dumez
Comment 20
2017-12-08 11:06:04 PST
Created
attachment 328836
[details]
Patch
Chris Dumez
Comment 21
2017-12-08 11:37:36 PST
Created
attachment 328841
[details]
Patch
Chris Dumez
Comment 22
2017-12-08 11:39:22 PST
<
rdar://problem/34866077
>
Chris Dumez
Comment 23
2017-12-08 12:08:42 PST
Created
attachment 328848
[details]
Patch
Brady Eidson
Comment 24
2017-12-08 12:29:32 PST
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?
Chris Dumez
Comment 25
2017-12-08 12:42:55 PST
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.
Chris Dumez
Comment 26
2017-12-08 13:33:23 PST
Created
attachment 328862
[details]
Patch
youenn fablet
Comment 27
2017-12-08 14:56:10 PST
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?
Chris Dumez
Comment 28
2017-12-08 15:08:06 PST
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).
Chris Dumez
Comment 29
2017-12-08 15:08:48 PST
Created
attachment 328874
[details]
Patch
Chris Dumez
Comment 30
2017-12-08 15:13:54 PST
Comment on
attachment 328874
[details]
Patch Clearing flags on attachment: 328874 Committed
r225711
: <
https://trac.webkit.org/changeset/225711
>
Chris Dumez
Comment 31
2017-12-08 15:13:56 PST
All reviewed patches have been landed. Closing bug.
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