Bug 128006 – Coalesce authentication credential requests

• Component: Page Loading
• Status: RESOLVED FIXED

[Daniel Bates]

We should look to coalesce authentication credential requests such that we ask the client to display the minimum number of authentication dialogs needed to access the requested resources.

One example of how this issue manifests itself is in the WebKit Bot Watcher's Dashboard with a Buildbot that requires HTTP authentication. At the time of writing, the WebKit Bot Watcher's Dashboard is coded such that each queue makes an independent XML HTTP request to its associated Buildbot for content. Assuming there are at least two queues associated with a Buildbot that requires HTTP authentication and looking only at requests sent by the queues, there may be at most two XML HTTP requests that are made and hence Safari will prompt for credentials twice.

[Alexey Proskuryakov]

This is usually not an issue because main resource tends to be authenticated, and subsequent loads don't start until it loads.

But it can get really annoying for something like the dashboard. Also, reopening windows after browser restart can result in multiple requests for the same credentials, if you had the same site open in multiple windows.

[David Kilzer (:ddkilzer)]

(In reply to comment #1)
> This is usually not an issue because main resource tends to be authenticated, and subsequent loads don't start until it loads.
> 
> But it can get really annoying for something like the dashboard. Also, reopening windows after browser restart can result in multiple requests for the same credentials, if you had the same site open in multiple windows.

This behavior drives me crazy every time I restart Safari.

[Radar WebKit Bug Importer]

<rdar://problem/16839069>

[Chris Dumez]

*** Bug 147496 has been marked as a duplicate of this bug. ***

[Chris Dumez]

Created attachment 257965 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 257965 [details]
Patch

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

> Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:58
> +    // Do not coalesce server trust evaluation requests.

This comment either needs to explain why, or to be dropped.

> Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:73
> +uint64_t AuthenticationManager::addChallengeToHashMap(const WebCore::AuthenticationChallenge& authenticationChallenge)

I'd say addChallengeToChallengeMap. Not a great name, but spelling out the data type in "addChallengeToHashMap" seems even less useful.

> Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:95
> +Vector<uint64_t> AuthenticationManager::coalesceChallenges(uint64_t challengeID) const

This took me awhile to understand - why does a "coalesce" function take a single challenge, what can it coalesce it with?

How would you feel about one of these ideas:
- coalesceChallengesMatching(uint64_t challengeID)
- or coalesceChallenges(const ProtectionSpace&)

[Chris Dumez]

Comment on attachment 257965 [details]
Patch

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

>> Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:58
>> +    // Do not coalesce server trust evaluation requests.
> 
> This comment either needs to explain why, or to be dropped.

Proposing: "Do not coalesce server trust evaluation requests because ProtectionSpace comparison does not evaluate server trust (e.g. certificate)."

>> Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:95
>> +Vector<uint64_t> AuthenticationManager::coalesceChallenges(uint64_t challengeID) const
> 
> This took me awhile to understand - why does a "coalesce" function take a single challenge, what can it coalesce it with?
> 
> How would you feel about one of these ideas:
> - coalesceChallengesMatching(uint64_t challengeID)
> - or coalesceChallenges(const ProtectionSpace&)

I'd prefer the first one. The second one is clear but a bit annoying because we then have to move the HashMap lookup to every call site.

[Chris Dumez]

Created attachment 257977 [details]
Patch

[Chris Dumez]

Comment on attachment 257977 [details]
Patch

Clearing flags on attachment: 257977

Committed r187691: <http://trac.webkit.org/changeset/187691>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Reopening to attach new patch.

[Chris Dumez]

Created attachment 258586 [details]
Patch

[Chris Dumez]

Uploaded a follow-up patch to fix post-mortem review comments from Darin on Bug 147496.

[WebKit Commit Bot]

Comment on attachment 258586 [details]
Patch

Clearing flags on attachment: 258586

Committed r188198: <http://trac.webkit.org/changeset/188198>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.