No longer preconnect to unsafe ports.

net/http/http_stream_factory_impl_job.cc

Index: net/http/http_stream_factory_impl_job.cc
===================================================================
--- net/http/http_stream_factory_impl_job.cc	(revision 113620)
+++ net/http/http_stream_factory_impl_job.cc	(working copy)
@@ -485,6 +485,10 @@
     State state = next_state_;
     next_state_ = STATE_NONE;
     switch (state) {
+      case STATE_START:
+        DCHECK_EQ(OK, rv);
+        rv = DoStart();
+        break;
       case STATE_RESOLVE_PROXY:
         DCHECK_EQ(OK, rv);
         rv = DoResolveProxy();
@@ -534,21 +538,29 @@
 
 int HttpStreamFactoryImpl::Job::StartInternal() {
   CHECK_EQ(STATE_NONE, next_state_);
-
-  origin_ = HostPortPair(request_info_.url.HostNoBrackets(),
-                         request_info_.url.EffectiveIntPort());
-  origin_url_ = HttpStreamFactory::ApplyHostMappingRules(
-      request_info_.url, &origin_);
-
+  next_state_ = STATE_START;
   net_log_.BeginEvent(NetLog::TYPE_HTTP_STREAM_JOB,
                       HttpStreamJobParameters::Create(request_info_.url,
                                                       origin_url_));

[wtc, 2012/01/14 01:34:46]

In the original code, we have already initialized origin_url_
at this point.  In the new code, origin_url_ is initialized
in DoStart().  It seems that we should also move this
net_log_.BeginEvent statement (which uses origin_url_) to
DoStart().  Do you agree?

[mmenke, 2012/01/14 02:19:21]

Definitely a bug.  Thanks for spotting this.

-  next_state_ = STATE_RESOLVE_PROXY;
   int rv = RunLoop(OK);
   DCHECK_EQ(ERR_IO_PENDING, rv);
   return rv;
 }
 
+int HttpStreamFactoryImpl::Job::DoStart() {

[willchan no longer on Chromium, 2011/12/12 18:24:16]

Almost all the rest of our states have DoFoo() and DoFooComplete(). Can we just
put this code into StartInternal()?

[mmenke, 2011/12/12 18:43:21]

Currently it's expected to always return asynchronously.  Changing that would
require modifying all call sites.

Alternatively, we could separate off the failure code in RunLoop off to another
function instead, and call that directly, or make DoResolveProxy take in an
error code as input instead, which seems a little weird.

I'm not a huge fan of either of those options, which is why I added a new state.

I think the best of those options is the DoResolveProxy() option, but passing it
an error unrelated to resolving the proxy seems a little weird.  If you prefer
one of the alternative approaches, just tell me which.

[willchan no longer on Chromium, 2011/12/12 19:04:58]

Oh I see. I think I'd like to modify all call sites. There appear to only be 2
of them. Does that seem reasonable?

[mmenke, 2011/12/12 19:10:03]

Sure, will do.

[mmenke, 2011/12/13 16:35:00]

Making this able to fail synchronously is enough of a pain that I'm just going
to put fixing this on hold for a while.  Because of the whole |alternate_job|
thing, I'll have to modify RequestStream's job/request creation function a fair
bit to prevent re-entrancy, and maybe change a couple class invariants.

Jobs require requests to be created, Jobs are only removed from requests on
failure or when one succeeds, which may result in a callback to the delegate if
there are no other jobs, so synchronous failing requires a lot of funkiness or a
significant modification of the request/job interface.

[mmenke, 2011/12/13 16:44:32]

Or I could just add an OnStartComplete function that just sets the the next
state to NONE or RESOLVE_PROXY, based on the error code.

+  // Don't connect to restricted ports.
+  int port = request_info_.url.EffectiveIntPort();
+  if (!IsPortAllowedByDefault(port) && !IsPortAllowedByOverride(port))
+    return ERR_UNSAFE_PORT;

[wtc, 2012/01/14 01:34:46]

Before DoResolveProxyComplete() returns an error code, it
calls dependent_job_->Resume(this) if there is a dependent
job.  See lines 596-600 below.

I think we should do the same here.

[mmenke, 2012/01/14 02:19:21]

I agree.  There's no fall through to automatically call it on error.

+
+  origin_ = HostPortPair(request_info_.url.HostNoBrackets(), port);
+  origin_url_ = HttpStreamFactory::ApplyHostMappingRules(
+      request_info_.url, &origin_);
+
+  next_state_ = STATE_RESOLVE_PROXY;
+  return OK;
+}
+
 int HttpStreamFactoryImpl::Job::DoResolveProxy() {
   DCHECK(!pac_request_);