Bug 197510

Summary: -[WKWebsiteDataStore removeDataOfTypes:forDataRecords:completionHandler:] doesn't delete _WKWebsiteDataTypeCredentials
Product: WebKit Reporter: Per Arne Vollan <pvollan>
Component: WebKit Misc.Assignee: Per Arne Vollan <pvollan>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, commit-queue, ews-watchlist, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews213 for win-future
none
Patch
none
Patch
achristensen: review+
Patch commit-queue: commit-queue-

Description Per Arne Vollan 2019-05-02 08:17:45 PDT
WebKit now supports fetching of  _WKWebsiteDataTypeCredentials from -[WKWebsiteDataStore fetchDataRecordsOfTypes:completionHandler:]. However, when deleting all data for a given domain, _WKWebsiteDataTypeCredentials isn't actually deleted.
Comment 1 Per Arne Vollan 2019-05-02 08:18:18 PDT
rdar://problem/50372338
Comment 2 Per Arne Vollan 2019-05-02 08:26:03 PDT
Created attachment 368772 [details]
Patch
Comment 3 Alex Christensen 2019-05-02 08:54:38 PDT
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.
Comment 4 Per Arne Vollan 2019-05-02 10:00:41 PDT
Created attachment 368779 [details]
Patch
Comment 5 EWS Watchlist 2019-05-02 11:35:12 PDT
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
Comment 6 EWS Watchlist 2019-05-02 11:35:30 PDT
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
Comment 7 Per Arne Vollan 2019-05-02 13:23:37 PDT
(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!
Comment 8 WebKit Commit Bot 2019-05-02 13:50:59 PDT
Comment on attachment 368779 [details]
Patch

Clearing flags on attachment: 368779

Committed r244883: <https://trac.webkit.org/changeset/244883>
Comment 9 Per Arne Vollan 2019-05-04 13:24:47 PDT
Created attachment 369073 [details]
Patch
Comment 10 Per Arne Vollan 2019-05-06 09:47:13 PDT
Created attachment 369127 [details]
Patch
Comment 11 Alex Christensen 2019-05-06 12:55:31 PDT
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?
Comment 12 Per Arne Vollan 2019-05-06 13:42:48 PDT
Created attachment 369165 [details]
Patch
Comment 13 Per Arne Vollan 2019-05-06 13:43:22 PDT
(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.
Comment 14 WebKit Commit Bot 2019-05-06 16:07:59 PDT
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 commit-queue@webkit.org...
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
Comment 15 Per Arne Vollan 2019-05-06 16:31:30 PDT
Committed r244988: <https://trac.webkit.org/changeset/244988/webkit>