Either an issue with WriteToFileNetLogObserver or with CronetEnvironment / CronetEnvironment's consumer, presumably.

Hi Yixin,

Since you were the last who modified the BrokenAlternativeServices code could you please take a look at the crash and let us why it can happen and whether we are doing something wrong in Cronet? It consistently happens when we try to stop the netlog in one of our embedder applications. The release version works fine. Please feel free to reassign the bug back to me.

Thanks,
Andrei

Hard to say without knowing what this application is doing. It looks like an alternative service associated with a blank host (a SchemeHostPort where host_ is empty string) was added to HttpServerPropertiesImpl.

The offending DCHECK existed before my change; my change simply moved it from HttpServerPropertiesImpl to BrokenAlternativeServices, so I don't think my change is causing this. Is there any way to confirm that?

zhongyi@, can you add anything?

Here's what our Cronet configuration code looks like:

    sQuicHosts = @[@"foo.googlevideo.com", @"foo.c.youtube.com"]

    for (NSString *host in sQuicHosts) {
      [Cronet addQuicHint:host port:443 altPort:443];
    }

    [Cronet setQuicEnabled:YES];
    [Cronet setHttp2Enabled:YES];
    [Cronet setUserAgent:YTUserAgent() partial:NO];
    [Cronet setHttpCacheType:CRNHttpCacheTypeDisabled];
    [Cronet start];

Oh, correction, sQuicHosts also contains "https://youtubei.googleapis.com/"

I wonder if the "https://" prefix is causing the problem. I'll try removing this entry and see if it does anything.

Yes, that was the bug. When I replaced "https://youtubei.googleapis.com/" with "youtubei.googleapis.com" we no longer hit the assertion.

Would it be possible to add the bad host to the debug message? That would have made this way easier to debug.

Also, it seems really weird to me that the crash occurs upon stopping the log rather than at some other point, like during configuration or when starting Cronet. We wouldn't have even noticed that the hosts were misconfigured unless we happened to use the logging functionality.

Hrm, I've also found that we pass empty host when we add quic hints: https://cs.chromium.org/chromium/src/components/cronet/ios/cronet_environment.mm?rcl=ca205ddf1a960d002da75bbcdd8243757a77e817&l=340 and appear to ignore altPort completely, so there seem to be several bugs on Cronet side.

Lily has a CL to refactor Cronet configuration, and that should fix this issue.

Tracing bottom up, the DCHECK fires as the alternative_service has an empty host, which we thought should be replace by server.host() which possibly is also an empty host.

https://cs.chromium.org/chromium/src/net/http/http_server_properties_impl.cc?type=cs&l=503

in HttpServerPropertiesImpl::GetAlternativeServiceInfoAsValue() {
  for (const auto& alternative_service_map_item : alternative_service_map_) {
    std::unique_ptr<base::ListValue> alternative_service_list(
        new base::ListValue);
    const url::SchemeHostPort& server = alternative_service_map_item.first;
  ...

    if (alternative_service.host.empty()) {
      alternative_service.host = server.host();
    }
    if (IsAlternativeServiceBroken(alternative_service)) {
      alternative_service_string.append(" (broken)");
    }
   ...
}
This could source from the SetAlternativeServices. Combing with quic hints setup in cronet:

CronetEnvironment::InitializeOnNetworkThread() {
  // ...
  for (const auto& quic_hint : quic_hints_) {
    net::AlternativeService alternative_service(net::kProtoQUIC, "",
                                                quic_hint.port());
    url::SchemeHostPort quic_hint_server("https", quic_hint.host(),
                                         quic_hint.port());
    http_server_properties->SetQuicAlternativeService(
        quic_hint_server, alternative_service, base::Time::Max(),
        net::QuicVersionVector());
  }
  // ...
}

We have empty host specified when set up alternative service, which was then saved to alternative_service_map_. 

OH, that empty host is in alternative service, shouldn't cause issues here.

Well, the quic_hint_server is added via 

void CronetEnvironment::AddQuicHint(const std::string& host,
                                    int port,
                                    int alternate_port) {
  DCHECK(port == alternate_port);
  quic_hints_.push_back(net::HostPortPair(host, port));
}

There was no check/processing on the host field, which might take an incorrect host and cause issues. 

lilyhoughton@: I think the current crash at net::BrokenAlternativeServices::IsAlternativeServiceBroken is just a trigger. The root cause is that when setting QUIC alternative service, the server field is empty and gets set to |alternative_service_map_|. This is a parsing failure from cronet when callsite mistakenly append "https://" prefix in the host field, this is not handled properly. You could verify this by adding DCHECKs when an entry is added to |quic_hints_| and when SetQuicAlternativeServices.

https://cs.chromium.org/chromium/src/components/cronet/ios/cronet_environment.mm?type=cs&q=CronetEnvironment::InitializeOnNetworkThrea&sq=package:chromium&l=228

https://cs.chromium.org/chromium/src/components/cronet/ios/cronet_environment.mm?type=cs&q=CronetEnvironment::InitializeOnNetworkThrea&sq=package:chromium&l=358

To repro the crash, have you try enable NetLog? It seems like GetAlternativeServiceInfoAsValue will be invoked in net logging. You might be able to trigger the crash if enabled. 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5a1a911b0e131701940bacb3fcbef6951e5a6e64

commit 5a1a911b0e131701940bacb3fcbef6951e5a6e64
Author: Lily Houghton <lilyhoughton@chromium.org>
Date: Thu Aug 10 17:21:55 2017

[Cronet] Add check to addQuicHint

This should help fix Youtube's crash, and hopefully prevent similar
problems in the future.

Bug:  739979 
Change-Id: Ifd0099439b1e33e85da4f73a522b2ba5c5bc063b
Reviewed-on: https://chromium-review.googlesource.com/576287
Reviewed-by: Misha Efimov <mef@chromium.org>
Commit-Queue: Lily Houghton <lilyhoughton@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493440}
[modify] https://crrev.com/5a1a911b0e131701940bacb3fcbef6951e5a6e64/components/cronet/ios/Cronet.h
[modify] https://crrev.com/5a1a911b0e131701940bacb3fcbef6951e5a6e64/components/cronet/ios/Cronet.mm
[modify] https://crrev.com/5a1a911b0e131701940bacb3fcbef6951e5a6e64/components/cronet/ios/cronet_environment.mm
[modify] https://crrev.com/5a1a911b0e131701940bacb3fcbef6951e5a6e64/components/cronet/ios/test/BUILD.gn
[add] https://crrev.com/5a1a911b0e131701940bacb3fcbef6951e5a6e64/components/cronet/ios/test/cronet_quic_test.mm