Delete TLS version fallback code in net/http.

Delete TLS version fallback code in net/http.

This removes the logic in HttpNetworkTransaction to drive the TLS version
fallback. It's gone now.

Note this does come with a behavior change. TLS 1.2 intolerant servers will now
fail with their true error code rather than
ERR_SSL_FALLBACK_BEYOND_MINIMUM_VERSION. That error code was intended for
diagnositics. With the fallback having been gone for several releases (by the
time this hits stable), we won't need it anymore.

Of course, this will all come back for TLS 1.3, but let's implement it in
SSLConnectJob next time. Avoid a bit of munging across layers.

BUG=621780
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/5a31215ed09f55697b00a9bdb1b76f5c4e3b66f6
Cr-Commit-Position: refs/heads/master@{#402312}

[davidben, 2016-06-24 17:54:52]

davidben@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2016-06-24 18:51:25]

rsleevi@chromium.org changed reviewers:
+ eroman@chromium.org

[Ryan Sleevi, 2016-06-24 18:51:26]

LGTM but please run one thing (below) past eroman

https://codereview.chromium.org/2099673003/diff/20001/net/log/net_log_event_t...
File net/log/net_log_event_type_list.h (left):

https://codereview.chromium.org/2099673003/diff/20001/net/log/net_log_event_t...
net/log/net_log_event_type_list.h:545: 
Do we need to preserve these IDs in any meaningful way? Just confirming "no" -
there's no need to keep legacy events

[eroman, 2016-06-24 18:56:07]

https://codereview.chromium.org/2099673003/diff/20001/net/log/net_log_event_t...
File net/log/net_log_event_type_list.h (left):

https://codereview.chromium.org/2099673003/diff/20001/net/log/net_log_event_t...
net/log/net_log_event_type_list.h:545: 
On 2016/06/24 18:51:26, Ryan Sleevi wrote:
> Do we need to preserve these IDs in any meaningful way? Just confirming "no" -
> there's no need to keep legacy events

No, there is no need to preserve IDs. Legacy log files will still load
correctly.

The log file format includes the symbolic names.
Both for these event types, but also for things like net error codes.

LGTM

[davidben, 2016-06-24 18:56:18]

Description was changed from

==========
Delete TLS version fallback code in net/http.

This removes the logic in HttpNetworkTransaction to drive the TLS version
fallback. It's gone now.

Note this does come with a behavior change. TLS 1.2 intolerant servers will now
fail with their true error code rather than
ERR_SSL_FALLBACK_BEYOND_MINIMUM_VERSION. That error code was intended for
diagnositics. With the fallback having been gone for several releases (by the
time this hits stable), we won't need it anymore.

Of course, this will all come back for TLS 1.3, but let's implement it in
SSLConnectJob next time. Avoid a bit of munging across layers.

BUG=621780
==========

to

==========
Delete TLS version fallback code in net/http.

This removes the logic in HttpNetworkTransaction to drive the TLS version
fallback. It's gone now.

Note this does come with a behavior change. TLS 1.2 intolerant servers will now
fail with their true error code rather than
ERR_SSL_FALLBACK_BEYOND_MINIMUM_VERSION. That error code was intended for
diagnositics. With the fallback having been gone for several releases (by the
time this hits stable), we won't need it anymore.

Of course, this will all come back for TLS 1.3, but let's implement it in
SSLConnectJob next time. Avoid a bit of munging across layers.

BUG=621780
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[davidben, 2016-06-24 18:57:08]

eroman: Turns out I need to update log_view_painter.js anyway. Mind taking a
look at that too?

[eroman, 2016-06-24 19:19:44]

https://codereview.chromium.org/2099673003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/net_internals/log_view_painter.js (right):

https://codereview.chromium.org/2099673003/diff/40001/chrome/browser/resource...
chrome/browser/resources/net_internals/log_view_painter.js:407: function
SSLVersionNumberToName(version) {
If you are going to delete the specific formatter code for fallback you should
delete this too.

That said, I would suggest not making changes to the file and instead adding a
TODO by the handler for SSL_VERSION_FALLBACK, indicating that it is deprecated
and a milestone for deleting the code -- say M56.

[davidben, 2016-06-24 19:26:18]

https://codereview.chromium.org/2099673003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/net_internals/log_view_painter.js (right):

https://codereview.chromium.org/2099673003/diff/40001/chrome/browser/resource...
chrome/browser/resources/net_internals/log_view_painter.js:407: function
SSLVersionNumberToName(version) {
On 2016/06/24 19:19:44, eroman wrote:
> If you are going to delete the specific formatter code for fallback you should
> delete this too.
> 
> That said, I would suggest not making changes to the file and instead adding a
> TODO by the handler for SSL_VERSION_FALLBACK, indicating that it is deprecated
> and a milestone for deleting the code -- say M56.

Done. I put it in the event list since it seems I need to keep that or the tests
get unhappy.

[eroman, 2016-06-24 19:42:00]

https://codereview.chromium.org/2099673003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/net_internals/log_view_painter.js (right):

https://codereview.chromium.org/2099673003/diff/40001/chrome/browser/resource...
chrome/browser/resources/net_internals/log_view_painter.js:407: function
SSLVersionNumberToName(version) {
On 2016/06/24 19:26:18, davidben wrote:
> On 2016/06/24 19:19:44, eroman wrote:
> > If you are going to delete the specific formatter code for fallback you
should
> > delete this too.
> > 
> > That said, I would suggest not making changes to the file and instead adding
a
> > TODO by the handler for SSL_VERSION_FALLBACK, indicating that it is
deprecated
> > and a milestone for deleting the code -- say M56.
> 
> Done. I put it in the event list since it seems I need to keep that or the
tests
> get unhappy.

Actually I was envisioning the deprecation solely in the JS, and you would still
delete everything from C++.

Concretely:
  (a) Go ahead and delete the event from the net_log_event_list.h

  (b) Add a TODO in log_view_painter.js indicating the functionality was removed
from logs in XXX and to remove the code for handling it in YYY.

  (c) Hack the unit-test for SSL_VERSION_FALLBACK painting so it passes, or just
remove it. I expect you can hack it by adding a single line (and accompanying
TODO):

function painterTestSSLVersionFallback() {

  // TODO(eroman): Hack bla bla bla. Remove bla bla bla.
  EventType.SSL_VERSION_FALLBACK = 96793;

  <test code as before>
}

The issue is these run as browser tests and use whatever the current constants
are (which came from current C++).

We are in the process of splitting this code and tests out of Chrome anyway so
this will necessarily be changing soon. I would be OK with the above hack, and
it can be cleaned up when the tests are transitioned to their new reality.

[davidben, 2016-06-27 19:27:44]

https://codereview.chromium.org/2099673003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/net_internals/log_view_painter.js (right):

https://codereview.chromium.org/2099673003/diff/40001/chrome/browser/resource...
chrome/browser/resources/net_internals/log_view_painter.js:407: function
SSLVersionNumberToName(version) {
From talking offline, it seems there were all kinds of screwiness around this
file where it didn't handle missing keys right (or relied on JavaScript's
behavior around undefined to do so). Would you still prefer I delete the event
from net_log_event_list.h or shall I leave the CL as it is?

[eroman, 2016-06-27 19:31:28]

https://codereview.chromium.org/2099673003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/net_internals/log_view_painter.js (right):

https://codereview.chromium.org/2099673003/diff/40001/chrome/browser/resource...
chrome/browser/resources/net_internals/log_view_painter.js:407: function
SSLVersionNumberToName(version) {
On 2016/06/27 19:27:44, davidben wrote:
> From talking offline, it seems there were all kinds of screwiness around this
> file where it didn't handle missing keys right (or relied on JavaScript's
> behavior around undefined to do so). Would you still prefer I delete the event
> from net_log_event_list.h or shall I leave the CL as it is?

Up to you, I don't feel too strongly about it.

[davidben, 2016-06-27 19:38:48]

https://codereview.chromium.org/2099673003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/net_internals/log_view_painter.js (right):

https://codereview.chromium.org/2099673003/diff/40001/chrome/browser/resource...
chrome/browser/resources/net_internals/log_view_painter.js:407: function
SSLVersionNumberToName(version) {
On 2016/06/27 19:31:28, eroman wrote:
> On 2016/06/27 19:27:44, davidben wrote:
> > From talking offline, it seems there were all kinds of screwiness around
this
> > file where it didn't handle missing keys right (or relied on JavaScript's
> > behavior around undefined to do so). Would you still prefer I delete the
event
> > from net_log_event_list.h or shall I leave the CL as it is?
> 
> Up to you, I don't feel too strongly about it.

Alright. I'll go land this as-is then and then assume you all will sort out the
painter later. :-)

Besides, perhaps I'll resurrect it for TLS 1.3 first anyway.

[davidben, 2016-06-27 19:39:01]

The CQ bit was checked by davidben@chromium.org

[davidben, 2016-06-27 19:39:02]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/2099673003/#ps60001
(title: "keep log entry")

[commit-bot: I haz the power, 2016-06-27 19:39:50]

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

[davidben, 2016-06-27 19:44:38]

The CQ bit was checked by davidben@chromium.org

[davidben, 2016-06-27 19:44:39]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/2099673003/#ps100001
(title: "Oops, got my branches confused.")

[commit-bot: I haz the power, 2016-06-27 19:45:35]

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

[commit-bot: I haz the power, 2016-06-27 22:12:01]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-27 22:13:43]

Description was changed from

==========
Delete TLS version fallback code in net/http.

This removes the logic in HttpNetworkTransaction to drive the TLS version
fallback. It's gone now.

Note this does come with a behavior change. TLS 1.2 intolerant servers will now
fail with their true error code rather than
ERR_SSL_FALLBACK_BEYOND_MINIMUM_VERSION. That error code was intended for
diagnositics. With the fallback having been gone for several releases (by the
time this hits stable), we won't need it anymore.

Of course, this will all come back for TLS 1.3, but let's implement it in
SSLConnectJob next time. Avoid a bit of munging across layers.

BUG=621780
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Delete TLS version fallback code in net/http.

This removes the logic in HttpNetworkTransaction to drive the TLS version
fallback. It's gone now.

Note this does come with a behavior change. TLS 1.2 intolerant servers will now
fail with their true error code rather than
ERR_SSL_FALLBACK_BEYOND_MINIMUM_VERSION. That error code was intended for
diagnositics. With the fallback having been gone for several releases (by the
time this hits stable), we won't need it anymore.

Of course, this will all come back for TLS 1.3, but let's implement it in
SSLConnectJob next time. Avoid a bit of munging across layers.

BUG=621780
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/5a31215ed09f55697b00a9bdb1b76f5c4e3b66f6
Cr-Commit-Position: refs/heads/master@{#402312}
==========

[commit-bot: I haz the power, 2016-06-27 22:13:44]

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