Bug 197510 - -[WKWebsiteDataStore removeDataOfTypes:forDataRecords:completionHandler:] doesn't delete _WKWebsiteDataTypeCredentials
Summary: -[WKWebsiteDataStore removeDataOfTypes:forDataRecords:completionHandler:] doe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-02 08:17 PDT by Per Arne Vollan
Modified: 2019-05-06 16:31 PDT (History)
5 users (show)

See Also:


Attachments
Patch (8.86 KB, patch)
2019-05-02 08:26 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (9.52 KB, patch)
2019-05-02 10:00 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews213 for win-future (13.68 MB, application/zip)
2019-05-02 11:35 PDT, EWS Watchlist
no flags Details
Patch (12.59 KB, patch)
2019-05-04 13:24 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (12.61 KB, patch)
2019-05-06 09:47 PDT, Per Arne Vollan
achristensen: review+
Details | Formatted Diff | Diff
Patch (12.54 KB, patch)
2019-05-06 13:42 PDT, Per Arne Vollan
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>