New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 372533 link

Starred by 1 user
Status: Fixed
Owner:
mmenke@chromium.org
Closed: May 2014
Cc:
mef@chromium.org
rch@chromium.org
█ willchan@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 372528



Sign in to add a comment

Get rid of NPN configuration globals.

Project Member Reported by mmenke@chromium.org, May 12 2014 
Currently net uses a number of globals, all in HttpStreamFactory, for NPN configuration.  These make for a rather ugly API, with unexpected effects (Change configuration while running chrome, and old connections may be grandfathered in, for instance).

One side effect of this is that other projects making use of the network stack do not get SPDY support unless they specifically enable it.

We should instead make these all part of the HttpNetworkSession, and give them reasonable defaults.  I consider this part of the refactoring needed to make Cronet have a reasonable API.

Comment 1 by mmenke@chromium.org, May 12 2014

Blocking: chromium:372528

Comment 2 by rch@chromium.org, May 12 2014

Cc: rch@chromium.org
YES PLEASE! I took at look at fixing this once, and it ended up harder than I expected. The issue is that currently there is an extensions API which enables and disables SPDY.  I look forward to the death of this mess!

Comment 3 by mmenke@chromium.org, May 12 2014 

Thanks for the pointer - Looking at the code, it looks like the benchmarking API is not documented as a public API, so is presumably only used by chrome's own benchmarking extension.  I'd rather just remove the function and update the extension - the fact is that there are no tests for what happens when SPDY is disabled at runtime, and I don't think they're worth adding.  We have command line flags, if someone really wants to experiment with it.

Comment 4 by rch@chromium.org, May 12 2014 

Works for me. At the time, I didn't feel comfortable breaking the API, but
I was a net-stack newbie. Go for it :>

Comment 5 by mmenke@chromium.org, May 13 2014 

Looks like WebPageTest uses the API.  Since it looks to be part of Chrome's test infrastructure, probably have to get rid of it there, first.

Comment 6 by mmenke@chromium.org, May 13 2014 

And...I was wrong about WebPageReplay.  It just uses code that was copied from the benchmark extension, but uses command line flags to enable/disable SPDY, like it should.
Project Member

Comment 7 by bugdroid1@chromium.org, May 15 2014 

------------------------------------------------------------------
r270811 | mmenke@chromium.org | 2014-05-15T20:54:46.015892Z

Changed paths:
   D http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs/examples/extensions/benchmark/jquery/jquery.flot.min.js?r1=270811&r2=270810&pathrev=270811
   D http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs/examples/extensions/benchmark/options.html?r1=270811&r2=270810&pathrev=270811
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/renderer/net_benchmarking_extension.cc?r1=270811&r2=270810&pathrev=270811
   D http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs/examples/extensions/benchmark/jquery/jquery-1.8.2.min.js?r1=270811&r2=270810&pathrev=270811
   D http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs/examples/extensions/benchmark/jst/jsevalcontext.js?r1=270811&r2=270810&pathrev=270811
   D http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs/examples/extensions/benchmark/options.js?r1=270811&r2=270810&pathrev=270811
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/benchmarking_messages.h?r1=270811&r2=270810&pathrev=270811
   D http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs/examples/extensions/benchmark/README.txt?r1=270811&r2=270810&pathrev=270811
   D http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs/examples/extensions/benchmark/util/table2CSV.js?r1=270811&r2=270810&pathrev=270811
   D http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs/examples/extensions/benchmark/manifest.json?r1=270811&r2=270810&pathrev=270811
   D http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs/examples/extensions/benchmark/script.js?r1=270811&r2=270810&pathrev=270811
   D http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs/examples/extensions/benchmark/jst/jstemplate_test.js?r1=270811&r2=270810&pathrev=270811
   D http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs/examples/extensions/benchmark/jquery/jquery.flot.dashes.js?r1=270811&r2=270810&pathrev=270811
   D http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs/examples/extensions/benchmark/background.html?r1=270811&r2=270810&pathrev=270811
   D http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs/examples/extensions/benchmark/jst/jstemplate.js?r1=270811&r2=270810&pathrev=270811
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/chrome_net_benchmarking_message_filter.cc?r1=270811&r2=270810&pathrev=270811
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/chrome_net_benchmarking_message_filter.h?r1=270811&r2=270810&pathrev=270811
   D http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs/examples/extensions/benchmark/util/sorttable.js?r1=270811&r2=270810&pathrev=270811
   D http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs/examples/extensions/benchmark/jst/util.js?r1=270811&r2=270810&pathrev=270811
   D http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs/examples/extensions/benchmark/background.js?r1=270811&r2=270810&pathrev=270811
   D http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs/examples/extensions/benchmark/stopwatch.jpg?r1=270811&r2=270810&pathrev=270811

Delete benchmarking extension, remove API's ability to enable/disable SPDY.

Delete the "Example" benchmarking extension, as no one's using it, and
we have better tools, like WebPageReplay.  This keeps the benchmarking
API, is it's used by telemetry and WebPageReplay, but does remove the
function to enable/disable SPDY support at runtime.

This is one step towards getting rid of the SPDY configuration globals.
There are no tests for enabling/disabling SPDY when a URLRequestContext has
in-flight requests, and supporting it really doesn't get us anything.

BUG= 372533 

Review URL: https://codereview.chromium.org/280383008
-----------------------------------------------------------------
Project Member

Comment 8 by bugdroid1@chromium.org, May 15 2014 

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

commit 48291f6e8a275a21cf5a6e831915299ec52d2d95
Author: mmenke@chromium.org <mmenke@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Thu May 15 20:54:46 2014

Delete benchmarking extension, remove API's ability to enable/disable SPDY.

Delete the "Example" benchmarking extension, as no one's using it, and
we have better tools, like WebPageReplay.  This keeps the benchmarking
API, is it's used by telemetry and WebPageReplay, but does remove the
function to enable/disable SPDY support at runtime.

This is one step towards getting rid of the SPDY configuration globals.
There are no tests for enabling/disabling SPDY when a URLRequestContext has
in-flight requests, and supporting it really doesn't get us anything.

BUG= 372533 

Review URL: https://codereview.chromium.org/280383008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@270811 0039d316-1c4b-4281-b951-d872f2087c98
Project Member

Comment 9 by bugdroid1@chromium.org, May 24 2014 

------------------------------------------------------------------
r272698 | mmenke@chromium.org | 2014-05-24T03:37:23.291957Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/quic/quic_network_transaction_unittest.cc?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_stream_factory_impl_unittest.cc?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_stream_factory_impl.cc?r1=272698&r2=272697&pathrev=272698
   A http://src.chromium.org/viewvc/chrome/trunk/src/net/socket/next_proto.cc?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/socket/next_proto.h?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/policy/url_blacklist_manager_unittest.cc?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/jingle/glue/proxy_resolving_client_socket.cc?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/net/preconnect.cc?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_stream_factory.cc?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/spdy/spdy_test_util_common.cc?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/components/cronet/android/url_request_context_peer.cc?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_stream_factory.h?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/spdy/spdy_test_util_common.h?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/io_thread.cc?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/components/domain_reliability/uploader_unittest.cc?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/net.gypi?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/webui/net_internals/net_internals_ui.cc?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_network_session.cc?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/io_thread.h?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/android_webview/browser/net/aw_url_request_context_getter.cc?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/websockets/websocket_stream.cc?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_network_transaction_unittest.cc?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_stream_factory_impl_job.cc?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/spdy/spdy_network_transaction_unittest.cc?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_network_transaction.cc?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_network_session.h?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_stream_factory_impl_job.h?r1=272698&r2=272697&pathrev=272698
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/test/net_test_suite.cc?r1=272698&r2=272697&pathrev=272698

Remove HttpStreamFactory's NPN/SPDY globals, except for spdy_enabled.

Instead, each HttpNetworkSession is given its own immutable copies on
construction.  Other than spdy_enabled, none of the globals were
changed before this CL, anyways.

Also, setting spdy_enabled back to true after setting it to false no
longer clears the NPN list.

spdy_enabled is still a global because group policy can set it to
false at runtime.

BUG= 372533 
R=joaodasilva@chromium.org, rch@chromium.org, sergeyu@chromium.org, sgurun@chromium.org, sky@chromium.org, ttuttle@chromium.org

Review URL: https://codereview.chromium.org/284423002
-----------------------------------------------------------------

Comment 10 by mmenke@chromium.org, May 24 2014

Status: Fixed

Comment 11 by laforge@google.com, Mar 4 2015

Labels: -Cr-Internals-Network-SPDY Cr-Internals-Network-HTTP2
Migrate from Cr-Internals-Network-SPDY to Cr-Internals-Network-HTTP2
►

Sign in to add a comment