Add IsAlternativeServiceBroken(), remove is_broken.

Add IsAlternativeServiceBroken(), remove is_broken.

Add HttpServerProperties::IsAlternativeServiceBroken(), remove
AlternateProtocolInfo.is_broken.  An alternative service can be broken by
itself, not with respect to an origin.

BUG=392576
R=rch@chromium.org

Committed: https://crrev.com/c0844e78ad3dd640f6a2a84f57681ead31647b07
Cr-Commit-Position: refs/heads/master@{#320116}

[Bence, 2015-03-10 16:52:49]

bnc@chromium.org changed reviewers:
+ rch@chromium.org

[Bence, 2015-03-10 16:52:50]

Ryan, PTAL.  Thanks.

[Ryan Hamilton, 2015-03-10 21:05:46]

Looks great. Just a few small comments. Starting to be able to see the future!

https://codereview.chromium.org/995013004/diff/1/net/http/http_server_propert...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/995013004/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:348: return
broken_alternative_services_.find(alternative_service) !=
Can you do this instead:

return ContainsKey(broken_alternative_services_, alternative_server);?

(I'm not sure if ContainsKey works for linked_hash_map. If it does, let's use
it)

https://codereview.chromium.org/995013004/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:370:
ignore_result(broken_alternative_services_.erase(alternative_service));
is ignore_result required?

https://codereview.chromium.org/995013004/diff/1/net/http/http_server_propert...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/995013004/diff/1/net/http/http_server_propert...
net/http/http_server_properties_manager.cc:788:
SaveAlternateProtocolToServerPrefs(server_pref.alternate_protocol,
Can you check IsBroken here to resolve your TODO below?

[Bence, 2015-03-10 21:46:13]

Ryan, PTAL.

https://codereview.chromium.org/995013004/diff/1/net/http/http_server_propert...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/995013004/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:348: return
broken_alternative_services_.find(alternative_service) !=
On 2015/03/10 21:05:46, Ryan Hamilton wrote:
> Can you do this instead:
> 
> return ContainsKey(broken_alternative_services_, alternative_server);?
> 
> (I'm not sure if ContainsKey works for linked_hash_map. If it does, let's use
> it)

Done.

https://codereview.chromium.org/995013004/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:370:
ignore_result(broken_alternative_services_.erase(alternative_service));
On 2015/03/10 21:05:46, Ryan Hamilton wrote:
> is ignore_result required?

erase(Key) returns size_t, so it makes sense, but it's not marked
WARN_UNUSED_RESULT, so it's not required.   Do you want me to remove
ignore_result?

https://codereview.chromium.org/995013004/diff/1/net/http/http_server_propert...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/995013004/diff/1/net/http/http_server_propert...
net/http/http_server_properties_manager.cc:788:
SaveAlternateProtocolToServerPrefs(server_pref.alternate_protocol,
On 2015/03/10 21:05:46, Ryan Hamilton wrote:
> Can you check IsBroken here to resolve your TODO below?

I'd rather not, because it does not belong here, but I can pass |server| to
SaveAlternateProtocolToServerPrefs.  Note that this is all temporary, because
soon we will be passing AlternativeServices around.

[Ryan Hamilton, 2015-03-10 22:15:27]

https://codereview.chromium.org/995013004/diff/1/net/http/http_server_propert...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/995013004/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:370:
ignore_result(broken_alternative_services_.erase(alternative_service));
On 2015/03/10 21:46:13, Bence wrote:
> On 2015/03/10 21:05:46, Ryan Hamilton wrote:
> > is ignore_result required?
> 
> erase(Key) returns size_t, so it makes sense, but it's not marked
> WARN_UNUSED_RESULT, so it's not required.   Do you want me to remove
> ignore_result?

I think I'd remove it since we don't typically do this when we erase things from
containers.

https://codereview.chromium.org/995013004/diff/1/net/http/http_server_propert...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/995013004/diff/1/net/http/http_server_propert...
net/http/http_server_properties_manager.cc:788:
SaveAlternateProtocolToServerPrefs(server_pref.alternate_protocol,
On 2015/03/10 21:46:13, Bence wrote:
> On 2015/03/10 21:05:46, Ryan Hamilton wrote:
> > Can you check IsBroken here to resolve your TODO below?
> 
> I'd rather not, because it does not belong here, but I can pass |server| to
> SaveAlternateProtocolToServerPrefs.  Note that this is all temporary, because
> soon we will be passing AlternativeServices around.

The only reason I suggest it is that as currently written, this CL changes the
behavior slightly, and doing adding the check here, while not the best solution,
would leave behavior unchanged. Even if it's temporary it seems like a good idea
not to change behavior.

[Bence, 2015-03-10 22:54:41]

Ryan, thanks for your feedback.  PTYAL.

https://codereview.chromium.org/995013004/diff/1/net/http/http_server_propert...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/995013004/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:370:
ignore_result(broken_alternative_services_.erase(alternative_service));
On 2015/03/10 22:15:26, Ryan Hamilton wrote:
> On 2015/03/10 21:46:13, Bence wrote:
> > On 2015/03/10 21:05:46, Ryan Hamilton wrote:
> > > is ignore_result required?
> > 
> > erase(Key) returns size_t, so it makes sense, but it's not marked
> > WARN_UNUSED_RESULT, so it's not required.   Do you want me to remove
> > ignore_result?
> 
> I think I'd remove it since we don't typically do this when we erase things
from
> containers.

Done.

https://codereview.chromium.org/995013004/diff/1/net/http/http_server_propert...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/995013004/diff/1/net/http/http_server_propert...
net/http/http_server_properties_manager.cc:788:
SaveAlternateProtocolToServerPrefs(server_pref.alternate_protocol,
On 2015/03/10 22:15:26, Ryan Hamilton wrote:
> On 2015/03/10 21:46:13, Bence wrote:
> > On 2015/03/10 21:05:46, Ryan Hamilton wrote:
> > > Can you check IsBroken here to resolve your TODO below?
> > 
> > I'd rather not, because it does not belong here, but I can pass |server| to
> > SaveAlternateProtocolToServerPrefs.  Note that this is all temporary,
because
> > soon we will be passing AlternativeServices around.
> 
> The only reason I suggest it is that as currently written, this CL changes the
> behavior slightly, and doing adding the check here, while not the best
solution,
> would leave behavior unchanged. Even if it's temporary it seems like a good
idea
> not to change behavior.

Good call.  I think with this change the behavior is mostly unchanged.  (Except
for the grotesque case of host:80 and host:81 both having host:443 as an
alternate protocol, now if it's broken for one, it's broken for the other one
too.)

[Ryan Hamilton, 2015-03-10 23:11:52]

lgtm modulo one suggestion I should have noticed earlier.

https://codereview.chromium.org/995013004/diff/40001/net/http/http_server_pro...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/995013004/diff/40001/net/http/http_server_pro...
net/http/http_server_properties_manager.cc:841: if
(IsAlternativeServiceBroken(alternative_service))
I'm sorry for not noticing this initally, but I think this might better live at
line 762, next to 

 if (!IsAlternateProtocolValid(port_alternate_protocol.protocol))

[Bence, 2015-03-11 02:47:34]

The CQ bit was checked by bnc@chromium.org

[Bence, 2015-03-11 02:47:35]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org
Link to the patchset: https://codereview.chromium.org/995013004/#ps60001 (title:
"Brokenness test in Manager.")

[Bence, 2015-03-11 02:47:44]

Thanks.

https://codereview.chromium.org/995013004/diff/40001/net/http/http_server_pro...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/995013004/diff/40001/net/http/http_server_pro...
net/http/http_server_properties_manager.cc:841: if
(IsAlternativeServiceBroken(alternative_service))
On 2015/03/10 23:11:52, Ryan Hamilton wrote:
> I'm sorry for not noticing this initally, but I think this might better live
at
> line 762, next to 
> 
>  if (!IsAlternateProtocolValid(port_alternate_protocol.protocol)) 

Good catch.

Done.

[commit-bot: I haz the power, 2015-03-11 02:47:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/995013004/60001

[commit-bot: I haz the power, 2015-03-11 07:32:37]

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

[commit-bot: I haz the power, 2015-03-11 07:32:38]

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

[Bence, 2015-03-11 13:23:26]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2015-03-11 13:23:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/995013004/60001

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

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

[commit-bot: I haz the power, 2015-03-11 18:21:59]

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

[Bence, 2015-03-11 18:49:04]

Committed patchset #4 (id:60001) manually as
c0844e78ad3dd640f6a2a84f57681ead31647b07 (presubmit successful).

[commit-bot: I haz the power, 2015-03-11 18:50:00]

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

[Bence, 2015-03-11 18:52:24]

On 2015/03/11 18:50:00, I haz the power (commit-bot) wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/c0844e78ad3dd640f6a2a84f57681ead31647b07
> Cr-Commit-Position: refs/heads/master@{#320116}

I believe that CQ failed because of some infra problem, not because of the CL,
that's why I landed manually.

[Lei Zhang, 2015-03-11 20:59:39]

On 2015/03/11 18:52:24, Bence wrote:
> On 2015/03/11 18:50:00, I haz the power (commit-bot) wrote:
> > Patchset 4 (id:??) landed as
> > https://crrev.com/c0844e78ad3dd640f6a2a84f57681ead31647b07
> > Cr-Commit-Position: refs/heads/master@{#320116}
> 
> I believe that CQ failed because of some infra problem, not because of the CL,
> that's why I landed manually.

Erm, if you look in the logs carefully, you'll see:

[19945:19945:0311/110444:FATAL:http_server_properties_manager.cc(209)] Check
failed: network_task_runner_->RunsTasksOnCurrentThread()

Now my ToT build fails the same way.

[Lei Zhang, 2015-03-11 21:00:42]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/999893002/ by thestig@chromium.org.

The reason for reverting is: Linux bots failing with:

Check failed: network_task_runner_->RunsTasksOnCurrentThread().

[Lei Zhang, 2015-03-11 21:02:23]

For the record:
http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29...
fails in the same way that the trybots failed.

[Bence, 2015-03-11 21:06:30]

On 2015/03/11 20:59:39, Lei Zhang wrote:
> On 2015/03/11 18:52:24, Bence wrote:
> > On 2015/03/11 18:50:00, I haz the power (commit-bot) wrote:
> > > Patchset 4 (id:??) landed as
> > > https://crrev.com/c0844e78ad3dd640f6a2a84f57681ead31647b07
> > > Cr-Commit-Position: refs/heads/master@{#320116}
> > 
> > I believe that CQ failed because of some infra problem, not because of the
CL,
> > that's why I landed manually.
> 
> Erm, if you look in the logs carefully, you'll see:
> 
> [19945:19945:0311/110444:FATAL:http_server_properties_manager.cc(209)] Check
> failed: network_task_runner_->RunsTasksOnCurrentThread()
> 
> Now my ToT build fails the same way.

I'm sorry.  Thanks for reverting.