Bug 197510 – -[WKWebsiteDataStore removeDataOfTypes:forDataRecords:completionHandler:] doesn't delete _WKWebsiteDataTypeCredentials

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Per Arne Vollan]

WebKit now supports fetching of  _WKWebsiteDataTypeCredentials from -[WKWebsiteDataStore fetchDataRecordsOfTypes:completionHandler:]. However, when deleting all data for a given domain, _WKWebsiteDataTypeCredentials isn't actually deleted.

[Per Arne Vollan]

rdar://problem/50372338

[Per Arne Vollan]

Created attachment 368772 [details]
Patch

[Alex Christensen]

Comment on attachment 368772 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368772&action=review

LGTM, can this be used instead of WKProcessPool._removeCredential:... for testing?  If so, we should remove that SPI.  This is much better.

> Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:225
> +void NetworkProcess::removeCredentialsWithOrigins(const Vector<WebCore::SecurityOriginData>& origins, CompletionHandler<void()>&& completionHandler)

Add a stub implementation for non-Cocoa ports.

[Per Arne Vollan]

Created attachment 368779 [details]
Patch

[EWS Watchlist]

Comment on attachment 368779 [details]
Patch

Attachment 368779 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12063563

New failing tests:
http/tests/css/filters-on-iframes.html

[EWS Watchlist]

Created attachment 368797 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Per Arne Vollan]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 368772 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=368772&action=review
> 
> LGTM, can this be used instead of WKProcessPool._removeCredential:... for
> testing?  If so, we should remove that SPI.  This is much better.
> 
> > Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:225
> > +void NetworkProcess::removeCredentialsWithOrigins(const Vector<WebCore::SecurityOriginData>& origins, CompletionHandler<void()>&& completionHandler)
> 
> Add a stub implementation for non-Cocoa ports.

Done. Thanks for reviewing!

[WebKit Commit Bot]

Comment on attachment 368779 [details]
Patch

Clearing flags on attachment: 368779

Committed r244883: <https://trac.webkit.org/changeset/244883>

[Per Arne Vollan]

Created attachment 369073 [details]
Patch

[Per Arne Vollan]

Created attachment 369127 [details]
Patch

[Alex Christensen]

Comment on attachment 369127 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369127&action=review

> Source/WebCore/platform/network/CredentialStorage.cpp:96
> +            || (!origin.port && protectionSpace.port() == 80))

I think this should be indented 4 more spaces.

> Source/WebCore/platform/network/CredentialStorage.cpp:98
> +            || (protectionSpace.serverType() == ProtectionSpaceServerHTTPS && origin.protocol == "https"_s)))

ditto

> Source/WebCore/platform/network/CredentialStorage.cpp:104
> +        auto& partitionName = key.first;
> +        auto& protectionSpace = key.second;
> +        remove(partitionName, protectionSpace);

I don't think these named variables help the readability of this code significantly.

> Source/WebCore/platform/network/CredentialStorage.cpp:130
> +            ASSERT_NOT_REACHED();

'continue' after this?

[Per Arne Vollan]

Created attachment 369165 [details]
Patch

[Per Arne Vollan]

(In reply to Alex Christensen from comment #11)
> Comment on attachment 369127 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369127&action=review
> 
> > Source/WebCore/platform/network/CredentialStorage.cpp:96
> > +            || (!origin.port && protectionSpace.port() == 80))
> 
> I think this should be indented 4 more spaces.
> 
> > Source/WebCore/platform/network/CredentialStorage.cpp:98
> > +            || (protectionSpace.serverType() == ProtectionSpaceServerHTTPS && origin.protocol == "https"_s)))
> 
> ditto
> 
> > Source/WebCore/platform/network/CredentialStorage.cpp:104
> > +        auto& partitionName = key.first;
> > +        auto& protectionSpace = key.second;
> > +        remove(partitionName, protectionSpace);
> 
> I don't think these named variables help the readability of this code
> significantly.
> 
> > Source/WebCore/platform/network/CredentialStorage.cpp:130
> > +            ASSERT_NOT_REACHED();
> 
> 'continue' after this?

Thanks for reviewing! I have updated the patch.

[WebKit Commit Bot]

Comment on attachment 369165 [details]
Patch

Rejecting attachment 369165 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 369165, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=369165&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=197510&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Updating working directory
Processing patch 369165 from bug 197510.
Fetching: https://bugs.webkit.org/attachment.cgi?id=369165
Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/platform/network/CredentialStorage.cpp
	M	Source/WebCore/platform/network/CredentialStorage.h
	M	Source/WebKit/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/Source/WebKit/ChangeLog' is out of date
W: d6b3ee4216596d7128d6d30fec8b6578d3971439 and refs/remotes/origin/master differ, using rebase:
:040000 040000 63bb4b920d5d056164d9523b7f32959febe9bed1 37f8e44c24b5c61f004f476b9a4701216043b9e7 M	Source
:040000 040000 cbf11c4a0646f4c7b6f9f024aaed8273f611ac40 8631ea4989a1a83a6fecc5460e31827a65569146 M	Tools
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/platform/network/CredentialStorage.cpp
	M	Source/WebCore/platform/network/CredentialStorage.h
	M	Source/WebKit/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/Source/WebKit/ChangeLog' is out of date
W: d6b3ee4216596d7128d6d30fec8b6578d3971439 and refs/remotes/origin/master differ, using rebase:
:040000 040000 63bb4b920d5d056164d9523b7f32959febe9bed1 37f8e44c24b5c61f004f476b9a4701216043b9e7 M	Source
:040000 040000 cbf11c4a0646f4c7b6f9f024aaed8273f611ac40 8631ea4989a1a83a6fecc5460e31827a65569146 M	Tools
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
From https://git.webkit.org/git/WebKit
   12dd69fff43..cf351778be6  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 244983 = 12dd69fff43615f4a03d2884921eb9fd9ffdc872
r244984 = cf351778be630d6608655a5ead37202da6437cd6
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: https://webkit-queues.webkit.org/results/12116519

[Per Arne Vollan]

Committed r244988: <https://trac.webkit.org/changeset/244988/webkit>