Clean up FtpTransactionFacory ownership.

Clean up FtpTransactionFacory ownership.

Previously, it was owned by the ProfileImplIOData, which is deleted
before the ProfileIOData, which owns the URLRequestContext.  Now it's
owned by the FtpProtocolHandler, which is owned by the
URLRequestJobFactory.  This also makes URLRequestContext creation a bit
simpler.

With this CL, isolated app URLRequestContexts now have their own
FtpTransactionFactories.  Since FTP connections aren't pooled, and the
FTP auth cache is owned by a higher layer, this effectively makes no
difference.

BUG=657135
Committed: https://crrev.com/cd4c75339e11e32ef36da7a856a19ca8ee3976f1
Cr-Commit-Position: refs/heads/master@{#426248}

[mmenke, 2016-10-18 21:11:11]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 21:11:44]

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

[mmenke, 2016-10-18 21:17:51]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 21:18:22]

Dry run: 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-10-18 22:26:02]

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

[commit-bot: I haz the power, 2016-10-18 22:26:03]

Dry run: Exceeded global retry quota

[mmenke, 2016-10-18 22:27:59]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 22:28:31]

Dry run: 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-10-18 22:38:20]

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

[commit-bot: I haz the power, 2016-10-18 22:38:21]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-10-19 15:26:55]

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

[mmenke, 2016-10-19 15:26:56]

While this isn't causing a crash, having the FtpTransactionFactory destroyed
before the ProfileIOData's URLRequestContext is just seems like a really bad
idea, particularly given that ProfileIOData indirectly owns from URLRequests it
has to shut down before destroyed the URLRequestContext.

[eroman, 2016-10-19 16:29:51]

lgtm

https://chromiumcodereview.appspot.com/2428143002/diff/20001/chrome/browser/p...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://chromiumcodereview.appspot.com/2428143002/diff/20001/chrome/browser/p...
chrome/browser/profiles/profile_impl_io_data.cc:710:
main_context->network_delegate(), main_context->host_resolver()));
I think this would be clearer if it accessed the io thread global host resolver
directly (even if less general)

https://chromiumcodereview.appspot.com/2428143002/diff/20001/chrome/browser/p...
File chrome/browser/profiles/profile_io_data.h (right):

https://chromiumcodereview.appspot.com/2428143002/diff/20001/chrome/browser/p...
chrome/browser/profiles/profile_io_data.h:364:
std::unique_ptr<net::URLRequestJobFactory> SetUpJobFactoryDefaults(
Can you add comments for this? In particular, would be nice to explain the role
of |host_resolver|, which is really just used for setting up ftp right now.

https://chromiumcodereview.appspot.com/2428143002/diff/20001/net/url_request/...
File net/url_request/ftp_protocol_handler.cc (right):

https://chromiumcodereview.appspot.com/2428143002/diff/20001/net/url_request/...
net/url_request/ftp_protocol_handler.cc:21: HostResolver* host_resolver) {
It would be good to have DCHECK that |host_resolver| is not null somewhere.

(Since we plumb it down a bunch of layers and may not surface as a crash until
FTP is used, which may be rarely)

Perhaps a DCHECK here, or in FtpNetworkLayer, or in FtpNetworkSession ?

https://chromiumcodereview.appspot.com/2428143002/diff/20001/net/url_request/...
net/url_request/ftp_protocol_handler.cc:22: return base::WrapUnique(
style: this line uses both WrapUnique and MakeUnique -- would read more
consistently it if only used one pattern for constructing the two classes.

https://chromiumcodereview.appspot.com/2428143002/diff/20001/net/url_request/...
File net/url_request/url_request_unittest.cc (right):

https://chromiumcodereview.appspot.com/2428143002/diff/20001/net/url_request/...
net/url_request/url_request_unittest.cc:9887: MockHostResolver host_resolver_;
Note this does mean that now the ftp protocol handler briefly outlives the host
resolver dependency it holds.

[mmenke, 2016-10-19 17:09:05]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[mmenke, 2016-10-19 17:09:35]

Comments were spot on, as usual.

https://chromiumcodereview.appspot.com/2428143002/diff/20001/chrome/browser/p...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://chromiumcodereview.appspot.com/2428143002/diff/20001/chrome/browser/p...
chrome/browser/profiles/profile_impl_io_data.cc:710:
main_context->network_delegate(), main_context->host_resolver()));
On 2016/10/19 16:29:51, eroman wrote:
> I think this would be clearer if it accessed the io thread global host
resolver
> directly (even if less general)

We don't have access to IOThread here - the two methods that use it get it from
ProfileParams, which is passed from the UI thread via ProfileImplIOData::Handle,
and then destroyed when we're done with it.  We could just grab it from
g_browser_process, I guess (g_browser_process does outlive the IOThread), but
then we should get rid of it as a member of ProfileParams and access it the same
way elsewhere in this file.

I've switched to just using context->host_resolver (i.e., the resolver we've
already set on the new context).  Seem reasonable?  Did the same for the
incognito code.

https://chromiumcodereview.appspot.com/2428143002/diff/20001/chrome/browser/p...
File chrome/browser/profiles/profile_io_data.h (right):

https://chromiumcodereview.appspot.com/2428143002/diff/20001/chrome/browser/p...
chrome/browser/profiles/profile_io_data.h:364:
std::unique_ptr<net::URLRequestJobFactory> SetUpJobFactoryDefaults(
On 2016/10/19 16:29:51, eroman wrote:
> Can you add comments for this? In particular, would be nice to explain the
role
> of |host_resolver|, which is really just used for setting up ftp right now.

Good idea, done.  Also added a little more general documentation (Though less
than the method really needs)

https://chromiumcodereview.appspot.com/2428143002/diff/20001/net/url_request/...
File net/url_request/ftp_protocol_handler.cc (right):

https://chromiumcodereview.appspot.com/2428143002/diff/20001/net/url_request/...
net/url_request/ftp_protocol_handler.cc:21: HostResolver* host_resolver) {
On 2016/10/19 16:29:51, eroman wrote:
> It would be good to have DCHECK that |host_resolver| is not null somewhere.
> 
> (Since we plumb it down a bunch of layers and may not surface as a crash until
> FTP is used, which may be rarely)
> 
> Perhaps a DCHECK here, or in FtpNetworkLayer, or in FtpNetworkSession ?

SGTM, done.  Added to FtpNetworkLayer (Wider coverage than in
FtpProtocolHandler.  Not sure FtpNetworkSession really should exist in the first
place).

https://chromiumcodereview.appspot.com/2428143002/diff/20001/net/url_request/...
net/url_request/ftp_protocol_handler.cc:22: return base::WrapUnique(
On 2016/10/19 16:29:51, eroman wrote:
> style: this line uses both WrapUnique and MakeUnique -- would read more
> consistently it if only used one pattern for constructing the two classes.

Switched to WrapUnique (Due to the private constructor, can't use MakeUnique to
make an FtpProtocolHandler, without make MakeUnique a friend, which I prefer to
avoid).

https://chromiumcodereview.appspot.com/2428143002/diff/20001/net/url_request/...
File net/url_request/url_request_unittest.cc (right):

https://chromiumcodereview.appspot.com/2428143002/diff/20001/net/url_request/...
net/url_request/url_request_unittest.cc:9887: MockHostResolver host_resolver_;
On 2016/10/19 16:29:51, eroman wrote:
> Note this does mean that now the ftp protocol handler briefly outlives the
host
> resolver dependency it holds.

Added a comment (This was the case with the FtpProtocolHandler before, so still
basically the same weirdness).

[commit-bot: I haz the power, 2016-10-19 17:09:49]

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

[eroman, 2016-10-19 17:15:15]

lgtm

https://chromiumcodereview.appspot.com/2428143002/diff/40001/chrome/browser/p...
File chrome/browser/profiles/profile_io_data.h (right):

https://chromiumcodereview.appspot.com/2428143002/diff/40001/chrome/browser/p...
chrome/browser/profiles/profile_io_data.h:365: // ProtocolHandlers to
|job_factory|, adds URLRequestIntercetors in front of
typo on interceptors

https://chromiumcodereview.appspot.com/2428143002/diff/40001/chrome/browser/p...
chrome/browser/profiles/profile_io_data.h:368: // |protocol_handler_interceptor
is configured to intercept URLRequests
missing close |

[mmenke, 2016-10-19 17:20:36]

Thanks!

https://chromiumcodereview.appspot.com/2428143002/diff/40001/chrome/browser/p...
File chrome/browser/profiles/profile_io_data.h (right):

https://chromiumcodereview.appspot.com/2428143002/diff/40001/chrome/browser/p...
chrome/browser/profiles/profile_io_data.h:365: // ProtocolHandlers to
|job_factory|, adds URLRequestIntercetors in front of
On 2016/10/19 17:15:15, eroman wrote:
> typo on interceptors

Done.

https://chromiumcodereview.appspot.com/2428143002/diff/40001/chrome/browser/p...
chrome/browser/profiles/profile_io_data.h:368: // |protocol_handler_interceptor
is configured to intercept URLRequests
On 2016/10/19 17:15:15, eroman wrote:
> missing close |

Done.

[mmenke, 2016-10-19 17:20:40]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2016-10-19 17:20:41]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/2428143002/#ps60001
(title: "--typos")

[commit-bot: I haz the power, 2016-10-19 17:21:07]

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-10-19 18:36:37]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-21 13:10:20]

Description was changed from

==========
Clean up FtpTransactionFacory ownership.

Previously, it was owned by the ProfileImplIOData, which is deleted
before the ProfileIOData, which owns the URLRequestContext.  Now it's
owned by the FtpProtocolHandler, which is owned by the
URLRequestJobFactory.  This also makes URLRequestContext creation a bit
simpler.

With this CL, isolated app URLRequestContexts now have their own
FtpTransactionFactories.  Since FTP connections aren't pooled, and the
FTP auth cache is owned by a higher layer, this effectively makes no
difference.

BUG=657135
==========

to

==========
Clean up FtpTransactionFacory ownership.

Previously, it was owned by the ProfileImplIOData, which is deleted
before the ProfileIOData, which owns the URLRequestContext.  Now it's
owned by the FtpProtocolHandler, which is owned by the
URLRequestJobFactory.  This also makes URLRequestContext creation a bit
simpler.

With this CL, isolated app URLRequestContexts now have their own
FtpTransactionFactories.  Since FTP connections aren't pooled, and the
FTP auth cache is owned by a higher layer, this effectively makes no
difference.

BUG=657135

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

[commit-bot: I haz the power, 2016-10-21 13:10:21]

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