Allow dynamic updating of authentication policies

Allow dynamic updating of authentication policies

This allows a number of the authentication policies to be updated
dynamically after the IO thread has been created.

BUG=545534
Committed: https://crrev.com/bba302d0ced354b71853bbf3582f4034ea825b9b
Cr-Commit-Position: refs/heads/master@{#362940}

[aberent, 2015-10-20 17:28:50]

aberent@chromium.org changed reviewers:
+ bauerb@chromium.org

[aberent, 2015-10-20 17:28:50]

Initial version. Comments welcome, although I still need to (at least) work out
how to test this.

[Bernhard Bauer, 2015-10-21 09:51:10]

Can you specify the policies that can now be dynamically updated in the
description?

https://codereview.chromium.org/1414313002/diff/1/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1414313002/diff/1/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:467:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO));
Extract this to a local variable so you can reuse it?

https://codereview.chromium.org/1414313002/diff/1/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/1414313002/diff/1/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:499: // No PrefMember for the GSSAPI library name,
since changing it after startup
Add an empty line before the comment please.

https://codereview.chromium.org/1414313002/diff/1/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:501: // of problems for active Negotiate
transactions.
Nit: s/ for active Negotiate transactions// -- there would be other problems as
well 😉

https://codereview.chromium.org/1414313002/diff/1/net/http/http_auth_handler_...
File net/http/http_auth_handler_factory.cc (right):

https://codereview.chromium.org/1414313002/diff/1/net/http/http_auth_handler_...
net/http/http_auth_handler_factory.cc:195: HttpAuthHandlerFactory* ntlm_factory
= GetSchemeFactory("ntml");
ntlm! Would it make sense to extract these to constants?

https://codereview.chromium.org/1414313002/diff/1/net/http/http_auth_handler_...
net/http/http_auth_handler_factory.cc:207:
(HttpAuthHandlerNegotiate::Factory*)GetSchemeFactory("negotiate");
Use static_cast<...>() instead of C-style casts.

[aberent, 2015-10-22 17:57:03]

https://codereview.chromium.org/1414313002/diff/1/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1414313002/diff/1/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:467:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO));
On 2015/10/21 09:51:10, Bernhard Bauer wrote:
> Extract this to a local variable so you can reuse it?

Done.

https://codereview.chromium.org/1414313002/diff/1/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/1414313002/diff/1/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:499: // No PrefMember for the GSSAPI library name,
since changing it after startup
On 2015/10/21 09:51:10, Bernhard Bauer wrote:
> Add an empty line before the comment please.

Done.

https://codereview.chromium.org/1414313002/diff/1/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:501: // of problems for active Negotiate
transactions.
On 2015/10/21 09:51:10, Bernhard Bauer wrote:
> Nit: s/ for active Negotiate transactions// -- there would be other problems
as
> well 😉
Changed to "for, for example, active Negotiate transactions".

https://codereview.chromium.org/1414313002/diff/1/net/http/http_auth_handler_...
File net/http/http_auth_handler_factory.cc (right):

https://codereview.chromium.org/1414313002/diff/1/net/http/http_auth_handler_...
net/http/http_auth_handler_factory.cc:195: HttpAuthHandlerFactory* ntlm_factory
= GetSchemeFactory("ntml");
On 2015/10/21 09:51:10, Bernhard Bauer wrote:
> ntlm! Would it make sense to extract these to constants?

Done.

https://codereview.chromium.org/1414313002/diff/1/net/http/http_auth_handler_...
net/http/http_auth_handler_factory.cc:207:
(HttpAuthHandlerNegotiate::Factory*)GetSchemeFactory("negotiate");
On 2015/10/21 09:51:10, Bernhard Bauer wrote:
> Use static_cast<...>() instead of C-style casts.

Done.

[Bernhard Bauer, 2015-10-23 08:15:41]

Nice!

https://codereview.chromium.org/1414313002/diff/20001/chrome/browser/io_threa...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:42:
pref_registry->RegisterDictionaryPref(proxy_config::prefs::kProxy);
Call PrefProxyConfigTrackerImpl::RegisterPrefs()?

https://codereview.chromium.org/1414313002/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:55: ~IOThreadPeer() {
io_thread_->CleanUp(); }
Is this clang-formatted? If not, I would move this to a separate line.

https://codereview.chromium.org/1414313002/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:89: scoped_ptr<TestingPrefServiceSimple>
pref_service_;
You could probably make this a direct member, so you'll save a line to
initialize it ☺

https://codereview.chromium.org/1414313002/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:93: scoped_ptr<IOThread> io_thread_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1414313002/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:593:
peer.pref_service()->SetString(prefs::kAuthServerWhitelist, "");
Use an empty std::string() constructor to save a couple of instructions.

https://codereview.chromium.org/1414313002/diff/20001/net/http/http_auth_hand...
File net/http/http_auth_handler_factory.cc (right):

https://codereview.chromium.org/1414313002/diff/20001/net/http/http_auth_hand...
net/http/http_auth_handler_factory.cc:22: static const char kBasic[] = "basic";
Static isn't necessary here, as const variables automatically get internal
linkage (and in any case, anonymous namespaces are prefered over static).

[aberent, 2015-10-23 15:40:33]

https://codereview.chromium.org/1414313002/diff/20001/chrome/browser/io_threa...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:42:
pref_registry->RegisterDictionaryPref(proxy_config::prefs::kProxy);
On 2015/10/23 08:15:41, Bernhard Bauer wrote:
> Call PrefProxyConfigTrackerImpl::RegisterPrefs()?

Done. Thanks, I was looking for something of the sort, but looked in the wrong
place.

https://codereview.chromium.org/1414313002/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:55: ~IOThreadPeer() {
io_thread_->CleanUp(); }
On 2015/10/23 08:15:41, Bernhard Bauer wrote:
> Is this clang-formatted? If not, I would move this to a separate line.

Yes, this is clang formatted; would not have been my choice.

https://codereview.chromium.org/1414313002/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:89: scoped_ptr<TestingPrefServiceSimple>
pref_service_;
On 2015/10/23 08:15:41, Bernhard Bauer wrote:
> You could probably make this a direct member, so you'll save a line to
> initialize it ☺

Done.

https://codereview.chromium.org/1414313002/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:93: scoped_ptr<IOThread> io_thread_;
On 2015/10/23 08:15:41, Bernhard Bauer wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1414313002/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:593:
peer.pref_service()->SetString(prefs::kAuthServerWhitelist, "");
On 2015/10/23 08:15:41, Bernhard Bauer wrote:
> Use an empty std::string() constructor to save a couple of instructions.
Instruction level performance is hardly and issue here, and doing this makes it
marginally less readable, especially since, in the UpdateDelegateWhitelist test
it makes the line longer than 80 chars.

https://codereview.chromium.org/1414313002/diff/20001/net/http/http_auth_hand...
File net/http/http_auth_handler_factory.cc (right):

https://codereview.chromium.org/1414313002/diff/20001/net/http/http_auth_hand...
net/http/http_auth_handler_factory.cc:22: static const char kBasic[] = "basic";
On 2015/10/23 08:15:41, Bernhard Bauer wrote:
> Static isn't necessary here, as const variables automatically get internal
> linkage (and in any case, anonymous namespaces are prefered over static).

Done.

[Bernhard Bauer, 2015-10-23 15:57:59]

lgtm

https://codereview.chromium.org/1414313002/diff/20001/chrome/browser/io_threa...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:55: ~IOThreadPeer() {
io_thread_->CleanUp(); }
On 2015/10/23 15:40:33, aberent wrote:
> On 2015/10/23 08:15:41, Bernhard Bauer wrote:
> > Is this clang-formatted? If not, I would move this to a separate line.
> 
> Yes, this is clang formatted; would not have been my choice.

Hm. Could we out-of-line this so clang-format will be happy?

[aberent, 2015-10-23 16:39:25]

aberent@chromium.org changed reviewers:
+ cbentzel@chromium.org, mlerman@chromium.org, zea@chromium.org

[aberent, 2015-10-23 16:39:25]

Already reviewed by bauerb@chromium.org.

zea@chromium.org: Owner review of mcs_probe.cc please

mlerman@chromium.org: Owner review of chrome/browser/profiles please.

cbentzel@chromium.org: Owner review of IOThread* and net/* please

[Mike Lerman, 2015-10-23 16:50:44]

https://codereview.chromium.org/1414313002/diff/50001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/1414313002/diff/50001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:59: #include
"net/http/http_auth_handler_factory.h"
You didn't edit the includes of any of the other header files, nor did you make
any other changes do this file - why is the #include for
http_auth_handler_factory now required?

(here and OTR_profile_io_data)

[cbentzel, 2015-10-23 16:54:30]

Description was changed from

==========
Allow dynamic updating of authentication policies

This allows a number of the authentication policies to be updated
dynamically after the IO thread has been created.

BUG=545534
==========

to

==========
Allow dynamic updating of authentication policies

This allows a number of the authentication policies to be updated
dynamically after the IO thread has been created.

BUG=545534
==========

[cbentzel, 2015-10-23 16:54:30]

cbentzel@chromium.org changed reviewers:
+ asanka@chromium.org

[cbentzel, 2015-10-23 16:54:41]

On 2015/10/23 16:50:44, Mike Lerman wrote:
>
https://codereview.chromium.org/1414313002/diff/50001/chrome/browser/profiles...
> File chrome/browser/profiles/profile_impl_io_data.cc (right):
> 
>
https://codereview.chromium.org/1414313002/diff/50001/chrome/browser/profiles...
> chrome/browser/profiles/profile_impl_io_data.cc:59: #include
> "net/http/http_auth_handler_factory.h"
> You didn't edit the includes of any of the other header files, nor did you
make
> any other changes do this file - why is the #include for
> http_auth_handler_factory now required?
> 
> (here and OTR_profile_io_data)

Adding asanka in place of me.

[aberent, 2015-10-23 17:22:28]

https://codereview.chromium.org/1414313002/diff/50001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/1414313002/diff/50001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:59: #include
"net/http/http_auth_handler_factory.h"
On 2015/10/23 16:50:44, Mike Lerman wrote:
> You didn't edit the includes of any of the other header files, nor did you
make
> any other changes do this file - why is the #include for
> http_auth_handler_factory now required?
> 
> (here and OTR_profile_io_data)

Because I had to change the type of IOThread::Globals::http_auth_handler_factory
to scoped_ptr<net::HttpAuthHandlerRegistryFactory> (from
scoped_ptr<net::HttpAuthHandlerFactory>). As a result it now needs to know that
HttpAuthHandlerRegistryFactory derives from HttpAuthHandlerFactory, which needs
the full definition, so needs the header file.

Without this change line 496 which calls http_auth_handler_factory.get() gives
compilation errors.

Similarly in OTR_profile_io_data.

[Mike Lerman, 2015-10-23 17:24:37]

On 2015/10/23 17:22:28, aberent wrote:
>
https://codereview.chromium.org/1414313002/diff/50001/chrome/browser/profiles...
> File chrome/browser/profiles/profile_impl_io_data.cc (right):
> 
>
https://codereview.chromium.org/1414313002/diff/50001/chrome/browser/profiles...
> chrome/browser/profiles/profile_impl_io_data.cc:59: #include
> "net/http/http_auth_handler_factory.h"
> On 2015/10/23 16:50:44, Mike Lerman wrote:
> > You didn't edit the includes of any of the other header files, nor did you
> make
> > any other changes do this file - why is the #include for
> > http_auth_handler_factory now required?
> > 
> > (here and OTR_profile_io_data)
> 
> Because I had to change the type of
IOThread::Globals::http_auth_handler_factory
> to scoped_ptr<net::HttpAuthHandlerRegistryFactory> (from
> scoped_ptr<net::HttpAuthHandlerFactory>). As a result it now needs to know
that
> HttpAuthHandlerRegistryFactory derives from HttpAuthHandlerFactory, which
needs
> the full definition, so needs the header file.
> 
> Without this change line 496 which calls http_auth_handler_factory.get() gives
> compilation errors.
> 
> Similarly in OTR_profile_io_data.

Thanks for that!

c/b/profiles LGTM

[Nicolas Zea, 2015-10-23 20:26:57]

mcs_probe lgtm

[asanka, 2015-10-26 15:11:54]

https://codereview.chromium.org/1414313002/diff/50001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1414313002/diff/50001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1007: globals_->url_security_manager.reset(
This isn't safe anymore since that UpdateSecurityManager() can be called while
there are active on-going auth handshakes. The AuthHandlers require that the
URLSecurityManager outlive the handler.

HttpAuthHandlerNegotiate, for example, keeps a pointer to the URLSecurityManager
which would end up pointing to a freed object after this reset() call. I see you
are updating the security manager for the factories below, but that doesn't take
existing AuthHandlers into account.

https://codereview.chromium.org/1414313002/diff/50001/net/http/http_auth_hand...
File net/http/http_auth_handler_negotiate.h (right):

https://codereview.chromium.org/1414313002/diff/50001/net/http/http_auth_hand...
net/http/http_auth_handler_negotiate.h:80: AuthLibrary* get_library() { return
auth_library_.get(); }
Nit: library().

[aberent, 2015-10-27 19:32:00]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-27 19:33:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414313002/70001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414313002/70001

[aberent, 2015-10-27 19:34:13]

https://codereview.chromium.org/1414313002/diff/50001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1414313002/diff/50001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1007: globals_->url_security_manager.reset(
On 2015/10/26 15:11:54, asanka wrote:
> This isn't safe anymore since that UpdateSecurityManager() can be called while
> there are active on-going auth handshakes. The AuthHandlers require that the
> URLSecurityManager outlive the handler.
> 
> HttpAuthHandlerNegotiate, for example, keeps a pointer to the
URLSecurityManager
> which would end up pointing to a freed object after this reset() call. I see
you
> are updating the security manager for the factories below, but that doesn't
take
> existing AuthHandlers into account.

Fixed by keeping the security manager and updating the whitelists within it.

[commit-bot: I haz the power, 2015-10-27 19:43:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-27 19:43:20]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[aberent, 2015-10-27 21:40:11]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-27 21:42:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414313002/90001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414313002/90001

[aberent, 2015-10-27 21:50:44]

https://codereview.chromium.org/1414313002/diff/50001/net/http/http_auth_hand...
File net/http/http_auth_handler_negotiate.h (right):

https://codereview.chromium.org/1414313002/diff/50001/net/http/http_auth_hand...
net/http/http_auth_handler_negotiate.h:80: AuthLibrary* get_library() { return
auth_library_.get(); }
On 2015/10/26 15:11:54, asanka wrote:
> Nit: library().

Done.

[commit-bot: I haz the power, 2015-10-27 22:19:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-27 22:19:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[asanka, 2015-10-28 03:27:43]

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:482: auth_android_negotiate_account_type_.Init(
Can we condition out the Android code?

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1001: if (!server_whitelist.empty()) {
It should be possible to set the whitelist to an empty string (effectively
removing any entries that were there before).

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1003: new
net::HttpAuthFilterWhitelist(server_whitelist));
Let's use scoped_ptrs<> to transfer ownership like you've used elsewhere.

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1009: if (!delegate_whitelist.empty()) {
Ditto. It's valid to set the whitelist to an empty string.

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1033: auth_schemes_, ",", base::TRIM_WHITESPACE,
base::SPLIT_WANT_ALL);
Pre-existing, but change this to specify base::SPLIT_WANT_NONEMPTY. An empty
string is not a valid scheme.

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1042:
globals_->http_auth_handler_factory->SetSecurityManager(
Let's move this up so that the Update*s are all in a group together. It should
be safe to do so.

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.h:492: // TODO(aberent) Make the list of auth schemes a
PrefMember, so that the
Could file a bug for this and add a reference here?

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:39: #if defined(ENABLE_EXTENSIONS)
Not an owner here, but this looks bad enough that I'd consider initializing this
in the constructor body.

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:94:
scoped_refptr<extensions::EventRouterForwarder> event_router_forwarder_;
Guard with #if defined(ENABLE_EXTENSIONS)

https://codereview.chromium.org/1414313002/diff/90001/net/http/http_auth_hand...
File net/http/http_auth_handler_factory.cc (right):

https://codereview.chromium.org/1414313002/diff/90001/net/http/http_auth_hand...
net/http/http_auth_handler_factory.cc:22: const char kBasic[] = "basic";
Move these to http_auth.h where we have the constants. Also http_auth.cc already
defines these strings. Might as well use them.

https://codereview.chromium.org/1414313002/diff/90001/net/http/http_auth_hand...
net/http/http_auth_handler_factory.cc:66: #if defined(OS_POSIX)
Preexisting: For #if/#elif chains for platform selection, my preference is
always to put OS_POSIX last since it's so all encompassing (equivalent to
!OS_WIN). It's pretty much the #else clause.

https://codereview.chromium.org/1414313002/diff/90001/net/http/http_auth_hand...
net/http/http_auth_handler_factory.cc:161: #if defined(OS_POSIX) &&
!defined(OS_ANDROID)
Same comment as above regarding OS_POSIX

https://codereview.chromium.org/1414313002/diff/90001/net/http/http_auth_hand...
net/http/http_auth_handler_factory.cc:234: auto negotiate_factory =
static_cast<HttpAuthHandlerNegotiate::Factory*>(
These static casts (here and elsewhere) are not compatible with the API that
HttpAuthHandlerRegistryFactory exposes. It's possible to call
HttpAuthHandlerRegistryFactory::RegisterSchemeFactory() and replace the
negotiate or ntlm handler with something that's is-a HttpAuthHandlerFactory, but
not is-a HttpAuthHandlerNegotiate::Factory.

https://codereview.chromium.org/1414313002/diff/90001/net/http/http_auth_hand...
File net/http/http_auth_handler_factory.h (right):

https://codereview.chromium.org/1414313002/diff/90001/net/http/http_auth_hand...
net/http/http_auth_handler_factory.h:187: void
SetSecurityManager(URLSecurityManager* security_manager);
Let's move this back to the constructor since we've determined that the security
manager can't be safely changed after the factory is created.

https://codereview.chromium.org/1414313002/diff/90001/net/http/http_auth_hand...
File net/http/http_auth_handler_negotiate_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/90001/net/http/http_auth_hand...
net/http/http_auth_handler_negotiate_unittest.cc:54:
factory_->set_library(make_scoped_ptr(auth_library_));
Preexisting: This is dangerous territory. Can we avoid passing ownership while
keeping a reference?

[aberent, 2015-10-29 20:36:46]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-29 20:37:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414313002/110001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414313002/110001

[commit-bot: I haz the power, 2015-10-29 21:21:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-29 21:21:52]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[aberent, 2015-11-02 18:43:16]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-02 18:44:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414313002/130001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414313002/130001

[aberent, 2015-11-02 18:52:49]

aberent@chromium.org changed reviewers:
+ pkasting@chromium.org

[aberent, 2015-11-02 18:52:50]

pkasting@ -Please review, as OWNER, changes to login_prompt.cc

Others - PTAL; additional files are mainly for adding constants for
authentication type strings.

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:482: auth_android_negotiate_account_type_.Init(
On 2015/10/28 03:27:43, asanka wrote:
> Can we condition out the Android code?

Not sure about this; should we also condition out the non-Android, Posix
specific code (gssapi_library_name is only used on non-Android Posix systems)?
In fact you could argue that much of this should be within an #if
defined(USE_KERBEROS).

I don't like conditioning out code excessively. If you leave it in
unconditionally then, if people make a change they at least compile it locally,
and find the compilation errors locally (even if it can't be tested locally)
whereas if it is only compiled on other platforms they are likely to not to find
the compilation errors until they send it to the trybots.

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1001: if (!server_whitelist.empty()) {
On 2015/10/28 03:27:43, asanka wrote:
> It should be possible to set the whitelist to an empty string (effectively
> removing any entries that were there before).

Done.

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1003: new
net::HttpAuthFilterWhitelist(server_whitelist));
On 2015/10/28 03:27:43, asanka wrote:
> Let's use scoped_ptrs<> to transfer ownership like you've used elsewhere.

Done.

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1009: if (!delegate_whitelist.empty()) {
On 2015/10/28 03:27:43, asanka wrote:
> Ditto. It's valid to set the whitelist to an empty string.

Done.

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1033: auth_schemes_, ",", base::TRIM_WHITESPACE,
base::SPLIT_WANT_ALL);
On 2015/10/28 03:27:43, asanka wrote:
> Pre-existing, but change this to specify base::SPLIT_WANT_NONEMPTY. An empty
> string is not a valid scheme.

Done.

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1042:
globals_->http_auth_handler_factory->SetSecurityManager(
On 2015/10/28 03:27:43, asanka wrote:
> Let's move this up so that the Update*s are all in a group together. It should
> be safe to do so.

Actually no longer applies, since now moved back into the constructor, as you
suggested elsewhere.

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.h:492: // TODO(aberent) Make the list of auth schemes a
PrefMember, so that the
On 2015/10/28 03:27:43, asanka wrote:
> Could file a bug for this and add a reference here?

Done.

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:39: #if defined(ENABLE_EXTENSIONS)
On 2015/10/28 03:27:43, asanka wrote:
> Not an owner here, but this looks bad enough that I'd consider initializing
this
> in the constructor body.

Done.

https://codereview.chromium.org/1414313002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:94:
scoped_refptr<extensions::EventRouterForwarder> event_router_forwarder_;
On 2015/10/28 03:27:43, asanka wrote:
> Guard with #if defined(ENABLE_EXTENSIONS)

Doing so means that one needs #ifs in the IOThread constructor call, so simpler
to leave it unguarded. The class EventRouteForwarder must exist unconditionally,
since the IO thread constructor has an argument of this type.

https://codereview.chromium.org/1414313002/diff/90001/net/http/http_auth_hand...
File net/http/http_auth_handler_factory.cc (right):

https://codereview.chromium.org/1414313002/diff/90001/net/http/http_auth_hand...
net/http/http_auth_handler_factory.cc:22: const char kBasic[] = "basic";
On 2015/10/28 03:27:43, asanka wrote:
> Move these to http_auth.h where we have the constants. Also http_auth.cc
already
> defines these strings. Might as well use them.

Done.

https://codereview.chromium.org/1414313002/diff/90001/net/http/http_auth_hand...
net/http/http_auth_handler_factory.cc:66: #if defined(OS_POSIX)
On 2015/10/28 03:27:43, asanka wrote:
> Preexisting: For #if/#elif chains for platform selection, my preference is
> always to put OS_POSIX last since it's so all encompassing (equivalent to
> !OS_WIN). It's pretty much the #else clause.

Done.

https://codereview.chromium.org/1414313002/diff/90001/net/http/http_auth_hand...
net/http/http_auth_handler_factory.cc:161: #if defined(OS_POSIX) &&
!defined(OS_ANDROID)
On 2015/10/28 03:27:43, asanka wrote:
> Same comment as above regarding OS_POSIX

Done.

https://codereview.chromium.org/1414313002/diff/90001/net/http/http_auth_hand...
net/http/http_auth_handler_factory.cc:234: auto negotiate_factory =
static_cast<HttpAuthHandlerNegotiate::Factory*>(
On 2015/10/28 03:27:43, asanka wrote:
> These static casts (here and elsewhere) are not compatible with the API that
> HttpAuthHandlerRegistryFactory exposes. It's possible to call
> HttpAuthHandlerRegistryFactory::RegisterSchemeFactory() and replace the
> negotiate or ntlm handler with something that's is-a HttpAuthHandlerFactory,
but
> not is-a HttpAuthHandlerNegotiate::Factory.

Done. Every HttpAuthHandlerFactory now has a SetNegotiateEnablePort method (and
similar for the other cases), which, by default, does nothing. It is overridden
here and in the negotiate factory.

https://codereview.chromium.org/1414313002/diff/90001/net/http/http_auth_hand...
File net/http/http_auth_handler_factory.h (right):

https://codereview.chromium.org/1414313002/diff/90001/net/http/http_auth_hand...
net/http/http_auth_handler_factory.h:187: void
SetSecurityManager(URLSecurityManager* security_manager);
On 2015/10/28 03:27:43, asanka wrote:
> Let's move this back to the constructor since we've determined that the
security
> manager can't be safely changed after the factory is created.

Done.

https://codereview.chromium.org/1414313002/diff/90001/net/http/http_auth_hand...
File net/http/http_auth_handler_negotiate_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/90001/net/http/http_auth_hand...
net/http/http_auth_handler_negotiate_unittest.cc:54:
factory_->set_library(make_scoped_ptr(auth_library_));
On 2015/10/28 03:27:43, asanka wrote:
> Preexisting: This is dangerous territory. Can we avoid passing ownership while
> keeping a reference?

Not without significant refactoring of the tests (probably taking this out of
setup and doing it in the individual tests). This isn't new, the factory
internally always took ownership; it simply didn't declare in its interface that
it was going to (i.e. it took a straight pointer that it then used internally to
update a scoped pointer, see the old comment at line 227). As such I think
fixing this should be a different CL.

[commit-bot: I haz the power, 2015-11-02 19:19:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-02 19:19:04]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Peter Kasting, 2015-11-02 19:28:56]

LGTM

[aberent, 2015-11-02 22:06:28]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-02 22:08:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414313002/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414313002/150001

[commit-bot: I haz the power, 2015-11-02 22:27:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-02 22:27:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aberent, 2015-11-03 09:46:17]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-03 09:46:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414313002/170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414313002/170001

[commit-bot: I haz the power, 2015-11-03 12:04:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-03 12:05:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[aberent, 2015-11-04 11:03:35]

On 2015/11/03 12:05:00, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

Testing updating issue. Please ignore.

[aberent, 2015-11-04 12:19:47]

On 2015/11/04 11:03:35, aberent wrote:
> On 2015/11/03 12:05:00, commit-bot: I haz the power wrote:
> > Dry run: Try jobs failed on following builders:
> >   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
> 
> Testing updating issue. Please ignore.

asanka@ - PTAL, I would like to get this landed.

[asanka, 2015-11-04 16:05:45]

https://codereview.chromium.org/1414313002/diff/170001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1414313002/diff/170001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:1020: void
IOThread::UpdateAndroidAuthNegotiateAccount() {
Shouldn't this be UpdateAndroidAuthNegotiateAccountType() ?

https://codereview.chromium.org/1414313002/diff/170001/chrome/browser/io_thre...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/170001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:41: class IOThreadPeer {
Shall we maintain IOThreadPeer as a pure peer? It currently owns a thread bundle
and an IOThread object, which are better off being owned by the test fixture.

https://codereview.chromium.org/1414313002/diff/170001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:575: IOThreadPeer peer;
Exercising the thread restrictions is important given the behavior that's being
introduced in this CL. Pref changes on the UI thread should propagate to auth
stack changes on the IO thread. Regression in this area might not be caught by
any other test since it requires specifically changing the auth related prefs in
the presence of real threads.

Let's use a real IO thread instead of one that's tied to the same message loop
as the UI thread. I.e. use the REAL_IO_THREAD option when initializing the
thread bundle. You may need to flush the IO thread before the changes become
visible.

https://codereview.chromium.org/1414313002/diff/170001/net/http/http_auth.h
File net/http/http_auth.h (right):

https://codereview.chromium.org/1414313002/diff/170001/net/http/http_auth.h#n...
net/http/http_auth.h:26: const char kBasicAuthScheme[] = "basic";
These constants have internal linkage (see http://goto/totw:105 and
http://en.cppreference.com/w/cpp/language/storage_duration). Let's move them
inside HttpAuth and make them static (i.e. "class HttpAuth { public: static
const char kBasicAuthScheme[]; ...}" along with definitions in http_auth.cc.
This is consistent with how //net has historically given symbolic names to
tokens (see http_request_headers.h for example).

https://codereview.chromium.org/1414313002/diff/170001/net/http/url_security_...
File net/http/url_security_manager.h (right):

https://codereview.chromium.org/1414313002/diff/170001/net/http/url_security_...
net/http/url_security_manager.h:41: // TODO(cbentzel): Perhaps it's better to
make a non-abstract HttpAuthFilter
Might as well remove this comment since it no longer makes sense.

https://codereview.chromium.org/1414313002/diff/170001/net/http/url_security_...
File net/http/url_security_manager_win.cc (right):

https://codereview.chromium.org/1414313002/diff/170001/net/http/url_security_...
net/http/url_security_manager_win.cc:121: // If we have a whitelist, just use
that.
Comment no longer accurate.

[aberent, 2015-11-06 13:57:34]

https://codereview.chromium.org/1414313002/diff/170001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1414313002/diff/170001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:1020: void
IOThread::UpdateAndroidAuthNegotiateAccount() {
On 2015/11/04 16:05:44, asanka wrote:
> Shouldn't this be UpdateAndroidAuthNegotiateAccountType() ?

Done.

https://codereview.chromium.org/1414313002/diff/170001/chrome/browser/io_thre...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/170001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:41: class IOThreadPeer {
On 2015/11/04 16:05:44, asanka wrote:
> Shall we maintain IOThreadPeer as a pure peer? It currently owns a thread
bundle
> and an IOThread object, which are better off being owned by the test fixture.

Done.

https://codereview.chromium.org/1414313002/diff/170001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:575: IOThreadPeer peer;
On 2015/11/04 16:05:44, asanka wrote:
> Exercising the thread restrictions is important given the behavior that's
being
> introduced in this CL. Pref changes on the UI thread should propagate to auth
> stack changes on the IO thread. Regression in this area might not be caught by
> any other test since it requires specifically changing the auth related prefs
in
> the presence of real threads.
> 
> Let's use a real IO thread instead of one that's tied to the same message loop
> as the UI thread. I.e. use the REAL_IO_THREAD option when initializing the
> thread bundle. You may need to flush the IO thread before the changes become
> visible.

Done.

https://codereview.chromium.org/1414313002/diff/170001/net/http/http_auth.h
File net/http/http_auth.h (right):

https://codereview.chromium.org/1414313002/diff/170001/net/http/http_auth.h#n...
net/http/http_auth.h:26: const char kBasicAuthScheme[] = "basic";
On 2015/11/04 16:05:44, asanka wrote:
> These constants have internal linkage (see http://goto/totw:105 and
> http://en.cppreference.com/w/cpp/language/storage_duration). Let's move them
> inside HttpAuth and make them static (i.e. "class HttpAuth { public: static
> const char kBasicAuthScheme[]; ...}" along with definitions in http_auth.cc.
> This is consistent with how //net has historically given symbolic names to
> tokens (see http_request_headers.h for example).

Moved to separate header and source files. As discussed, putting these in
HttpAuth adds unnecessary dependencies and code to libnet_nacl.

https://codereview.chromium.org/1414313002/diff/170001/net/http/url_security_...
File net/http/url_security_manager.h (right):

https://codereview.chromium.org/1414313002/diff/170001/net/http/url_security_...
net/http/url_security_manager.h:41: // TODO(cbentzel): Perhaps it's better to
make a non-abstract HttpAuthFilter
On 2015/11/04 16:05:44, asanka wrote:
> Might as well remove this comment since it no longer makes sense.

Done. Actually rewrote the whole comment.

https://codereview.chromium.org/1414313002/diff/170001/net/http/url_security_...
File net/http/url_security_manager_win.cc (right):

https://codereview.chromium.org/1414313002/diff/170001/net/http/url_security_...
net/http/url_security_manager_win.cc:121: // If we have a whitelist, just use
that.
On 2015/11/04 16:05:44, asanka wrote:
> Comment no longer accurate.

Done.

[aberent, 2015-11-06 13:57:59]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-06 13:58:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414313002/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414313002/190001

[commit-bot: I haz the power, 2015-11-06 14:02:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-06 14:02:42]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[aberent, 2015-11-06 16:05:50]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-06 16:06:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414313002/210001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414313002/210001

[commit-bot: I haz the power, 2015-11-06 16:21:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-06 16:22:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aberent, 2015-11-06 18:22:07]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-06 18:22:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414313002/90002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414313002/90002

[commit-bot: I haz the power, 2015-11-06 18:58:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-06 18:58:04]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[aberent, 2015-11-09 11:48:22]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-09 11:48:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414313002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414313002/240001

[commit-bot: I haz the power, 2015-11-09 12:56:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-09 12:56:37]

Dry run: This issue passed the CQ dry run.

[asanka, 2015-11-10 15:48:06]

The architectural problems I have with this are:

* Android code is building on all platforms.

I'd push for separation of cross-platform vs. per-platform configuration into
separate files. If that'd derail this CL, perhaps we can condition them out.

* //chrome depends on implementation details in //net.

Sure it's test code, but someone sometime is going to run into this and will
need to fix it later. //net unfortunately doesn't have a clear cut
public/private interface boundary (yet). But assuming that the
HttpAuthHandlerRegistryFactory returns a HttpAuthHandlerNegotiate for the
"negotiate" scheme is an internal implementation detail that can change.

The interface between //chrome and //net ends when the preference is updated in
HttpAuthHandlerRegistryFactory. Let's limit tests in //chrome to examining that
the HttpAuthHandlerRegistryFactory is updated when the pref changes. How and
whether the value propagates to specific handler factories is something that
should be tested in //net, not //chrome.

* [Somewhat pre-existing] Keeping copies of a policy doesn't work when the
policy starts changing dynamically.

Policies like 'disable CNAME lookup' and 'use port in SPN' were meant to be
common across a request context. I.e. individual handlers aren't supposed to
carry different values. This wasn't a big deal when the policies were
essentially constants. But now that these policies can change dynamically, we
should consider changing how these settings are stored and accessed. Instead of
trying to manually propagate the value down the hierarchy of auth handler
factories, we should consider storing them centrally since only one value is
supposed to exist.

https://codereview.chromium.org/1414313002/diff/90002/net/http/http_auth_hand...
File net/http/http_auth_handler_negotiate.h (right):

https://codereview.chromium.org/1414313002/diff/90002/net/http/http_auth_hand...
net/http/http_auth_handler_negotiate.h:86: 
Nit: extra space

https://codereview.chromium.org/1414313002/diff/240001/chrome/browser/io_thre...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/240001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:654:
net::HttpAuthHandlerNegotiate::Factory* getNegotiateFactory() {
GetNegotiateFactory

https://codereview.chromium.org/1414313002/diff/240001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:657: auto negotiate_factory =
This test relies on implementation details in //net. :-(

https://codereview.chromium.org/1414313002/diff/240001/net/http/http_auth_han...
File net/http/http_auth_handler_factory.cc (right):

https://codereview.chromium.org/1414313002/diff/240001/net/http/http_auth_han...
net/http/http_auth_handler_factory.cc:109: void
HttpAuthHandlerRegistryFactory::RegisterSchemeFactory(
This method should propagate dynamic properties when registering a new handler.

https://codereview.chromium.org/1414313002/diff/240001/net/http/http_auth_han...
net/http/http_auth_handler_factory.cc:202: #if defined(OS_ANDROID)
Can we comment out the entire method for !OS_ANDROID?

https://codereview.chromium.org/1414313002/diff/240001/net/http/url_security_...
File net/http/url_security_manager.h (right):

https://codereview.chromium.org/1414313002/diff/240001/net/http/url_security_...
net/http/url_security_manager.h:35: // credentials sxhould be used. If the
default whitelist is NULL and the
*should

https://codereview.chromium.org/1414313002/diff/240001/net/http/url_security_...
File net/http/url_security_manager_win.cc (right):

https://codereview.chromium.org/1414313002/diff/240001/net/http/url_security_...
net/http/url_security_manager_win.cc:50: if (HasDefaultWhitelist()) {
Nit: no braces.

[aberent, 2015-11-10 18:46:19]

n 2015/11/10 15:48:06, asanka wrote:
> The architectural problems I have with this are:
> 
> * Android code is building on all platforms.
> 
> I'd push for separation of cross-platform vs. per-platform configuration into
> separate files. If that'd derail this CL, perhaps we can condition them out.

Are you referring to http_auth_handler_factory and http_auth_handler_negotiate,
or to io_thread (or both)? Either way I think it would be quite complex to make
this split; since we would need to split each class into portable and non
portable parts, and then create two to four versions of the non-portable parts
of each (each with its own tests, presumably). If we do this then we should
presumably move the Posix, Windows, and Mac specific code into the the
non-portable parts. For now, if you are happy with this, I will simply add some
more conditional compilation.

> 
> * //chrome depends on implementation details in //net.
> 
> Sure it's test code, but someone sometime is going to run into this and will
> need to fix it later. //net unfortunately doesn't have a clear cut
> public/private interface boundary (yet). But assuming that the
> HttpAuthHandlerRegistryFactory returns a HttpAuthHandlerNegotiate for the
> "negotiate" scheme is an internal implementation detail that can change.
> 
> The interface between //chrome and //net ends when the preference is updated
in
> HttpAuthHandlerRegistryFactory. Let's limit tests in //chrome to examining
that
> the HttpAuthHandlerRegistryFactory is updated when the pref changes. How and
> whether the value propagates to specific handler factories is something that
> should be tested in //net, not //chrome.

Agreed, I will take another look at this. Probably involves adding test only
functions to HttpAuthHandlerRegistryFactory, but that could be the right thing
to do.
> 
> * [Somewhat pre-existing] Keeping copies of a policy doesn't work when the
> policy starts changing dynamically.
> 
> Policies like 'disable CNAME lookup' and 'use port in SPN' were meant to be
> common across a request context. I.e. individual handlers aren't supposed to
> carry different values. This wasn't a big deal when the policies were
> essentially constants. But now that these policies can change dynamically, we
> should consider changing how these settings are stored and accessed. Instead
of
> trying to manually propagate the value down the hierarchy of auth handler
> factories, we should consider storing them centrally since only one value is
> supposed to exist.

I am slightly confused by this, partly because I don't fully understand how
request contexts work. When are they created? What is their lifetime?

Would you see each request context reading the policies each time a request
created a handler (which would have the same effect as my current code), or
would they be only read when the request is created (or at some other time)?

Is there a good way of communicating between the IOThread object and the request
contexts, so that the request contexts see changes to the policies (I think the
policy PrefMembers have to be owned by the IOThread object, in order to
correctly move them from the UI thread to the IO thread and to handle their
lifetimes correctly)?

[asanka, 2015-11-11 16:48:48]

On 2015/11/10 at 18:46:19, aberent wrote:
> n 2015/11/10 15:48:06, asanka wrote:
> > The architectural problems I have with this are:
> > 
> > * Android code is building on all platforms.
> > 
> > I'd push for separation of cross-platform vs. per-platform configuration
into
> > separate files. If that'd derail this CL, perhaps we can condition them out.
> 
> Are you referring to http_auth_handler_factory and
http_auth_handler_negotiate, or to io_thread (or both)? Either way I think it
would be quite complex to make this split; since we would need to split each
class into portable and non portable parts, and then create two to four versions
of the non-portable parts of each (each with its own tests, presumably). If we
do this then we should presumably move the Posix, Windows, and Mac specific code
into the the non-portable parts. For now, if you are happy with this, I will
simply add some more conditional compilation.


Both. I'd be okay with conditioning out Android code for this CL.

> > * //chrome depends on implementation details in //net.
> > 
> > Sure it's test code, but someone sometime is going to run into this and will
> > need to fix it later. //net unfortunately doesn't have a clear cut
> > public/private interface boundary (yet). But assuming that the
> > HttpAuthHandlerRegistryFactory returns a HttpAuthHandlerNegotiate for the
> > "negotiate" scheme is an internal implementation detail that can change.
> > 
> > The interface between //chrome and //net ends when the preference is updated
in
> > HttpAuthHandlerRegistryFactory. Let's limit tests in //chrome to examining
that
> > the HttpAuthHandlerRegistryFactory is updated when the pref changes. How and
> > whether the value propagates to specific handler factories is something that
> > should be tested in //net, not //chrome.
> 
> Agreed, I will take another look at this. Probably involves adding test only
functions to HttpAuthHandlerRegistryFactory, but that could be the right thing
to do.

> > * [Somewhat pre-existing] Keeping copies of a policy doesn't work when the
> > policy starts changing dynamically.
> > 
> > Policies like 'disable CNAME lookup' and 'use port in SPN' were meant to be
> > common across a request context. I.e. individual handlers aren't supposed to
> > carry different values. This wasn't a big deal when the policies were
> > essentially constants. But now that these policies can change dynamically,
we
> > should consider changing how these settings are stored and accessed. Instead
of
> > trying to manually propagate the value down the hierarchy of auth handler
> > factories, we should consider storing them centrally since only one value is
> > supposed to exist.
> 
> I am slightly confused by this, partly because I don't fully understand how
request contexts work. When are they created? What is their lifetime?

This document has a good overview:
https://chromium.googlesource.com/chromium/src/+/master/net/docs/life-of-a-ur...

> Would you see each request context reading the policies each time a request
created a handler (which would have the same effect as my current code), or
would they be only read when the request is created (or at some other time)?
> 
> Is there a good way of communicating between the IOThread object and the
request contexts, so that the request contexts see changes to the policies (I
think the policy PrefMembers have to be owned by the IOThread object, in order
to correctly move them from the UI thread to the IO thread and to handle their
lifetimes correctly)?

We can follow the model for URLSecurityManager. It's created in IOThread, stored
in IOThread::Globals, used to created an HttpAuthHandlerFactory, which is itself
stored in IOThread::Global. The HttpAuthHandlerFactory in IOThread::Globals is
then referenced by all per-profile URLRequestContexts created in
ProfileImplIOData.

[aberent, 2015-11-11 18:32:28]

On 2015/11/11 16:48:48, asanka wrote:
> On 2015/11/10 at 18:46:19, aberent wrote:
> > n 2015/11/10 15:48:06, asanka wrote:
> > > The architectural problems I have with this are:
> > > 
> > > * Android code is building on all platforms.
> > > 
> > > I'd push for separation of cross-platform vs. per-platform configuration
> into
> > > separate files. If that'd derail this CL, perhaps we can condition them
out.
> > 
> > Are you referring to http_auth_handler_factory and
> http_auth_handler_negotiate, or to io_thread (or both)? Either way I think it
> would be quite complex to make this split; since we would need to split each
> class into portable and non portable parts, and then create two to four
versions
> of the non-portable parts of each (each with its own tests, presumably). If we
> do this then we should presumably move the Posix, Windows, and Mac specific
code
> into the the non-portable parts. For now, if you are happy with this, I will
> simply add some more conditional compilation.
> 
> 
> Both. I'd be okay with conditioning out Android code for this CL.
> 
> > > * //chrome depends on implementation details in //net.
> > > 
> > > Sure it's test code, but someone sometime is going to run into this and
will
> > > need to fix it later. //net unfortunately doesn't have a clear cut
> > > public/private interface boundary (yet). But assuming that the
> > > HttpAuthHandlerRegistryFactory returns a HttpAuthHandlerNegotiate for the
> > > "negotiate" scheme is an internal implementation detail that can change.
> > > 
> > > The interface between //chrome and //net ends when the preference is
updated
> in
> > > HttpAuthHandlerRegistryFactory. Let's limit tests in //chrome to examining
> that
> > > the HttpAuthHandlerRegistryFactory is updated when the pref changes. How
and
> > > whether the value propagates to specific handler factories is something
that
> > > should be tested in //net, not //chrome.
> > 
> > Agreed, I will take another look at this. Probably involves adding test only
> functions to HttpAuthHandlerRegistryFactory, but that could be the right thing
> to do.
> 
> > > * [Somewhat pre-existing] Keeping copies of a policy doesn't work when the
> > > policy starts changing dynamically.
> > > 
> > > Policies like 'disable CNAME lookup' and 'use port in SPN' were meant to
be
> > > common across a request context. I.e. individual handlers aren't supposed
to
> > > carry different values. This wasn't a big deal when the policies were
> > > essentially constants. But now that these policies can change dynamically,
> we
> > > should consider changing how these settings are stored and accessed.
Instead
> of
> > > trying to manually propagate the value down the hierarchy of auth handler
> > > factories, we should consider storing them centrally since only one value
is
> > > supposed to exist.
> > 
> > I am slightly confused by this, partly because I don't fully understand how
> request contexts work. When are they created? What is their lifetime?
> 
> This document has a good overview:
>
https://chromium.googlesource.com/chromium/src/+/master/net/docs/life-of-a-ur...
> 
> > Would you see each request context reading the policies each time a request
> created a handler (which would have the same effect as my current code), or
> would they be only read when the request is created (or at some other time)?
> > 
> > Is there a good way of communicating between the IOThread object and the
> request contexts, so that the request contexts see changes to the policies (I
> think the policy PrefMembers have to be owned by the IOThread object, in order
> to correctly move them from the UI thread to the IO thread and to handle their
> lifetimes correctly)?
> 
> We can follow the model for URLSecurityManager. It's created in IOThread,
stored
> in IOThread::Globals, used to created an HttpAuthHandlerFactory, which is
itself
> stored in IOThread::Global. The HttpAuthHandlerFactory in IOThread::Globals is
> then referenced by all per-profile URLRequestContexts created in
> ProfileImplIOData.

Good. That's quite close to what I have been working on this afternoon (in
parallel with Android sheriffing). My intention is to create a single class
(HttpAuthPreferences) that contains all the authentication preferences, with
their PrefMembers, with its unique instance owned by IOThread, and then pass
this down to all the handlers. I think, however, this has to be owned directly
by IOThread (either as a direct member or as a scoped_ptr), not by 
IOThread::Global, since, for the pref members to work it has to be created on
the browser thread, and the IOThread::Global object only exists once Init is
called on the IO thread.

Does this make sense to you?

I should have something for you to look at later this evening (my time) or
tomorrow.

[asanka, 2015-11-11 20:02:12]

On 2015/11/11 at 18:32:28, aberent wrote:
> On 2015/11/11 16:48:48, asanka wrote:
> > On 2015/11/10 at 18:46:19, aberent wrote:
> > > n 2015/11/10 15:48:06, asanka wrote:
> > > > The architectural problems I have with this are:
> > > > 
> > > > * Android code is building on all platforms.
> > > > 
> > > > I'd push for separation of cross-platform vs. per-platform configuration
> > into
> > > > separate files. If that'd derail this CL, perhaps we can condition them
out.
> > > 
> > > Are you referring to http_auth_handler_factory and
> > http_auth_handler_negotiate, or to io_thread (or both)? Either way I think
it
> > would be quite complex to make this split; since we would need to split each
> > class into portable and non portable parts, and then create two to four
versions
> > of the non-portable parts of each (each with its own tests, presumably). If
we
> > do this then we should presumably move the Posix, Windows, and Mac specific
code
> > into the the non-portable parts. For now, if you are happy with this, I will
> > simply add some more conditional compilation.
> > 
> > 
> > Both. I'd be okay with conditioning out Android code for this CL.
> > 
> > > > * //chrome depends on implementation details in //net.
> > > > 
> > > > Sure it's test code, but someone sometime is going to run into this and
will
> > > > need to fix it later. //net unfortunately doesn't have a clear cut
> > > > public/private interface boundary (yet). But assuming that the
> > > > HttpAuthHandlerRegistryFactory returns a HttpAuthHandlerNegotiate for
the
> > > > "negotiate" scheme is an internal implementation detail that can change.
> > > > 
> > > > The interface between //chrome and //net ends when the preference is
updated
> > in
> > > > HttpAuthHandlerRegistryFactory. Let's limit tests in //chrome to
examining
> > that
> > > > the HttpAuthHandlerRegistryFactory is updated when the pref changes. How
and
> > > > whether the value propagates to specific handler factories is something
that
> > > > should be tested in //net, not //chrome.
> > > 
> > > Agreed, I will take another look at this. Probably involves adding test
only
> > functions to HttpAuthHandlerRegistryFactory, but that could be the right
thing
> > to do.
> > 
> > > > * [Somewhat pre-existing] Keeping copies of a policy doesn't work when
the
> > > > policy starts changing dynamically.
> > > > 
> > > > Policies like 'disable CNAME lookup' and 'use port in SPN' were meant to
be
> > > > common across a request context. I.e. individual handlers aren't
supposed to
> > > > carry different values. This wasn't a big deal when the policies were
> > > > essentially constants. But now that these policies can change
dynamically,
> > we
> > > > should consider changing how these settings are stored and accessed.
Instead
> > of
> > > > trying to manually propagate the value down the hierarchy of auth
handler
> > > > factories, we should consider storing them centrally since only one
value is
> > > > supposed to exist.
> > > 
> > > I am slightly confused by this, partly because I don't fully understand
how
> > request contexts work. When are they created? What is their lifetime?
> > 
> > This document has a good overview:
> >
https://chromium.googlesource.com/chromium/src/+/master/net/docs/life-of-a-ur...
> > 
> > > Would you see each request context reading the policies each time a
request
> > created a handler (which would have the same effect as my current code), or
> > would they be only read when the request is created (or at some other time)?
> > > 
> > > Is there a good way of communicating between the IOThread object and the
> > request contexts, so that the request contexts see changes to the policies
(I
> > think the policy PrefMembers have to be owned by the IOThread object, in
order
> > to correctly move them from the UI thread to the IO thread and to handle
their
> > lifetimes correctly)?
> > 
> > We can follow the model for URLSecurityManager. It's created in IOThread,
stored
> > in IOThread::Globals, used to created an HttpAuthHandlerFactory, which is
itself
> > stored in IOThread::Global. The HttpAuthHandlerFactory in IOThread::Globals
is
> > then referenced by all per-profile URLRequestContexts created in
> > ProfileImplIOData.
> 
> Good. That's quite close to what I have been working on this afternoon (in
parallel with Android sheriffing). My intention is to create a single class
(HttpAuthPreferences) that contains all the authentication preferences, with
their PrefMembers, with its unique instance owned by IOThread, and then pass
this down to all the handlers. I think, however, this has to be owned directly
by IOThread (either as a direct member or as a scoped_ptr), not by 
IOThread::Global, since, for the pref members to work it has to be created on
the browser thread, and the IOThread::Global object only exists once Init is
called on the IO thread.
> 
> Does this make sense to you?
> 
> I should have something for you to look at later this evening (my time) or
tomorrow.

SGTM. If it helps, you can generalize the responsibilities of URLSecurityManager
to include carrying the SPN generation policies as well. You'll need to rename
the class to match.

[aberent, 2015-11-13 17:32:43]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-13 17:33:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414313002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414313002/260001

[aberent, 2015-11-13 17:46:32]

PTAL - I have now moved all the updating and retrieval of the authentication
preferences to a new class (HttpAuthPreferences), and am using this throughout
the authentication handlers.

https://codereview.chromium.org/1414313002/diff/240001/chrome/browser/io_thre...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/240001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:657: auto negotiate_factory =
On 2015/11/10 15:48:06, asanka wrote:
> This test relies on implementation details in //net. :-(

Done.

https://codereview.chromium.org/1414313002/diff/240001/net/http/http_auth_han...
File net/http/http_auth_handler_factory.cc (right):

https://codereview.chromium.org/1414313002/diff/240001/net/http/http_auth_han...
net/http/http_auth_handler_factory.cc:109: void
HttpAuthHandlerRegistryFactory::RegisterSchemeFactory(
On 2015/11/10 15:48:06, asanka wrote:
> This method should propagate dynamic properties when registering a new
handler.

Done.

https://codereview.chromium.org/1414313002/diff/240001/net/http/http_auth_han...
net/http/http_auth_handler_factory.cc:202: #if defined(OS_ANDROID)
On 2015/11/10 15:48:06, asanka wrote:
> Can we comment out the entire method for !OS_ANDROID? 

Method no longer exists. Methods for account type in HttpAuthPreferences are
Android only.

https://codereview.chromium.org/1414313002/diff/240001/net/http/url_security_...
File net/http/url_security_manager.h (right):

https://codereview.chromium.org/1414313002/diff/240001/net/http/url_security_...
net/http/url_security_manager.h:35: // credentials sxhould be used. If the
default whitelist is NULL and the
On 2015/11/10 15:48:06, asanka wrote:
> *should

Done.

[aberent, 2015-11-13 18:06:58]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-13 18:07:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414313002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414313002/300001

[dgn, 2015-11-13 18:24:16]

dgn@chromium.org changed reviewers:
+ dgn@chromium.org

[dgn, 2015-11-13 18:24:17]

https://codereview.chromium.org/1414313002/diff/300001/net/http/http_auth_pre...
File net/http/http_auth_preferences.cc (right):

https://codereview.chromium.org/1414313002/diff/300001/net/http/http_auth_pre...
net/http/http_auth_preferences.cc:21: "basic,digest,ntlm,negotiate");
for webview, we would need to have "basic,digest,negotiate" as defaults.

[aberent, 2015-11-13 18:37:19]

aberent@chromium.org changed reviewers:
+ atwilson@chromium.org

[aberent, 2015-11-13 18:37:20]

atwilson@chromium.org: Please review changes in
configuration_policy_handler_list_factory.cc

The only change is that some of the preferences referenced here have moved into
net.

[commit-bot: I haz the power, 2015-11-13 20:11:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-13 20:11:25]

Dry run: This issue passed the CQ dry run.

[asanka, 2015-11-19 02:58:21]

Wow. This is a lot of work. Let's have a quick chat tomorrow before the next
round.

The high-level concern is that we don't want //base/prefs to be the mechanism
for setting parameters for //net. It's fine for other higher level components to
use prefs for storing preferences, and an embedder of //net may use whichever
mechanism they choose to determine parmeters for settings up //net. However, the
API that //net exposes shouldn't tie itself to //base/prefs.

[aberent, 2015-11-19 12:36:37]

On 2015/11/19 02:58:21, asanka wrote:
> Wow. This is a lot of work. Let's have a quick chat tomorrow before the next
> round.
> 
> The high-level concern is that we don't want //base/prefs to be the mechanism
> for setting parameters for //net. It's fine for other higher level components
to
> use prefs for storing preferences, and an embedder of //net may use whichever
> mechanism they choose to determine parmeters for settings up //net. However,
the
> API that //net exposes shouldn't tie itself to //base/prefs.

I have set up a VC for this afternoon (this morning your time). I clearly don't
understand the requirements for the net API.

[aberent, 2015-11-19 18:40:49]

On 2015/11/19 12:36:37, aberent wrote:
> On 2015/11/19 02:58:21, asanka wrote:
> > Wow. This is a lot of work. Let's have a quick chat tomorrow before the next
> > round.
> > 
> > The high-level concern is that we don't want //base/prefs to be the
mechanism
> > for setting parameters for //net. It's fine for other higher level
components
> to
> > use prefs for storing preferences, and an embedder of //net may use
whichever
> > mechanism they choose to determine parmeters for settings up //net. However,
> the
> > API that //net exposes shouldn't tie itself to //base/prefs.
> 
> I have set up a VC for this afternoon (this morning your time). I clearly
don't
> understand the requirements for the net API.

Asanka - I have now done the core updates to fix this and got Chrome building,
although I haven't yet done anything to the tests or other uses of net. Please
take a look ASAP, to check that we are now really in agreement about how this
should work. The main changes from the last patch are in IOThread and
HttpAuthPreferences.

[asanka, 2015-11-20 15:32:10]

Thanks. Converging!

https://codereview.chromium.org/1414313002/diff/320001/net/base/net_pref_names.h
File net/base/net_pref_names.h (right):

https://codereview.chromium.org/1414313002/diff/320001/net/base/net_pref_name...
net/base/net_pref_names.h:13: namespace prefs {
Pref names shouldn't be in //net. I understand that multiple components may need
the pref names, and that the pref names correspond to specific //net settings.
But the names themselves are up to the respective embedders.

https://codereview.chromium.org/1414313002/diff/320001/net/http/http_auth_han...
File net/http/http_auth_handler_factory.cc (right):

https://codereview.chromium.org/1414313002/diff/320001/net/http/http_auth_han...
net/http/http_auth_handler_factory.cc:50: const char* default_auth_types[] =
{kBasicAuthScheme, kDigestAuthScheme,
const char* const kDefaultAuthTypes[] = {...};

Without the extra const, kDefaultAuthTypes would be a non-const array of const
pointers. I.e. it would be legal to do:
  kDefaultAuthTypes[0] = "asdf";

https://codereview.chromium.org/1414313002/diff/320001/net/http/http_auth_han...
File net/http/http_auth_handler_factory.h (right):

https://codereview.chromium.org/1414313002/diff/320001/net/http/http_auth_han...
net/http/http_auth_handler_factory.h:116: virtual void
SetNegotiateDisableCnameLookup(bool disable_cname_lookup) {}
Why have individual setters? Wouldn't the callers update the HttpAuthPreferences
object instead?

[aberent, 2015-11-23 16:34:00]

aberent@chromium.org changed reviewers:
- atwilson@chromium.org

[aberent, 2015-11-23 16:34:01]

atwilson@ - I have removed the changes I asked you to review, so please feel
free to ignore this CL.

https://codereview.chromium.org/1414313002/diff/320001/net/base/net_pref_names.h
File net/base/net_pref_names.h (right):

https://codereview.chromium.org/1414313002/diff/320001/net/base/net_pref_name...
net/base/net_pref_names.h:13: namespace prefs {
On 2015/11/20 15:32:09, asanka wrote:
> Pref names shouldn't be in //net. I understand that multiple components may
need
> the pref names, and that the pref names correspond to specific //net settings.
> But the names themselves are up to the respective embedders.

Done. These aren't actually used any more. I just forgot to delete the file (and
a few references to it).

https://codereview.chromium.org/1414313002/diff/320001/net/http/http_auth_han...
File net/http/http_auth_handler_factory.cc (right):

https://codereview.chromium.org/1414313002/diff/320001/net/http/http_auth_han...
net/http/http_auth_handler_factory.cc:50: const char* default_auth_types[] =
{kBasicAuthScheme, kDigestAuthScheme,
On 2015/11/20 15:32:09, asanka wrote:
> const char* const kDefaultAuthTypes[] = {...};
> 
> Without the extra const, kDefaultAuthTypes would be a non-const array of const
> pointers. I.e. it would be legal to do:
>   kDefaultAuthTypes[0] = "asdf";
> 

Done.

https://codereview.chromium.org/1414313002/diff/320001/net/http/http_auth_han...
File net/http/http_auth_handler_factory.h (right):

https://codereview.chromium.org/1414313002/diff/320001/net/http/http_auth_han...
net/http/http_auth_handler_factory.h:116: virtual void
SetNegotiateDisableCnameLookup(bool disable_cname_lookup) {}
On 2015/11/20 15:32:09, asanka wrote:
> Why have individual setters? Wouldn't the callers update the
HttpAuthPreferences
> object instead?

Done.

[aberent, 2015-11-23 16:35:57]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-23 16:37:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414313002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414313002/340001

[commit-bot: I haz the power, 2015-11-23 16:56:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-23 16:56:58]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)

[aberent, 2015-11-23 19:25:49]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-23 19:26:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414313002/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414313002/380001

[commit-bot: I haz the power, 2015-11-23 20:39:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-23 20:39:06]

Dry run: Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[aberent, 2015-11-24 10:50:45]

aberent@chromium.org changed reviewers:
+ sdefresne@chromium.org

[aberent, 2015-11-24 10:50:46]

sdefresne@chromium.org: Please review changes in ios_chrome_io_thread.*. Changes
match changes in io_thread.*.

[aberent, 2015-11-24 15:53:43]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-24 15:54:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414313002/440001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414313002/440001

[sdefresne, 2015-11-24 16:51:24]

ios/chrome/browser/ios_chrome_io_thread.* lgtm

[commit-bot: I haz the power, 2015-11-24 18:14:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-24 18:14:40]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[asanka, 2015-11-25 19:00:10]

Very close. And thanks for working on this.

https://codereview.chromium.org/1414313002/diff/440001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1414313002/diff/440001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:1064: void
IOThread::CreateDefaultAuthHandlerFactory() {
Nit: add new line.

https://codereview.chromium.org/1414313002/diff/440001/chrome/browser/io_thre...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/440001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:12: #include
"chrome/browser/extensions/event_router_forwarder.h"
Extensions code including dependencies should be conditioned out based on
ENABLE_EXTENSIONS.

https://codereview.chromium.org/1414313002/diff/440001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:599: #if defined(ENABLE_EXTENSIONS)
Let's move initialization / teardown into SetUp()/TearDown().

https://codereview.chromium.org/1414313002/diff/440001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:635: IOThread* io_thread() { return
io_thread_.get(); }
Remove this accessor since it can't be safely used from a test. By the time the
TestBody() runs, IOThread should only be accessed on the IO thread.

https://codereview.chromium.org/1414313002/diff/440001/ios/chrome/browser/ios...
File ios/chrome/browser/ios_chrome_io_thread.cc (right):

https://codereview.chromium.org/1414313002/diff/440001/ios/chrome/browser/ios...
ios/chrome/browser/ios_chrome_io_thread.cc:91: const char
kSupportedAuthSchemes[] = "basic,digest,ntlm,negotiate";
Huh. This list specifies 'negotiate' although //net/net.gyp explicitly excludes
Negotiate support on iOS.

https://codereview.chromium.org/1414313002/diff/440001/net/android/http_auth_...
File net/android/http_auth_negotiate_android.cc (right):

https://codereview.chromium.org/1414313002/diff/440001/net/android/http_auth_...
net/android/http_auth_negotiate_android.cc:108: return ERR_METHOD_NOT_SUPPORTED;
ERR_METHOD_NOT_SUPPORTED is used to indicate that an HTTP method (a.k.a. verb)
is not supported. Use ERR_UNSUPPORTED_AUTH_SCHEME here instead.

https://codereview.chromium.org/1414313002/diff/440001/net/android/http_auth_...
File net/android/http_auth_negotiate_android.h (right):

https://codereview.chromium.org/1414313002/diff/440001/net/android/http_auth_...
net/android/http_auth_negotiate_android.h:62: explicit
HttpAuthNegotiateAndroid(HttpAuthPreferences* prefs);
Let's pass in const references for the auth handlers. There's nothing in
HttpAuthPreferences that they should be able to modify.

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_han...
File net/http/http_auth_handler_factory.cc (right):

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_han...
net/http/http_auth_handler_factory.cc:50: const char* const default_auth_types[]
= {kBasicAuthScheme, kDigestAuthScheme,
kDefaultAuthSchemes

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_han...
net/http/http_auth_handler_factory.cc:63: void InitAuthHandlerRegistryFactory(
The existence of this function is unfortunate. Is this because of
HttpAuthHandlerFactory::CreateDefault() ?

An alternative is to have InitAuthHandlerRegistryFactory() take a
HttpAuthPreferences directly. Then construct a temporary HttpAuthPreferences in
HAHF::CreateDefault() and use that to call InitAuthHandlerRegistryFactory().

HttpAuthHandlerRegistryFactory::Create() can call SetHttpAuthPreferences() in
addition to calling Init...

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_han...
net/http/http_auth_handler_factory.cc:178: for (std::pair<std::string,
HttpAuthHandlerFactory*> factory_entry :
You can use auto here for iterators.

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_han...
File net/http/http_auth_handler_factory.h (right):

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_han...
net/http/http_auth_handler_factory.h:38: HttpAuthHandlerFactory() :
http_auth_preferences_(NULL) {}
nullptr

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_han...
File net/http/http_auth_handler_factory_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_han...
net/http/http_auth_handler_factory_unittest.cc:5: #include
<net/http/mock_allow_http_auth_preferences.h>
#include "..."

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_han...
File net/http/http_auth_handler_negotiate_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_han...
net/http/http_auth_handler_negotiate_unittest.cc:5: #include
<net/http/mock_allow_http_auth_preferences.h>
#include "..."

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_pre...
File net/http/http_auth_preferences.h (right):

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_pre...
net/http/http_auth_preferences.h:31: virtual std::vector<std::string>
auth_schemes() const;
Virtual getters aren't usually considered trivial. I suggest using CamelCase for
these.

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_pre...
File net/http/http_auth_preferences_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_pre...
net/http/http_auth_preferences_unittest.cc:9: #include "base/callback.h"
Insert new line above.

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_uni...
File net/http/http_auth_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_uni...
net/http/http_auth_unittest.cc:5: #include
<net/http/mock_allow_http_auth_preferences.h>
include "..." here and elsewhere when including files from the source tree.

https://codereview.chromium.org/1414313002/diff/440001/net/http/url_security_...
File net/http/url_security_manager.h (right):

https://codereview.chromium.org/1414313002/diff/440001/net/http/url_security_...
net/http/url_security_manager.h:11: #include "net/http/http_auth_filter.h"
scoped_ptr<> can use an incomplete class.

[aberent, 2015-11-26 15:58:00]

All comments answered.

At least one more patch set will be needed to take into account
https://codereview.chromium.org/1431473004/, which landed today.

https://codereview.chromium.org/1414313002/diff/440001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1414313002/diff/440001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:1064: void
IOThread::CreateDefaultAuthHandlerFactory() {
On 2015/11/25 19:00:09, asanka wrote:
> Nit: add new line.

Done.

https://codereview.chromium.org/1414313002/diff/440001/chrome/browser/io_thre...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/440001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:12: #include
"chrome/browser/extensions/event_router_forwarder.h"
On 2015/11/25 19:00:09, asanka wrote:
> Extensions code including dependencies should be conditioned out based on
> ENABLE_EXTENSIONS.

Done.

https://codereview.chromium.org/1414313002/diff/440001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:599: #if defined(ENABLE_EXTENSIONS)
On 2015/11/25 19:00:09, asanka wrote:
> Let's move initialization / teardown into SetUp()/TearDown().

Why? The googletest documentation definitely recommends using the constructor in
preference to SetUp. See
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul....

"When you need to write per-test set-up and tear-down logic, you have the choice
between using the test fixture constructor/destructor or SetUp()/TearDown(). The
former is usually preferred, as it has the following benefits:..."

https://codereview.chromium.org/1414313002/diff/440001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:635: IOThread* io_thread() { return
io_thread_.get(); }
On 2015/11/25 19:00:09, asanka wrote:
> Remove this accessor since it can't be safely used from a test. By the time
the
> TestBody() runs, IOThread should only be accessed on the IO thread.

Done.

https://codereview.chromium.org/1414313002/diff/440001/ios/chrome/browser/ios...
File ios/chrome/browser/ios_chrome_io_thread.cc (right):

https://codereview.chromium.org/1414313002/diff/440001/ios/chrome/browser/ios...
ios/chrome/browser/ios_chrome_io_thread.cc:91: const char
kSupportedAuthSchemes[] = "basic,digest,ntlm,negotiate";
On 2015/11/25 19:00:09, asanka wrote:
> Huh. This list specifies 'negotiate' although //net/net.gyp explicitly
excludes
> Negotiate support on iOS.

I will point this out the iOS team, however I think fixing this should be a
different CL (this isn't changed by my CL).

https://codereview.chromium.org/1414313002/diff/440001/net/android/http_auth_...
File net/android/http_auth_negotiate_android.cc (right):

https://codereview.chromium.org/1414313002/diff/440001/net/android/http_auth_...
net/android/http_auth_negotiate_android.cc:108: return ERR_METHOD_NOT_SUPPORTED;
On 2015/11/25 19:00:09, asanka wrote:
> ERR_METHOD_NOT_SUPPORTED is used to indicate that an HTTP method (a.k.a. verb)
> is not supported. Use ERR_UNSUPPORTED_AUTH_SCHEME here instead.

Done.

https://codereview.chromium.org/1414313002/diff/440001/net/android/http_auth_...
File net/android/http_auth_negotiate_android.h (right):

https://codereview.chromium.org/1414313002/diff/440001/net/android/http_auth_...
net/android/http_auth_negotiate_android.h:62: explicit
HttpAuthNegotiateAndroid(HttpAuthPreferences* prefs);
On 2015/11/25 19:00:09, asanka wrote:
> Let's pass in const references for the auth handlers. There's nothing in
> HttpAuthPreferences that they should be able to modify.

Sadly we can't, because most of the auth handlers have to check for a null prefs
pointer before using it. This one doesn't because we only enable negotiate on
Android if the necessary preferences are set. I have changed these pointers
(and, if fact, every other raw HttpAuthPreferences pointer) to pointers to
const.

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_han...
File net/http/http_auth_handler_factory.cc (right):

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_han...
net/http/http_auth_handler_factory.cc:50: const char* const default_auth_types[]
= {kBasicAuthScheme, kDigestAuthScheme,
On 2015/11/25 19:00:09, asanka wrote:
> kDefaultAuthSchemes

Done.

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_han...
net/http/http_auth_handler_factory.cc:63: void InitAuthHandlerRegistryFactory(
On 2015/11/25 19:00:09, asanka wrote:
> The existence of this function is unfortunate. Is this because of
> HttpAuthHandlerFactory::CreateDefault() ?

Yes, precisely.
> 
> An alternative is to have InitAuthHandlerRegistryFactory() take a
> HttpAuthPreferences directly. Then construct a temporary HttpAuthPreferences
in
> HAHF::CreateDefault() and use that to call InitAuthHandlerRegistryFactory().
> 
> HttpAuthHandlerRegistryFactory::Create() can call SetHttpAuthPreferences() in
> addition to calling Init...

Done.

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_han...
net/http/http_auth_handler_factory.cc:178: for (std::pair<std::string,
HttpAuthHandlerFactory*> factory_entry :
On 2015/11/25 19:00:09, asanka wrote:
> You can use auto here for iterators.

Done.

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_han...
File net/http/http_auth_handler_factory.h (right):

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_han...
net/http/http_auth_handler_factory.h:38: HttpAuthHandlerFactory() :
http_auth_preferences_(NULL) {}
On 2015/11/25 19:00:09, asanka wrote:
> nullptr

Done.

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_han...
File net/http/http_auth_handler_factory_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_han...
net/http/http_auth_handler_factory_unittest.cc:5: #include
<net/http/mock_allow_http_auth_preferences.h>
On 2015/11/25 19:00:10, asanka wrote:
> #include "..."

Done. Missed that Eclipse had done this when I renamed the class :-(

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_han...
File net/http/http_auth_handler_negotiate_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_han...
net/http/http_auth_handler_negotiate_unittest.cc:5: #include
<net/http/mock_allow_http_auth_preferences.h>
On 2015/11/25 19:00:10, asanka wrote:
> #include "..."

Done.

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_pre...
File net/http/http_auth_preferences.h (right):

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_pre...
net/http/http_auth_preferences.h:31: virtual std::vector<std::string>
auth_schemes() const;
On 2015/11/25 19:00:10, asanka wrote:
> Virtual getters aren't usually considered trivial. I suggest using CamelCase
for
> these.

Done.

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_pre...
File net/http/http_auth_preferences_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_pre...
net/http/http_auth_preferences_unittest.cc:9: #include "base/callback.h"
On 2015/11/25 19:00:10, asanka wrote:
> Insert new line above.

Done.

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_uni...
File net/http/http_auth_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/440001/net/http/http_auth_uni...
net/http/http_auth_unittest.cc:5: #include
<net/http/mock_allow_http_auth_preferences.h>
On 2015/11/25 19:00:10, asanka wrote:
> include "..." here and elsewhere when including files from the source tree.

Done.

https://codereview.chromium.org/1414313002/diff/440001/net/http/url_security_...
File net/http/url_security_manager.h (right):

https://codereview.chromium.org/1414313002/diff/440001/net/http/url_security_...
net/http/url_security_manager.h:11: #include "net/http/http_auth_filter.h"
On 2015/11/25 19:00:10, asanka wrote:
> scoped_ptr<> can use an incomplete class.

Done.

[sdefresne, 2015-11-26 16:11:17]

https://codereview.chromium.org/1414313002/diff/440001/ios/chrome/browser/ios...
File ios/chrome/browser/ios_chrome_io_thread.cc (right):

https://codereview.chromium.org/1414313002/diff/440001/ios/chrome/browser/ios...
ios/chrome/browser/ios_chrome_io_thread.cc:91: const char
kSupportedAuthSchemes[] = "basic,digest,ntlm,negotiate";
On 2015/11/26 at 15:58:00, aberent wrote:
> On 2015/11/25 19:00:09, asanka wrote:
> > Huh. This list specifies 'negotiate' although //net/net.gyp explicitly
excludes
> > Negotiate support on iOS.
> 
> I will point this out the iOS team, however I think fixing this should be a
different CL (this isn't changed by my CL).

Thank you for pointing this out. I guess that if "negotiate" support is not
compiled in on iOS, then we can remove it from this list.

[aberent, 2015-11-26 17:09:43]

aberent@chromium.org changed reviewers:
+ torne@chromium.org

[aberent, 2015-11-26 17:09:44]

torne@chromium.org: Please review changes in android_webview as owner.

[aberent, 2015-11-26 17:10:27]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-26 17:10:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414313002/480001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414313002/480001

[Torne, 2015-11-26 17:12:43]

lgtm

[aberent, 2015-11-26 17:15:38]

On 2015/11/26 17:09:44, aberent wrote:
> mailto:torne@chromium.org: Please review changes in android_webview as owner.

Note that this doesn't allow dynamic updates to authentication policies in
Webview. If we do that, it will be a separate CL.

[commit-bot: I haz the power, 2015-11-26 21:10:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-26 21:10:38]

Dry run: This issue passed the CQ dry run.

[asanka, 2015-12-01 05:30:04]

Thanks for bearing with me so far. One more round to clean up some destruction
order dependencies and eliminate some unnecessary temporaries and we should be
done.

https://codereview.chromium.org/1414313002/diff/440001/chrome/browser/io_thre...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/440001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:599: #if defined(ENABLE_EXTENSIONS)
On 2015/11/26 at 15:58:00, aberent wrote:
> On 2015/11/25 19:00:09, asanka wrote:
> > Let's move initialization / teardown into SetUp()/TearDown().
> 
> Why? The googletest documentation definitely recommends using the constructor
in preference to SetUp. See
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul....
> 
> "When you need to write per-test set-up and tear-down logic, you have the
choice between using the test fixture constructor/destructor or
SetUp()/TearDown(). The former is usually preferred, as it has the following
benefits:..."

Suggestion retracted.

The suggestion was for consistency with other tests which need to do
initialization in SetUp() due to dependencies on base classes which also do
initialization in SetUp(), and other cases where test initialization may fail
(requires creating temporary directories, for example). However based on the
chromium-dev thread, there's no consensus on preferring SetUp() for consistency.

https://codereview.chromium.org/1414313002/diff/480001/android_webview/browse...
File android_webview/browser/net/aw_url_request_context_getter.h (right):

https://codereview.chromium.org/1414313002/diff/480001/android_webview/browse...
android_webview/browser/net/aw_url_request_context_getter.h:87:
scoped_ptr<net::HttpAuthPreferences> http_auth_preferences_;
The scoped_ptr<net::URLRequestContext> in line 83 actually is-a
ContainerURLRequestContext that bundles a net::URLRequestContextStorage which
owns a net::HttpAuthHandlerRegistryFactory which holds a reference to the
HttpAuthPreferences object owned by http_auth_preferences_.

However the destruction order for the AwURLRequestContextGetter results in
freeing the owned http_auth_preferences_ object prior to destroying the
url_request_context_ object. By the time the url_request_context_ is destroyed,
its contained auth handler factory references freed memory.

Could you move the http_auth_preferences_ definition above url_request_context_
so that the destruction order is safe?

https://codereview.chromium.org/1414313002/diff/480001/chrome/browser/io_thre...
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/1414313002/diff/480001/chrome/browser/io_thre...
chrome/browser/io_thread.h:82: class HttpAuthPreferences;
Sort.

https://codereview.chromium.org/1414313002/diff/480001/chrome/browser/io_thre...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/480001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:642: void
RunOnIOThreadBlocking(base::Closure task) {
Nit: const base::Closure&

https://codereview.chromium.org/1414313002/diff/480001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:658: IOThread::Globals globals_;
Not used?

https://codereview.chromium.org/1414313002/diff/480001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:692:
base::Bind(&IOThreadTestWithIOThreadObject::CheckCanUseDefaultCredentials,
On Windows, this is going to invoke the system security manager. For this test I
suggest testing with two non-empty whitelists since the empty list has extra
semantics.

https://codereview.chromium.org/1414313002/diff/480001/google_apis/gcm/tools/...
File google_apis/gcm/tools/mcs_probe.cc (right):

https://codereview.chromium.org/1414313002/diff/480001/google_apis/gcm/tools/...
google_apis/gcm/tools/mcs_probe.cc:251: MCSProbeAuthPreferences
http_auth_preferences;
http_auth_preferences_

https://codereview.chromium.org/1414313002/diff/480001/ios/chrome/browser/ios...
File ios/chrome/browser/ios_chrome_io_thread.h (right):

https://codereview.chromium.org/1414313002/diff/480001/ios/chrome/browser/ios...
ios/chrome/browser/ios_chrome_io_thread.h:121:
scoped_ptr<net::HttpAuthPreferences> http_auth_preferences;
Destruction order. http_auth_preferences above http_auth_handler_factory.

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_han...
File net/http/http_auth_handler_factory.cc (right):

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_han...
net/http/http_auth_handler_factory.cc:72: if
(IsSupportedScheme(prefs.AuthSchemes(), kBasicAuthScheme))
Each of these AuthSchemes() calls constructs and destroys a temporary vector of
strings.

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_han...
net/http/http_auth_handler_factory.cc:152: kDefaultAuthSchemes,
std::begin() / std::end()

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_han...
File net/http/http_auth_handler_factory.h (right):

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_han...
net/http/http_auth_handler_factory.h:43: // HttpAuthPreference object should
outlive the factory and any handlers it
Nit: HttpAuthPreferences

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_han...
File net/http/http_auth_handler_negotiate.cc (right):

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_han...
net/http/http_auth_handler_negotiate.cc:155: (!http_auth_preferences_ ||
use_port_ was defaulting to false everywhere. Why default to true here?

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_han...
File net/http/http_auth_handler_negotiate.h (right):

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_han...
net/http/http_auth_handler_negotiate.h:28: class HttpAuthPreferences;
Nit: sort

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_pre...
File net/http/http_auth_preferences.h (right):

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_pre...
net/http/http_auth_preferences.h:10: #include "base/macros.h"
Nit: whitespace above.

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_pre...
net/http/http_auth_preferences.h:31: virtual std::vector<std::string>
AuthSchemes() const;
Why is this returning a copy of vector of strings? The primary (non-test) usage
in HttpAuthHandlerRegistryFactory is to call it repeatedly to test if some auth
scheme is supported.

I'd suggest either returning a const& or changing this to something like `bool
IsAuthSchemeSupported(const std::string& scheme) const`.

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_pre...
net/http/http_auth_preferences.h:64: // policy can change after startup
(https://crbug/549273).
Comment belongs in //chrome.

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_pre...
net/http/http_auth_preferences.h:73: // No PrefMember for the GSSAPI library
name, since changing it after startup
Update comment to mention that the library name cannot be changed. This class is
not dependent on PrefMembers.

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_pre...
net/http/http_auth_preferences.h:82: // namespace net
Should be on previous line.

https://codereview.chromium.org/1414313002/diff/480001/net/http/mock_allow_ht...
File net/http/mock_allow_http_auth_preferences.h (right):

https://codereview.chromium.org/1414313002/diff/480001/net/http/mock_allow_ht...
net/http/mock_allow_http_auth_preferences.h:13: // An HttpAuthPreferences class
containing a URLSecurityManager which is very
Nit: Actually the URLSecurityManager is not relevant.

[aberent, 2015-12-01 14:33:25]

All requested fixes done. PTAL.

https://codereview.chromium.org/1414313002/diff/480001/android_webview/browse...
File android_webview/browser/net/aw_url_request_context_getter.h (right):

https://codereview.chromium.org/1414313002/diff/480001/android_webview/browse...
android_webview/browser/net/aw_url_request_context_getter.h:87:
scoped_ptr<net::HttpAuthPreferences> http_auth_preferences_;
On 2015/12/01 05:30:03, asanka wrote:
> The scoped_ptr<net::URLRequestContext> in line 83 actually is-a
> ContainerURLRequestContext that bundles a net::URLRequestContextStorage which
> owns a net::HttpAuthHandlerRegistryFactory which holds a reference to the
> HttpAuthPreferences object owned by http_auth_preferences_.
> 
> However the destruction order for the AwURLRequestContextGetter results in
> freeing the owned http_auth_preferences_ object prior to destroying the
> url_request_context_ object. By the time the url_request_context_ is
destroyed,
> its contained auth handler factory references freed memory.
> 
> Could you move the http_auth_preferences_ definition above
url_request_context_
> so that the destruction order is safe?

Done.

https://codereview.chromium.org/1414313002/diff/480001/chrome/browser/io_thre...
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/1414313002/diff/480001/chrome/browser/io_thre...
chrome/browser/io_thread.h:82: class HttpAuthPreferences;
On 2015/12/01 05:30:03, asanka wrote:
> Sort.

Done.

https://codereview.chromium.org/1414313002/diff/480001/chrome/browser/io_thre...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/480001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:642: void
RunOnIOThreadBlocking(base::Closure task) {
On 2015/12/01 05:30:03, asanka wrote:
> Nit: const base::Closure&

Done.

https://codereview.chromium.org/1414313002/diff/480001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:658: IOThread::Globals globals_;
On 2015/12/01 05:30:03, asanka wrote:
> Not used?

Done.

https://codereview.chromium.org/1414313002/diff/480001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:692:
base::Bind(&IOThreadTestWithIOThreadObject::CheckCanUseDefaultCredentials,
On 2015/12/01 05:30:03, asanka wrote:
> On Windows, this is going to invoke the system security manager. For this test
I
> suggest testing with two non-empty whitelists since the empty list has extra
> semantics.

Done.

https://codereview.chromium.org/1414313002/diff/480001/google_apis/gcm/tools/...
File google_apis/gcm/tools/mcs_probe.cc (right):

https://codereview.chromium.org/1414313002/diff/480001/google_apis/gcm/tools/...
google_apis/gcm/tools/mcs_probe.cc:251: MCSProbeAuthPreferences
http_auth_preferences;
On 2015/12/01 05:30:03, asanka wrote:
> http_auth_preferences_

Done.

https://codereview.chromium.org/1414313002/diff/480001/ios/chrome/browser/ios...
File ios/chrome/browser/ios_chrome_io_thread.h (right):

https://codereview.chromium.org/1414313002/diff/480001/ios/chrome/browser/ios...
ios/chrome/browser/ios_chrome_io_thread.h:121:
scoped_ptr<net::HttpAuthPreferences> http_auth_preferences;
On 2015/12/01 05:30:03, asanka wrote:
> Destruction order. http_auth_preferences above http_auth_handler_factory.

Done.

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_han...
File net/http/http_auth_handler_factory.cc (right):

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_han...
net/http/http_auth_handler_factory.cc:72: if
(IsSupportedScheme(prefs.AuthSchemes(), kBasicAuthScheme))
On 2015/12/01 05:30:03, asanka wrote:
> Each of these AuthSchemes() calls constructs and destroys a temporary vector
of
> strings.
Removed this by making IsSupportedScheme a HttpAuthPreferences member function.

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_han...
net/http/http_auth_handler_factory.cc:152: kDefaultAuthSchemes,
On 2015/12/01 05:30:03, asanka wrote:
> std::begin() / std::end()

Done.

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_han...
File net/http/http_auth_handler_factory.h (right):

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_han...
net/http/http_auth_handler_factory.h:43: // HttpAuthPreference object should
outlive the factory and any handlers it
On 2015/12/01 05:30:03, asanka wrote:
> Nit: HttpAuthPreferences

Done.

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_han...
File net/http/http_auth_handler_negotiate.cc (right):

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_han...
net/http/http_auth_handler_negotiate.cc:155: (!http_auth_preferences_ ||
On 2015/12/01 05:30:03, asanka wrote:
> use_port_ was defaulting to false everywhere. Why default to true here?

Done.

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_han...
File net/http/http_auth_handler_negotiate.h (right):

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_han...
net/http/http_auth_handler_negotiate.h:28: class HttpAuthPreferences;
On 2015/12/01 05:30:03, asanka wrote:
> Nit: sort

Done.

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_pre...
File net/http/http_auth_preferences.h (right):

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_pre...
net/http/http_auth_preferences.h:10: #include "base/macros.h"
On 2015/12/01 05:30:03, asanka wrote:
> Nit: whitespace above.

Done.

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_pre...
net/http/http_auth_preferences.h:31: virtual std::vector<std::string>
AuthSchemes() const;
On 2015/12/01 05:30:03, asanka wrote:
> Why is this returning a copy of vector of strings? The primary (non-test)
usage
> in HttpAuthHandlerRegistryFactory is to call it repeatedly to test if some
auth
> scheme is supported.
> 
> I'd suggest either returning a const& or changing this to something like `bool
> IsAuthSchemeSupported(const std::string& scheme) const`.

Done.

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_pre...
net/http/http_auth_preferences.h:64: // policy can change after startup
(https://crbug/549273).
On 2015/12/01 05:30:03, asanka wrote:
> Comment belongs in //chrome.

Done.

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_pre...
net/http/http_auth_preferences.h:73: // No PrefMember for the GSSAPI library
name, since changing it after startup
On 2015/12/01 05:30:03, asanka wrote:
> Update comment to mention that the library name cannot be changed. This class
is
> not dependent on PrefMembers.

Done.

https://codereview.chromium.org/1414313002/diff/480001/net/http/http_auth_pre...
net/http/http_auth_preferences.h:82: // namespace net
On 2015/12/01 05:30:03, asanka wrote:
> Should be on previous line.

Done.

https://codereview.chromium.org/1414313002/diff/480001/net/http/mock_allow_ht...
File net/http/mock_allow_http_auth_preferences.h (right):

https://codereview.chromium.org/1414313002/diff/480001/net/http/mock_allow_ht...
net/http/mock_allow_http_auth_preferences.h:13: // An HttpAuthPreferences class
containing a URLSecurityManager which is very
On 2015/12/01 05:30:03, asanka wrote:
> Nit: Actually the URLSecurityManager is not relevant.

Done.

[aberent, 2015-12-01 14:33:40]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-01 14:33:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414313002/500001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414313002/500001

[commit-bot: I haz the power, 2015-12-01 16:24:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-01 16:24:57]

Dry run: This issue passed the CQ dry run.

[asanka, 2015-12-01 17:23:05]

LGTM! Thanks for your patience along the way.

https://codereview.chromium.org/1414313002/diff/500001/net/http/http_auth_han...
File net/http/http_auth_handler_negotiate.cc (right):

https://codereview.chromium.org/1414313002/diff/500001/net/http/http_auth_han...
net/http/http_auth_handler_negotiate.cc:156:
+http_auth_preferences_->NegotiateEnablePort())) {
spurious '+' sign.

[cbentzel, 2015-12-01 20:56:16]

Only one concern (in io_thread.cc) the rest are mostly nits.

https://codereview.chromium.org/1414313002/diff/500001/android_webview/browse...
File android_webview/browser/net/aw_url_request_context_getter.h (right):

https://codereview.chromium.org/1414313002/diff/500001/android_webview/browse...
android_webview/browser/net/aw_url_request_context_getter.h:85:
scoped_ptr<net::HttpAuthPreferences> http_auth_preferences_;
Is this guaranteed to outlive the negotiate factory that is created.

https://codereview.chromium.org/1414313002/diff/500001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1414313002/diff/500001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:1038:
globals_->http_auth_preferences->set_server_whitelist(
Is it possible for these callbacks to be called prior to IOThread::Init (and
globals_ being non-NULL)? 

For example, it looks like PrefMember::MoveToThread loads up values on UI thread
first (but does not invoke the passed in callback?). But I'm wondering if
there's some race potential of modifications that happen between IOThread
constructor time and when CreateDefaultAuthHandleFactory is called.

https://codereview.chromium.org/1414313002/diff/500001/net/http/http_auth_pre...
File net/http/http_auth_preferences.h (right):

https://codereview.chromium.org/1414313002/diff/500001/net/http/http_auth_pre...
net/http/http_auth_preferences.h:19: /*
Nit: /* is pretty inconsistent with how commenting is done.

https://google.github.io/styleguide/cppguide.html#Comment_Style mentions /* is
valid, but should be consistent.

https://codereview.chromium.org/1414313002/diff/500001/net/http/http_auth_sch...
File net/http/http_auth_scheme.h (right):

https://codereview.chromium.org/1414313002/diff/500001/net/http/http_auth_sch...
net/http/http_auth_scheme.h:8: namespace net {
Nit: Perhaps the Scheme enum should be at the same location?

https://codereview.chromium.org/1414313002/diff/500001/net/http/url_security_...
File net/http/url_security_manager_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/500001/net/http/url_security_...
net/http/url_security_manager_unittest.cc:47: HttpAuthFilterWhitelist*
auth_filter = new HttpAuthFilterWhitelist(
Nit: Maybe make |auth_filter| a scoped_ptr?

[aberent, 2015-12-02 13:30:19]

https://codereview.chromium.org/1414313002/diff/500001/android_webview/browse...
File android_webview/browser/net/aw_url_request_context_getter.h (right):

https://codereview.chromium.org/1414313002/diff/500001/android_webview/browse...
android_webview/browser/net/aw_url_request_context_getter.h:85:
scoped_ptr<net::HttpAuthPreferences> http_auth_preferences_;
On 2015/12/01 20:56:16, cbentzel wrote:
> Is this guaranteed to outlive the negotiate factory that is created.

Yes. The factory is owned by the URLRequestContext, which in turn is owned by
the AwURLRequestContextGetter. (member url_request_context_).
http_auth_preferences_ is only updated by CreateNegotiateAuthHandlerFactory,
which, in turn, is only called by InitializeURLRequestContext. Immediately after
the call the url_request_context is updated with the new factory using the new
preferences.

This is less obvious than it should be, but before this CL the same logic
applied to url_security_manager_, so I don't intend to improve this in this CL.

https://codereview.chromium.org/1414313002/diff/500001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1414313002/diff/500001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:1038:
globals_->http_auth_preferences->set_server_whitelist(
On 2015/12/01 20:56:16, cbentzel wrote:
> Is it possible for these callbacks to be called prior to IOThread::Init (and
> globals_ being non-NULL)? 
> 
> For example, it looks like PrefMember::MoveToThread loads up values on UI
thread
> first (but does not invoke the passed in callback?). But I'm wondering if
> there's some race potential of modifications that happen between IOThread
> constructor time and when CreateDefaultAuthHandleFactory is called.

No. MoveToThread means that any changes to the preferences will queue calls to
these on the IO thread's message queue, but nothing will be taken off the IO
thread message queue until the thread is started. Init is called as part of
thread start-up, and before anything is taken off the thread's message queue.

https://codereview.chromium.org/1414313002/diff/500001/net/http/http_auth_han...
File net/http/http_auth_handler_negotiate.cc (right):

https://codereview.chromium.org/1414313002/diff/500001/net/http/http_auth_han...
net/http/http_auth_handler_negotiate.cc:156:
+http_auth_preferences_->NegotiateEnablePort())) {
On 2015/12/01 17:23:05, asanka wrote:
> spurious '+' sign.

Done.

https://codereview.chromium.org/1414313002/diff/500001/net/http/http_auth_pre...
File net/http/http_auth_preferences.h (right):

https://codereview.chromium.org/1414313002/diff/500001/net/http/http_auth_pre...
net/http/http_auth_preferences.h:19: /*
On 2015/12/01 20:56:16, cbentzel wrote:
> Nit: /* is pretty inconsistent with how commenting is done.
> 
> https://google.github.io/styleguide/cppguide.html#Comment_Style mentions /* is
> valid, but should be consistent.

Done.

https://codereview.chromium.org/1414313002/diff/500001/net/http/http_auth_sch...
File net/http/http_auth_scheme.h (right):

https://codereview.chromium.org/1414313002/diff/500001/net/http/http_auth_sch...
net/http/http_auth_scheme.h:8: namespace net {
On 2015/12/01 20:56:16, cbentzel wrote:
> Nit: Perhaps the Scheme enum should be at the same location?

Probably a good idea, but I would prefer not to in this CL, since doing so adds
lots more files (with new owners in some cases) to the CL.

https://codereview.chromium.org/1414313002/diff/500001/net/http/url_security_...
File net/http/url_security_manager_unittest.cc (right):

https://codereview.chromium.org/1414313002/diff/500001/net/http/url_security_...
net/http/url_security_manager_unittest.cc:47: HttpAuthFilterWhitelist*
auth_filter = new HttpAuthFilterWhitelist(
On 2015/12/01 20:56:16, cbentzel wrote:
> Nit: Maybe make |auth_filter| a scoped_ptr?

Done.

[aberent, 2015-12-02 13:31:45]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-02 13:33:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414313002/520001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414313002/520001

[commit-bot: I haz the power, 2015-12-02 14:38:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-02 14:38:57]

Dry run: This issue passed the CQ dry run.

[cbentzel, 2015-12-02 20:27:28]

On 2015/12/02 14:38:57, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

chrome/browser/io_thread LGTM

[aberent, 2015-12-03 09:59:31]

The CQ bit was checked by aberent@chromium.org

[aberent, 2015-12-03 09:59:32]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
mlerman@chromium.org, zea@chromium.org, pkasting@chromium.org,
sdefresne@chromium.org, torne@chromium.org, asanka@chromium.org
Link to the patchset: https://codereview.chromium.org/1414313002/#ps520001
(title: "Respond to cbentzel@'s comments.")

[commit-bot: I haz the power, 2015-12-03 09:59:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414313002/520001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414313002/520001

[commit-bot: I haz the power, 2015-12-03 10:20:26]

Description was changed from

==========
Allow dynamic updating of authentication policies

This allows a number of the authentication policies to be updated
dynamically after the IO thread has been created.

BUG=545534
==========

to

==========
Allow dynamic updating of authentication policies

This allows a number of the authentication policies to be updated
dynamically after the IO thread has been created.

BUG=545534
==========

[commit-bot: I haz the power, 2015-12-03 10:20:27]

Committed patchset #28 (id:520001)

[commit-bot: I haz the power, 2015-12-03 10:21:29]

Description was changed from

==========
Allow dynamic updating of authentication policies

This allows a number of the authentication policies to be updated
dynamically after the IO thread has been created.

BUG=545534
==========

to

==========
Allow dynamic updating of authentication policies

This allows a number of the authentication policies to be updated
dynamically after the IO thread has been created.

BUG=545534

Committed: https://crrev.com/bba302d0ced354b71853bbf3582f4034ea825b9b
Cr-Commit-Position: refs/heads/master@{#362940}
==========

[commit-bot: I haz the power, 2015-12-03 10:21:31]

Patchset 28 (id:??) landed as
https://crrev.com/bba302d0ced354b71853bbf3582f4034ea825b9b
Cr-Commit-Position: refs/heads/master@{#362940}