Log how often DTLS negotiation failed because of incompatible ciphersuites.

Log how often DTLS negotiation failed because of incompatible ciphersuites.

Log the DTLS handshake error code in OpenSSLStreamAdapter.
Forward the error code to WebRTCSession with the Signals.
This part is only for the WebRTC native code.
To make it work, need another CL for Chromium.

BUG=webrtc:5959
Committed: https://crrev.com/d82eee0675cf971cdc5f02340b5799bb303449e3
Cr-Commit-Position: refs/heads/master@{#13940}

[Zhi Huang, 2016-07-21 21:44:43]

zhihuang@webrtc.org changed reviewers:
+ honghaiz@webrtc.org, skvlad@webrtc.org

[Zhi Huang, 2016-07-21 21:44:44]

I noticed that there are some compiled issues on some of the trybots. The error
is that :
"webrtc/base/sslstreamadapter.h:89:3: error: [chromium-style] Complex
constructor has an inlined body.
  explicit SSLStreamAdapter(StreamInterface* stream)"

But it seems that the error is not caused by this CL. Please correct me if I am
wrong.

[skvlad, 2016-07-22 01:25:14]

https://codereview.webrtc.org/2167363002/diff/1/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/2167363002/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:2014: metrics_observer_->IncrementEnumCounter(
Please add a DCHECK(metrics_observer_ != NULL)

https://codereview.webrtc.org/2167363002/diff/1/webrtc/base/sslstreamadapter.h
File webrtc/base/sslstreamadapter.h (right):

https://codereview.webrtc.org/2167363002/diff/1/webrtc/base/sslstreamadapter....
webrtc/base/sslstreamadapter.h:80: enum class SSLHandshakeError {
ERR_INCOMPATIBLE_CIPHERSUITE, ERR_OTHER };
Would it be better to make ERR_OTHER the first value of the enum?
The UMA methods take a maximum value but I'm not sure if they can handle
reordering of enum values correctly. E.g. if subsequently we add another value
ERR_NEW before ERR_OTHER - it would be hard to interpret if a value of "1" means
ERR_OTHER (for older versions) or ERR_NEW (for newer ones).

To maximize forward compatibility, I'd rather define it this way:
enum class SSLHandshakeError { UNKNOWN, INCOMPATIBLE_CIPHERSUITE, MAX_VALUE }
and use MAX_VALUE in calls to UMA.

Furthermore, you don't need prefixes (ERR_...) for enum class types as they are
already in their own namespace unlike regular enums.

[skvlad, 2016-07-22 01:28:08]

On 2016/07/21 21:44:44, Zhi Huang wrote:
> I noticed that there are some compiled issues on some of the trybots. The
error
> is that :
> "webrtc/base/sslstreamadapter.h:89:3: error: [chromium-style] Complex
> constructor has an inlined body.
>   explicit SSLStreamAdapter(StreamInterface* stream)"
> 
> But it seems that the error is not caused by this CL. Please correct me if I
am
> wrong.

I think it might be - you have added a field that has a non-trivial constructor,
while there was none before. It might be better to make the constructor
non-inline and move the empty body to the .cc file.

[honghaiz3, 2016-07-22 21:35:26]

https://codereview.webrtc.org/2167363002/diff/1/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/2167363002/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:2014: metrics_observer_->IncrementEnumCounter(
On 2016/07/22 01:25:14, skvlad wrote:
> Please add a DCHECK(metrics_observer_ != NULL)
I think you should add a condition on it. 
if (metrics_observer_) {
...

https://codereview.webrtc.org/2167363002/diff/1/webrtc/base/opensslstreamadap...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/2167363002/diff/1/webrtc/base/opensslstreamadap...
webrtc/base/opensslstreamadapter.cc:859: if ((err_code = ERR_peek_last_error())
!= 0 &&
you could do
int err_code = ERR_peek_last_error();
if (err_code != 0 && ...

https://codereview.webrtc.org/2167363002/diff/1/webrtc/base/sslstreamadapter.h
File webrtc/base/sslstreamadapter.h (right):

https://codereview.webrtc.org/2167363002/diff/1/webrtc/base/sslstreamadapter....
webrtc/base/sslstreamadapter.h:79: // Used to back UMA histogram value.
=> "Used to send back UMA histogram values." or something like that.

[Zhi Huang, 2016-07-25 17:24:29]

https://codereview.webrtc.org/2167363002/diff/1/webrtc/base/opensslstreamadap...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/2167363002/diff/1/webrtc/base/opensslstreamadap...
webrtc/base/opensslstreamadapter.cc:859: if ((err_code = ERR_peek_last_error())
!= 0 &&
On 2016/07/22 21:35:26, honghaiz3 wrote:
> you could do
> int err_code = ERR_peek_last_error();
> if (err_code != 0 && ...

Done.

https://codereview.webrtc.org/2167363002/diff/1/webrtc/base/sslstreamadapter.h
File webrtc/base/sslstreamadapter.h (right):

https://codereview.webrtc.org/2167363002/diff/1/webrtc/base/sslstreamadapter....
webrtc/base/sslstreamadapter.h:79: // Used to back UMA histogram value.
On 2016/07/22 21:35:26, honghaiz3 wrote:
> => "Used to send back UMA histogram values." or something like that. 

Done.

https://codereview.webrtc.org/2167363002/diff/1/webrtc/base/sslstreamadapter....
webrtc/base/sslstreamadapter.h:80: enum class SSLHandshakeError {
ERR_INCOMPATIBLE_CIPHERSUITE, ERR_OTHER };
On 2016/07/22 01:25:14, skvlad wrote:
> Would it be better to make ERR_OTHER the first value of the enum?
> The UMA methods take a maximum value but I'm not sure if they can handle
> reordering of enum values correctly. E.g. if subsequently we add another value
> ERR_NEW before ERR_OTHER - it would be hard to interpret if a value of "1"
means
> ERR_OTHER (for older versions) or ERR_NEW (for newer ones).
> 
> To maximize forward compatibility, I'd rather define it this way:
> enum class SSLHandshakeError { UNKNOWN, INCOMPATIBLE_CIPHERSUITE, MAX_VALUE }
> and use MAX_VALUE in calls to UMA.
> 
> Furthermore, you don't need prefixes (ERR_...) for enum class types as they
are
> already in their own namespace unlike regular enums.

Yes, this is better!

[skvlad, 2016-07-27 00:29:19]

lgtm

https://codereview.webrtc.org/2167363002/diff/60001/webrtc/base/opensslstream...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/2167363002/diff/60001/webrtc/base/opensslstream...
webrtc/base/opensslstreamadapter.cc:862:
SignalSSLHandshakeError(ssl_handshake_err);
I wonder if it could be better to directly log the ERR_GET_REASON values instead
of translating it into our own enums? That way we won't need to extend it to
other types of errors; on the other hand we'll need OpenSSL headers to interpret
this data correctly.

[honghaiz3, 2016-07-27 17:11:48]

https://codereview.webrtc.org/2167363002/diff/60001/webrtc/base/opensslstream...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/2167363002/diff/60001/webrtc/base/opensslstream...
webrtc/base/opensslstreamadapter.cc:862:
SignalSSLHandshakeError(ssl_handshake_err);
On 2016/07/27 00:29:18, skvlad wrote:
> I wonder if it could be better to directly log the ERR_GET_REASON values
instead
> of translating it into our own enums? That way we won't need to extend it to
> other types of errors; on the other hand we'll need OpenSSL headers to
interpret
> this data correctly.

Are we potentially interested in knowing other types of SSL errors?
If yes, we may want to use the return value from ERR_GET_REASON. The range of
that is pretty big though, up to 2^12.

[Zhi Huang, 2016-07-27 20:21:47]

Patchset #5 (id:80001) has been deleted

[Zhi Huang, 2016-07-27 20:36:21]

Patchset #5 (id:100001) has been deleted

[Zhi Huang, 2016-07-27 21:04:14]

https://codereview.webrtc.org/2167363002/diff/60001/webrtc/base/opensslstream...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/2167363002/diff/60001/webrtc/base/opensslstream...
webrtc/base/opensslstreamadapter.cc:862:
SignalSSLHandshakeError(ssl_handshake_err);
On 2016/07/27 17:11:48, honghaiz3 wrote:
> On 2016/07/27 00:29:18, skvlad wrote:
> > I wonder if it could be better to directly log the ERR_GET_REASON values
> instead
> > of translating it into our own enums? That way we won't need to extend it to
> > other types of errors; on the other hand we'll need OpenSSL headers to
> interpret
> > this data correctly.
> 
> Are we potentially interested in knowing other types of SSL errors?
> If yes, we may want to use the return value from ERR_GET_REASON. The range of
> that is pretty big though, up to 2^12. 

I will remove the new defined enum and use the error code directly from
ERR_GET_REASON(). Since in this issue we are more interested in
incompatible-ciphersuite, I will just log that for now. It won't be too hard to
extend the histogram if we become interested in other types.

https://codereview.webrtc.org/2167363002/diff/120001/webrtc/base/opensslstrea...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/2167363002/diff/120001/webrtc/base/opensslstrea...
webrtc/base/opensslstreamadapter.cc:858: int err_reason = 0;
I can leave a comment, like "Use 0 to represent unknown error.", here before
submitting.

[honghaiz3, 2016-07-29 15:49:54]

https://codereview.webrtc.org/2167363002/diff/120001/webrtc/base/opensslstrea...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/2167363002/diff/120001/webrtc/base/opensslstrea...
webrtc/base/opensslstreamadapter.cc:862: SignalSSLHandshakeError(err_reason);
Should this just be 
int err_reason = ERR_GET_REASON(err_code);
SignalSSLHandshakeError(err_reason);  
?

[honghaiz3, 2016-08-01 17:19:19]

lgtm

https://codereview.webrtc.org/2167363002/diff/120001/webrtc/base/opensslstrea...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/2167363002/diff/120001/webrtc/base/opensslstrea...
webrtc/base/opensslstreamadapter.cc:862: SignalSSLHandshakeError(err_reason);
On 2016/07/29 15:49:54, honghaiz3 wrote:
> Should this just be 
> int err_reason = ERR_GET_REASON(err_code);
> SignalSSLHandshakeError(err_reason);  
> ?
If you only want to log the error SSL_R_NO_SHARED_CIPHER, that is fine.

[Zhi Huang, 2016-08-02 00:17:03]

zhihuang@webrtc.org changed reviewers:
+ pthatcher@webrtc.org
- honghaiz@webrtc.org, skvlad@webrtc.org

[Zhi Huang, 2016-08-02 00:17:04]

Hi Peter,
Please take a look. I got two lgtms.

[pthatcher1, 2016-08-03 22:40:46]

https://codereview.webrtc.org/2167363002/diff/120001/webrtc/base/sslstreamada...
File webrtc/base/sslstreamadapter.h (right):

https://codereview.webrtc.org/2167363002/diff/120001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:209: sigslot::signal1<int>
SignalSSLHandshakeError;
We should document what values will be in here.  It looks like we only have two,
and one is for "unknown".  Perhaps an enum would make sense.

[Zhi Huang, 2016-08-22 19:41:14]

Please take another look, thanks.

[pthatcher1, 2016-08-23 00:15:18]

lgtm

[Zhi Huang, 2016-08-23 18:35:57]

The CQ bit was checked by zhihuang@webrtc.org

[Zhi Huang, 2016-08-23 18:35:57]

The patchset sent to the CQ was uploaded after l-g-t-m from skvlad@webrtc.org,
honghaiz@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2167363002/#ps140001 (title:
"Use enum for handshake error code.")

[commit-bot: I haz the power, 2016-08-23 18:36:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-08-23 20:36:27]

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

[commit-bot: I haz the power, 2016-08-23 20:36:28]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[Zhi Huang, 2016-08-23 20:37:29]

The CQ bit was checked by zhihuang@webrtc.org

[commit-bot: I haz the power, 2016-08-23 20:37:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-08-23 22:38:07]

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

[commit-bot: I haz the power, 2016-08-23 22:38:09]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[Zhi Huang, 2016-08-25 20:44:04]

The CQ bit was checked by zhihuang@webrtc.org

[commit-bot: I haz the power, 2016-08-25 20:44:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-08-25 22:44:46]

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

[commit-bot: I haz the power, 2016-08-25 22:44:47]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[Zhi Huang, 2016-08-26 18:16:55]

The CQ bit was checked by zhihuang@webrtc.org

[commit-bot: I haz the power, 2016-08-26 18:17:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-08-26 18:25:12]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-08-26 18:25:14]

Description was changed from

==========
Log how often DTLS negotiation failed because of incompatible ciphersuites.

Log the DTLS handshake error code in OpenSSLStreamAdapter.
Forward the error code to WebRTCSession with the Signals.
This part is only for the WebRTC native code.
To make it work, need another CL for Chromium.

BUG=webrtc:5959
==========

to

==========
Log how often DTLS negotiation failed because of incompatible ciphersuites.

Log the DTLS handshake error code in OpenSSLStreamAdapter.
Forward the error code to WebRTCSession with the Signals.
This part is only for the WebRTC native code.
To make it work, need another CL for Chromium.

BUG=webrtc:5959

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

[commit-bot: I haz the power, 2016-08-26 18:25:15]

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