Bug 198854

Created attachment 372113 [details] Patch

Created attachment 372126 [details] Patch

Comment on attachment 372126 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372126&action=review I think this looks very good! Leaving the final review to Geoff or Alex, since they are experts in this area :) > Source/WebCore/ChangeLog:5 > + -[WKWebsiteDataStore removeDataOfTypes:modifiedSince:completionHandler:] doesn't delete _WKWebsiteDataTypeCredentials > + https://bugs.webkit.org/show_bug.cgi?id=198854 > + Add radar?

Comment on attachment 372126 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372126&action=review > Source/WebCore/platform/network/mac/CredentialStorageMac.mm:41 > +Vector<WebCore::SecurityOriginData> CredentialStorage::getOriginsWithPersistentCredentials() We usually only use "get" in a function name to indicate that it returns its value through an out parameter. So, this should remain "originsWithPersistentCredentials()". > Source/WebCore/platform/network/mac/CredentialStorageMac.mm:53 > + auto allCredentials = [[NSURLCredentialStorage sharedCredentialStorage] allCredentials]; You should move this line out of the loop so that you don't have to call it for each origin. Also, I think this is a memory leak. -allCredentials returns a dictionary by copy. If you don't release the dictionary, it will leak. You can avoid memory leaks by using smart pointers. The best way to use a smart pointer here is probably auto allCredentials = adoptNS([[NSURLCredentialStorage sharedCredentialStorage] allCredentials]); You should also put sharedCredentialStorage in a local variable to avoid re-fetching it. > Source/WebCore/platform/network/mac/CredentialStorageMac.mm:71 > + removePersistentCredentialsWithOrigins(getOriginsWithPersistentCredentials()); Can you do something more efficient here, since we don't need to match against a set of origins, and we can just remove all credentials? > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1285 > + if (!sessionID.isEphemeral()) { Why do we avoid returning this data for ephemeral sessions? > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1369 > + if (!sessionID.isEphemeral()) > + WebCore::CredentialStorage::clearPersistentCredentials(); Why do we avoid deleting website data for ephemeral sessions? > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1508 > + if (!sessionID.isEphemeral()) > + WebCore::CredentialStorage::removePersistentCredentialsWithOrigins(originDatas); Ditto.

<rdar://problem/51386058>

(In reply to Geoffrey Garen from comment #4) > Comment on attachment 372126 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=372126&action=review > > > Source/WebCore/platform/network/mac/CredentialStorageMac.mm:41 > > +Vector<WebCore::SecurityOriginData> CredentialStorage::getOriginsWithPersistentCredentials() > > We usually only use "get" in a function name to indicate that it returns its > value through an out parameter. So, this should remain > "originsWithPersistentCredentials()". > Okay. > > Source/WebCore/platform/network/mac/CredentialStorageMac.mm:53 > > + auto allCredentials = [[NSURLCredentialStorage sharedCredentialStorage] allCredentials]; > > You should move this line out of the loop so that you don't have to call it > for each origin. > > Also, I think this is a memory leak. -allCredentials returns a dictionary by > copy. If you don't release the dictionary, it will leak. You can avoid > memory leaks by using smart pointers. The best way to use a smart pointer > here is probably > > auto allCredentials = adoptNS([[NSURLCredentialStorage > sharedCredentialStorage] allCredentials]); > > You should also put sharedCredentialStorage in a local variable to avoid > re-fetching it. > Okay. > > Source/WebCore/platform/network/mac/CredentialStorageMac.mm:71 > > + removePersistentCredentialsWithOrigins(getOriginsWithPersistentCredentials()); > > Can you do something more efficient here, since we don't need to match > against a set of origins, and we can just remove all credentials? > Since we don't have API for clear all credentials, we can do similar thing as removePersistentCredentialsWithOrigins. > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1285 > > + if (!sessionID.isEphemeral()) { > > Why do we avoid returning this data for ephemeral sessions? > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1369 > > + if (!sessionID.isEphemeral()) > > + WebCore::CredentialStorage::clearPersistentCredentials(); > > Why do we avoid deleting website data for ephemeral sessions? > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1508 > > + if (!sessionID.isEphemeral()) > > + WebCore::CredentialStorage::removePersistentCredentialsWithOrigins(originDatas); > > Ditto. This is the old behavior and I didn't change. My understanding is [NSURLCredentialStorage sharedCredentialStorage] is used for credentials shared across different apps(?), and network process keeps a copy of credentials from ephemeral session in memory with maps in WebCore::CredentialStorage.

Created attachment 372142 [details] Patch

Comment on attachment 372142 [details] Patch r=me

Created attachment 372306 [details] Patch for landing

Comment on attachment 372306 [details] Patch for landing Clearing flags on attachment: 372306 Committed r246530: <https://trac.webkit.org/changeset/246530>

All reviewed patches have been landed. Closing bug.