Bug 180558 – Clearing all Website Data should remove service worker registrations on disk

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Clearing all Website Data should remove service worker registrations on disk.

[Chris Dumez]

Created attachment 328758 [details]
WIP Patch

[Chris Dumez]

Created attachment 328762 [details]
Patch

[youenn fablet]

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 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]

Created attachment 328765 [details]
Patch

[Brady Eidson]

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]

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

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

Created attachment 328788 [details]
Patch

[Chris Dumez]

Ok, I now remove the database files entirely. The database file is now also created lazily upon writing the first records.

[EWS Watchlist]

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]

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]

Created attachment 328819 [details]
Patch

[Chris Dumez]

Created attachment 328820 [details]
Patch

[Chris Dumez]

Created attachment 328822 [details]
Patch

[Chris Dumez]

Created attachment 328826 [details]
Patch

[Chris Dumez]

Now also clears service workers for a given origin.

[Chris Dumez]

Created attachment 328828 [details]
Patch

[Chris Dumez]

Created attachment 328835 [details]
Patch

[Chris Dumez]

Created attachment 328836 [details]
Patch

[Chris Dumez]

Created attachment 328841 [details]
Patch

[Chris Dumez]

<rdar://problem/34866077>

[Chris Dumez]

Created attachment 328848 [details]
Patch

[Brady Eidson]

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 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]

Created attachment 328862 [details]
Patch

[youenn fablet]

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 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]

Created attachment 328874 [details]
Patch

[Chris Dumez]

Comment on attachment 328874 [details]
Patch

Clearing flags on attachment: 328874

Committed r225711: <https://trac.webkit.org/changeset/225711>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.