Issue metadata
Sign in to add a comment
|
SSLClientSocketImpl crash possibly related to TLS CECPQ1 |
||||||||||||||||||||||
Issue descriptionThere are new failures in org.chromium.net.BidirectionalStreamTest: https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Builder/builds/2595 Relevant Stack Trace: Map::find(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) /b/build/slave/Android_Cronet_Builder/build/src/third_party/android_tools/ndk/sources/cxx-stl/llvm-libc++/libcxx/include/map:1090 0006e219 base::FeatureList::IsFeatureEnabled(base::Feature const&)+40 /b/build/slave/Android_Cronet_Builder/build/src/base/feature_list.cc:207 000b7d15 net::SSLClientSocketImpl::Init()+516 /b/build/slave/Android_Cronet_Builder/build/src/net/socket/ssl_client_socket_impl.cc:977 000b8d6d net::SSLClientSocketImpl::Connect(base::Callback<void (int), (base::internal::CopyMode)1> const&)+64 /b/build/slave/Android_Cronet_Builder/build/src/net/socket/ssl_client_socket_impl.cc:633 00165d0b net::SSLConnectJob::DoSSLConnect()+362 /b/build/slave/Android_Cronet_Builder/build/src/net/socket/ssl_client_socket_pool.cc:321 0016707d net::SSLConnectJob::DoLoop(int)+232 /b/build/slave/Android_Cronet_Builder/build/src/net/socket/ssl_client_socket_pool.cc:205
,
Jun 29 2016
Cronet is probably not setting up base::FeatureList here. We should do the usual thing of not using base::FeatureList and friends (+asvitkine FYI) in //net and plumbing it through SSLConfig and everything, obnoxious as that is.
,
Jun 29 2016
I think it would be simpler to just set up an (empty) base::FeatureList in the cronet init code, rather than prohibiting use of base::Feature in net/
,
Jun 29 2016
Basically, just call base::FeatureList::InitializeInstance(std::string(), std::string()) in Cronet initialization code.
,
Jun 29 2016
That seems reasonable to me as well. I've never been a fan of just how much effort it is to do experiments in //net, especially since we already set up enough of that stuff that UMA works fine. Too much plumbing for stuff that truly has no need for anything complicated.
,
Jun 29 2016
I will add the initialization of the FeatureList to cronet_library_loader.cc::CronetInitOnMainThread().
,
Jun 29 2016
(The CECPQ1 CL was reverted in https://codereview.chromium.org/2107273002/)
,
Jun 29 2016
,
Jun 29 2016
Since we're branching tomorrow, I'm going to land a revert of the CECPQ1 change now and we'll try again with more plumbing.
,
Jun 29 2016
Reverted in https://codereview.chromium.org/2107273002/
,
Jun 29 2016
I can confirm that initializing the feature list in Cronet start code has fixed the Bidirectional Stream crash. Here is the CL if it still relevant: https://codereview.chromium.org/2110803002
,
Jun 29 2016
I think initializing feature list is a right thing to do despite the revert.
,
Jun 29 2016
The sheriff reverted my revert (although it seems unlikely that'll solve any problems). The change to initialise FeatureList is still in CQ however, so this will break again because the revert-revert has landed and will be fixed when CQ lands https://codereview.chromium.org/2110803002. (I can revert the revert-revert if you like.)
,
Jun 29 2016
The change has landed. We should be good.
,
Jun 29 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3180f53c36b834e7571ac1e6938f5d29806dec8c commit 3180f53c36b834e7571ac1e6938f5d29806dec8c Author: kapishnikov <kapishnikov@chromium.org> Date: Wed Jun 29 17:39:50 2016 Initialize base::FeatureList on Cronet init BUG= 624435 Review-Url: https://codereview.chromium.org/2110803002 Cr-Commit-Position: refs/heads/master@{#402856} [modify] https://crrev.com/3180f53c36b834e7571ac1e6938f5d29806dec8c/components/cronet/android/cronet_library_loader.cc
,
Jun 29 2016
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by kapishnikov@chromium.org
, Jun 29 2016