DCHECK firing on policy_enforcer_ from SSLClientSocketImpl() |
||||||||||||
Issue descriptionVersion: 53.0.2775.0 OS: All What steps will reproduce the problem? (1) Start chrome with the media-router flag enabled on a network with cast devices. What is the expected output? Chrome starts normally. What do you see instead? DCHECK fires when creating a cast channel. This is the stack trace: [88197:88287:0621/153135:FATAL:ssl_client_socket_impl.cc(523)] Check failed: policy_enforcer_. #0 0x7f044464cdbe base::debug::StackTrace::StackTrace() #1 0x7f044466d6fb logging::LogMessage::~LogMessage() #2 0x7f0443f27744 net::SSLClientSocketImpl::SSLClientSocketImpl() #3 0x7f044413581a net::(anonymous namespace)::DefaultClientSocketFactory::CreateSSLClientSocket() #4 0x7f04454476da extensions::api::cast_channel::CastSocketImpl::CreateSslSocket() #5 0x7f04454490dd extensions::api::cast_channel::CastSocketImpl::DoSslConnect() #6 0x7f044544847f extensions::api::cast_channel::CastSocketImpl::DoConnectLoop() #7 0x7f044415040d net::TCPClientSocket::DidCompleteConnect() #8 0x7f0444151fb3 net::TCPSocketPosix::ConnectCompleted() #9 0x7f0444144050 net::SocketPosix::ConnectCompleted() #10 0x7f0444143ef4 net::SocketPosix::OnFileCanWriteWithoutBlocking() #11 0x7f044467b495 base::MessagePumpLibevent::OnLibeventNotification() #12 0x7f0444713907 event_base_loop #13 0x7f044467b61e base::MessagePumpLibevent::Run() #14 0x7f0444677ae1 base::MessageLoop::RunHandler() #15 0x7f04446a6c00 base::RunLoop::Run() #16 0x7f0444676b10 base::MessageLoop::Run() #17 0x7f0441fd8786 content::BrowserThreadImpl::IOThreadRun() #18 0x7f0441fd8991 content::BrowserThreadImpl::Run() #19 0x7f04446dbb2b base::Thread::ThreadMain() #20 0x7f04446d5475 base::(anonymous namespace)::ThreadFunc() #21 0x7f043c948184 start_thread #22 0x7f043b3d737d clone It seems that crrev.com/d6de8300 added this DCHECK, but I'm not sure what the most appropriate way to add a CTPolicyEnforcer to the cast channel code is.
,
Jun 21 2016
This means that you have no unittests running on any waterfall for this feature. You should treat that as a P-0 in order to prevent future regressions. That's not a good state to be in - such breakages are expected. For Chromecast, https://chromium.googlesource.com/chromium/src/+/d6de8300%5E%21/#F4 was a sufficient change. Doing something like https://cs.chromium.org/chromium/src/extensions/browser/api/cast_channel/cast_socket.cc?rcl=0&l=188 is sufficient While I'd be happy to work up a CL, the absence of tests for this case make it difficult to test beyond compilation. However, it should be a 4 line change - 2 to add the std::unique_ptr members, and 2 to supply them in the SSLContext
,
Jun 23 2016
Ping? How can I help?
,
Jun 23 2016
btolsch@ has a patch out for review. (https://codereview.chromium.org/2082213002/)
,
Jun 23 2016
,
Jun 23 2016
,
Jun 23 2016
+govind - this was crashing Chrome on launch, 100% of the time for me. Can we / should we respin 2776 once the fix has landed?
,
Jun 23 2016
Right, EnableMediaRouter is enabled for 100% Canary/Dev, and this crash triggers when EnableMediaRouter is set, so... we should probably respin / not ship.
,
Jun 23 2016
In the interim, can we turn the experiment to 0%?
,
Jun 23 2016
,
Jun 23 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f0df1d51b68f008d04d38f53b8f7758407375a65 commit f0df1d51b68f008d04d38f53b8f7758407375a65 Author: btolsch <btolsch@chromium.org> Date: Thu Jun 23 01:51:39 2016 Add CTVerifier and CTPolicyEnforcer to cast channel ssl socket This change adds the CTVerifier and CTPolicyEnforcer parameters to the ssl context when initializing an ssl socket for a cast channel. These were became required by https://codereview.chromium.org/2067843003 but this code wasn't updated because it is not used in tests. BUG= 622098 Review-Url: https://codereview.chromium.org/2082213002 Cr-Commit-Position: refs/heads/master@{#401514} [modify] https://crrev.com/f0df1d51b68f008d04d38f53b8f7758407375a65/extensions/browser/api/cast_channel/cast_socket.cc [modify] https://crrev.com/f0df1d51b68f008d04d38f53b8f7758407375a65/extensions/browser/api/cast_channel/cast_socket.h
,
Jun 23 2016
,
Jun 23 2016
We are still seeing couple of crash instances on Latest Canary#53.0.2777.0. 53.0.2777.0 1.12% 8 (Mac) 53.0.2777.0 3.52% 27 (Win) List of chrome builds having this issue: https://crash.corp.google.com/browse?q=product.name%3D%27Chrome%27%20AND%20custom_data.ChromeCrashProto.ptype%3D%27browser%27%20AND%20custom_data.ChromeCrashProto.magic_signature_1.name%3D%27net%3A%3ASSLClientSocketImpl%3A%3AVerifyCT%27&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D btolsch@, could you please take a look? Thank you!
,
Jun 23 2016
A new bug may be appropriate: only 30% of these crashes are related to EnableMediaRouter, which is what btolsch was looking at. All of the remaining crashes are on Windows (AFAICT). I'm looking into the 53.0.2777.0 dumps to see what I can find
,
Jun 23 2016
Users experienced this crash on the following builds: Win Canary 53.0.2777.0 - 8.71 CPM, 80 reports, 21 clients (signature net::SSLClientSocketImpl::VerifyCT) Mac Canary 53.0.2777.0 - 7.06 CPM, 9 reports, 9 clients (signature net::SSLClientSocketImpl::VerifyCT) If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates. - Go/Fracas
,
Jun 23 2016
None of the crashes related to EnableMediaRouter have any cast_channel code in the stack traces, which is what prompted this bug.
,
Jun 24 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/be81cd6044e1bc5d846cb52712fc2ee15779a85a commit be81cd6044e1bc5d846cb52712fc2ee15779a85a Author: rsleevi <rsleevi@chromium.org> Date: Fri Jun 24 01:38:59 2016 Make DCHECKs into CHECKs for CT policy code Unfortunately, there's some code in the tree making SSL and QUIC sockets without any test coverage, and supplying NULL objects. Make the DCHECKs into CHECKs, so that they can be quickly isolated, as otherwise the crash does not trigger until the socket is in the asynchronous cert verification mode, which does not have enough context to narrow down which caller created the socket. BUG= 622098 R=eroman@chromium.org Review-Url: https://codereview.chromium.org/2095823002 Cr-Commit-Position: refs/heads/master@{#401775} [modify] https://crrev.com/be81cd6044e1bc5d846cb52712fc2ee15779a85a/net/quic/crypto/proof_verifier_chromium.cc [modify] https://crrev.com/be81cd6044e1bc5d846cb52712fc2ee15779a85a/net/socket/ssl_client_socket_impl.cc
,
Jun 27 2016
Seems like we are not seeing any new crash instances on >53.0.2777.0. https://crash.corp.google.com/browse?q=product.name%3D%27Chrome%27%20AND%20custom_data.ChromeCrashProto.ptype%3D%27browser%27%20AND%20custom_data.ChromeCrashProto.magic_signature_1.name%3D%27net%3A%3ASSLClientSocketImpl%3A%3AVerifyCT%27&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D Thank you!
,
Jun 27 2016
Closing this out; two more callers turned up in new bugs, and code is addressing those. |
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by imch...@chromium.org
, Jun 21 2016