No longer preconnect to unsafe ports.

No longer preconnect to unsafe ports.  As UrlRequests were
never allowed to use these ports anyways, this wasn't a
security issue, but just doesn't seem like a good idea.

BUG=93326
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114261

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

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:550: int
HttpStreamFactoryImpl::Job::DoStart() {
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]

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:550: int
HttpStreamFactoryImpl::Job::DoStart() {
On 2011/12/12 18:24:16, willchan wrote:
> Almost all the rest of our states have DoFoo() and DoFooComplete(). Can we
just
> put this code into StartInternal()?

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:55]

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:550: int
HttpStreamFactoryImpl::Job::DoStart() {
On 2011/12/12 18:43:21, Matt Menke wrote:
> On 2011/12/12 18:24:16, willchan wrote:
> > Almost all the rest of our states have DoFoo() and DoFooComplete(). Can we
> just
> > put this code into StartInternal()?
> 
> Currently it's expected to always return asynchronously.  Changing that would
> require modifying all call sites.

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?

> 
> 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.

[mmenke, 2011-12-12 19:10:02]

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:550: int
HttpStreamFactoryImpl::Job::DoStart() {
On 2011/12/12 19:04:58, willchan wrote:
> On 2011/12/12 18:43:21, Matt Menke wrote:
> > On 2011/12/12 18:24:16, willchan wrote:
> > > Almost all the rest of our states have DoFoo() and DoFooComplete(). Can we
> > just
> > > put this code into StartInternal()?
> > 
> > Currently it's expected to always return asynchronously.  Changing that
would
> > require modifying all call sites.
> 
> 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?

Sure, will do.

[mmenke, 2011-12-13 16:34:59]

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:550: int
HttpStreamFactoryImpl::Job::DoStart() {
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]

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:550: int
HttpStreamFactoryImpl::Job::DoStart() {
On 2011/12/13 16:35:00, Matt Menke wrote:
> 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.

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.

[willchan no longer on Chromium, 2011-12-13 16:59:50]

Sorry if my suggestion was too much of a yak shave. I'm happy with your
previous solution if you deem it to be a better use of your time.

On Tue, Dec 13, 2011 at 8:44 AM, <mmenke@chromium.org> wrote:

>
> http://codereview.chromium.**org/8898008/diff/14005/net/**
>
http/http_stream_factory_impl_**job.cc<http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factory_impl_job.cc>
> File net/http/http_stream_factory_**impl_job.cc (right):
>
> http://codereview.chromium.**org/8898008/diff/14005/net/**
>
http/http_stream_factory_impl_**job.cc#newcode550<http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factory_impl_job.cc#newcode550>
> net/http/http_stream_factory_**impl_job.cc:550: int
> HttpStreamFactoryImpl::Job::**DoStart() {
> On 2011/12/13 16:35:00, Matt Menke wrote:
>
>> 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.
>>
>
> 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.
>
>
http://codereview.chromium.**org/8898008/<http://codereview.chromium.org/8898...
>

[mmenke, 2011-12-13 17:04:00]

On 2011/12/13 16:59:50, willchan wrote:
> Sorry if my suggestion was too much of a yak shave. I'm happy with your
> previous solution if you deem it to be a better use of your time.

Yea, I think I'd like to just land my original solution.

[willchan no longer on Chromium, 2011-12-13 18:21:14]

OK, lgtm

Sent from my iNexus.
On Dec 13, 2011 9:04 AM, <mmenke@chromium.org> wrote:

> On 2011/12/13 16:59:50, willchan wrote:
>
>> Sorry if my suggestion was too much of a yak shave. I'm happy with your
>> previous solution if you deem it to be a better use of your time.
>>
>
> Yea, I think I'd like to just land my original solution.
>
>
http://codereview.chromium.**org/8898008/<http://codereview.chromium.org/8898...
>

[mmenke, 2011-12-13 18:24:06]

Great, thanks so much for the review.  May be reasonable to add a sync failure
path in a future refactoring.

[commit-bot: I haz the power, 2011-12-13 18:24:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/8898008/14005

[commit-bot: I haz the power, 2011-12-13 20:34:25]

Change committed as 114261

[wtc, 2012-01-14 01:34:45]

mmenke: while investigating bug 109876, I came across this
CL of yours.  I have some review comments below.

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (left):

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:545: origin_url_));

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?

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:554: return ERR_UNSAFE_PORT;

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.

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:598: dependent_job_->Resume(this);

Should we set dependent_job_ to NULL after calling
dependent_job_->Resume(this)?  We do this elsewhere
in this file.  (Search for "dependent_job_->Resume".)

By the way, I find dependent_job_ very confusing.  (It
comes from the original code by willchan.)  I suggest
renaming it waiting_job_ (the job that is waiting for |this|).

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

Thanks so much for catching these.  I have a CL that should fix the issues, but
I want to look into writing a unit test.

It's not exactly a common or big issue, but it could theoretically lead to a
socket request hanging indefinitely, and is sufficiently obscure that
regressions could easily occur and not be noticed.

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (left):

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:545: origin_url_));
On 2012/01/14 01:34:46, wtc wrote:
> 
> 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?

Definitely a bug.  Thanks for spotting this.

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:554: return ERR_UNSAFE_PORT;
On 2012/01/14 01:34:46, wtc wrote:
> 
> 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.

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

http://codereview.chromium.org/8898008/diff/14005/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:598: dependent_job_->Resume(this);
On 2012/01/14 01:34:46, wtc wrote:
> 
> Should we set dependent_job_ to NULL after calling
> dependent_job_->Resume(this)?  We do this elsewhere
> in this file.  (Search for "dependent_job_->Resume".)
> 
> By the way, I find dependent_job_ very confusing.  (It
> comes from the original code by willchan.)  I suggest
> renaming it waiting_job_ (the job that is waiting for |this|).

It shouldn't really be needed, since if we fail, the Request will end up
deleting |this|.  Since the other job can't fail (Or succeed) synchronously, we
also don't have to worry about what it will do, once started.

That having been said, it does seem like something we should do, just to protect
against future changes in behavior.

I've also renamed the job and updated the description in the header.