Reland "Change ProxyResolver::GetProxyForURL() to take a unique_ptr<Request>* "

Reland "Change ProxyResolver::GetProxyForURL() to take a unique_ptr<Request>*"

Original issue's description:
> Change ProxyResolver::GetProxyForURL() to take a scoped_ptr<Request>* rather than a RequestHandle*
>
> * ProxyResolver::GetProxyForURL() fills a |scoped_pointer<Request>*|
>   rather than a |void*|
> * ProxyResolver::CancelRequest(void*) has been removed. Requests
>   are instead cancelled by resetting the scoped_ptr<Request>.
>
> This makes for less error prone code as cancellation of
> requests is automatic when the
> scoped_ptr<Request> goes out of scope.
> ProxyResolver::GetLoadState() is removed and replaced
> by Request::GetLoadState().
>
> Also made some renaming, as there were similar class
> named Job or Request. Now they are all Job and this new thing
> is Request.
>
> Referencing by address to object in vector was not wise in net/proxy/mojo_proxy_resolver_impl_unittest.cc which is now fixed by using scoped_ptrs in that vector.
>
> BUG=478934
>
> Committed: https://crrev.com/a750e126346aa42df1b0cbc2ae6a58abbe7a5069
> Cr-Commit-Position: refs/heads/master@{#377856}

BUG=478934
Committed: https://crrev.com/7e15726eca3718f99606e24c67c5794d9cb4a601
Cr-Commit-Position: refs/heads/master@{#426450}

[maksims (do not use this acc), 2016-09-01 10:51:30]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 10:51:43]

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

[maksims (do not use this acc), 2016-09-01 10:59:44]

Description was changed from

==========
ProxySolver::Request

BUG=
==========

to

==========
ProxySolver::Request

BUG=478934
==========

[maksims (do not use this acc), 2016-09-01 11:01:33]

Description was changed from

==========
ProxySolver::Request

BUG=478934
==========

to

==========
Revert of Revert of Change ProxyResolver::GetProxyForURL() to take a
scoped_ptr<Request>* rather than a RequestHandle*

BUG=478934
==========

[maksims (do not use this acc), 2016-09-01 11:01:49]

Description was changed from

==========
Revert of Revert of Change ProxyResolver::GetProxyForURL() to take a
scoped_ptr<Request>* rather than a RequestHandle*

BUG=478934
==========

to

==========
Revert of Revert of Change ProxyResolver::GetProxyForURL() to take a
unique_ptr<Request>* rather tha

BUG=478934
==========

[maksims (do not use this acc), 2016-09-01 11:02:08]

Description was changed from

==========
Revert of Revert of Change ProxyResolver::GetProxyForURL() to take a
unique_ptr<Request>* rather tha

BUG=478934
==========

to

==========
Revert of Revert of Change ProxyResolver::GetProxyForURL() to take a
unique_ptr<Request>* rather than RequestHandle*

BUG=478934
==========

[maksims (do not use this acc), 2016-09-01 11:11:29]

Description was changed from

==========
Revert of Revert of Change ProxyResolver::GetProxyForURL() to take a
unique_ptr<Request>* rather than RequestHandle*

BUG=478934
==========

to

==========
Reland Change ProxyResolver::GetProxyForURL() to take a unique_ptr<Request>* 

BUG=478934
==========

[maksims (do not use this acc), 2016-09-01 11:11:46]

Description was changed from

==========
Reland Change ProxyResolver::GetProxyForURL() to take a unique_ptr<Request>* 

BUG=478934
==========

to

==========
Reland "Change ProxyResolver::GetProxyForURL() to take a unique_ptr<Request>*"

BUG=478934
==========

[commit-bot: I haz the power, 2016-09-01 12:37:12]

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

[commit-bot: I haz the power, 2016-09-01 12:37:13]

Dry run: This issue passed the CQ dry run.

[maksims (do not use this acc), 2016-09-02 05:44:53]

Description was changed from

==========
Reland "Change ProxyResolver::GetProxyForURL() to take a unique_ptr<Request>*"

BUG=478934
==========

to

==========
Reland "Change ProxyResolver::GetProxyForURL() to take a unique_ptr<Request>*"

Original issue's description:
> Change ProxyResolver::GetProxyForURL() to take a scoped_ptr<Request>* rather
than a RequestHandle*
>
> * ProxyResolver::GetProxyForURL() fills a |scoped_pointer<Request>*|
>   rather than a |void*|
> * ProxyResolver::CancelRequest(void*) has been removed. Requests
>   are instead cancelled by resetting the scoped_ptr<Request>.
>
> This makes for less error prone code as cancellation of
> requests is automatic when the
> scoped_ptr<Request> goes out of scope.
> ProxyResolver::GetLoadState() is removed and replaced
> by Request::GetLoadState().
>
> Also made some renaming, as there were similar class
> named Job or Request. Now they are all Job and this new thing
> is Request.
>
> Referencing by address to object in vector was not wise in
net/proxy/mojo_proxy_resolver_impl_unittest.cc which is now fixed by using
scoped_ptrs in that vector.
>
> BUG=478934
>
> Committed: https://crrev.com/a750e126346aa42df1b0cbc2ae6a58abbe7a5069
> Cr-Commit-Position: refs/heads/master@{#377856}

BUG=478934
==========

[maksims (do not use this acc), 2016-09-02 05:45:11]

maksim.sisov@intel.com changed reviewers:
+ eroman@chromium.org

[maksims (do not use this acc), 2016-09-02 11:31:03]

Can we try to upstream this? I couldn't reproduce the issue. How dangerous is it
to do so?

[maksims (do not use this acc), 2016-09-05 06:58:20]

maksim.sisov@intel.com changed reviewers:
+ mmenke@chromium.org

[maksims (do not use this acc), 2016-09-05 06:58:45]

Can we try to upstream this? I couldn't reproduce the issue. How dangerous is it
to do so?

[mmenke, 2016-09-06 14:44:16]

On 2016/09/05 06:58:45, maksims wrote:
> Can we try to upstream this? I couldn't reproduce the issue. How dangerous is
it
> to do so?

It was the top crasher on Canary, quite possibly making Chrome unusable for
users with proxies.  It's not about security (Well, maybe that was an issue,
too), but basic usability.  Unfortunately, it's been so long since the revert
that we don't have any crash logs for this any more, so we don't have the
callstack of the crash.  I'll defer to eroman on where to go from here.

[eroman, 2016-09-06 20:56:45]

We absolutely need to understand why this failed the first time before
re-landing.

Or at a minimum have some theories to test.

We do have the callstacks for this, including high quality ones form syzygy asan
(see my comment:
https://bugs.chromium.org/p/chromium/issues/detail?id=478934#c10)

Both those bugs should be publicly visible.

Until someone has a chance to look through that and summarize the problem and
the fix (or why it is no longer applicable) I cannot approve this.

I haven't had a chance to investigate this myself; if I did I would have grabbed
this bug and done the re-land :)

[maksims (do not use this acc), 2016-09-20 04:12:50]

On 2016/09/06 20:56:45, eroman wrote:
> We absolutely need to understand why this failed the first time before
> re-landing.
> 
> Or at a minimum have some theories to test.
> 
> We do have the callstacks for this, including high quality ones form syzygy
asan
> (see my comment:
> https://bugs.chromium.org/p/chromium/issues/detail?id=478934#c10)
> 
> Both those bugs should be publicly visible.
> 
> Until someone has a chance to look through that and summarize the problem and
> the fix (or why it is no longer applicable) I cannot approve this.
> 
> I haven't had a chance to investigate this myself; if I did I would have
grabbed
> this bug and done the re-land :)

Sorry, I've been busy. Will take a deep look into this soon

[maksims (do not use this acc), 2016-09-28 08:38:16]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 08:38:31]

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

[maksims (do not use this acc), 2016-09-28 08:46:45]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-09-28 08:48:42]

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

[commit-bot: I haz the power, 2016-09-28 08:48:43]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[maksims (do not use this acc), 2016-09-28 12:23:02]

Patchset #1 (id:20001) has been deleted

[maksims (do not use this acc), 2016-09-28 12:59:48]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 12:59:57]

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-09-28 13:09:40]

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

[commit-bot: I haz the power, 2016-09-28 13:09:42]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[maksims (do not use this acc), 2016-09-29 06:41:34]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 06:41:46]

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

[maksims (do not use this acc), 2016-09-29 06:50:59]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 06:51:19]

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-09-29 07:10:39]

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

[commit-bot: I haz the power, 2016-09-29 07:10:40]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[maksims (do not use this acc), 2016-09-29 10:58:11]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 10:58:26]

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-09-29 11:52:28]

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

[commit-bot: I haz the power, 2016-09-29 11:52:29]

Dry run: This issue passed the CQ dry run.

[maksims (do not use this acc), 2016-09-30 08:11:44]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 08:11:57]

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-09-30 08:29:40]

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

[commit-bot: I haz the power, 2016-09-30 08:29:41]

Dry run: Exceeded global retry quota

[maksims (do not use this acc), 2016-10-05 09:28:47]

Patchset #5 (id:120001) has been deleted

[maksims (do not use this acc), 2016-10-14 11:00:39]

Patchset #4 (id:100001) has been deleted

[maksims (do not use this acc), 2016-10-14 11:00:48]

Patchset #3 (id:80001) has been deleted

[maksims (do not use this acc), 2016-10-14 11:00:56]

Patchset #2 (id:60001) has been deleted

[maksims (do not use this acc), 2016-10-14 11:10:55]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 11:11:05]

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-14 11:51:17]

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

[commit-bot: I haz the power, 2016-10-14 11:51:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[maksims (do not use this acc), 2016-10-17 06:02:56]

Patchset #2 (id:140001) has been deleted

[maksims (do not use this acc), 2016-10-17 06:12:57]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 06:13:12]

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-17 07:07:11]

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

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

Dry run: This issue passed the CQ dry run.

[maksims (do not use this acc), 2016-10-17 07:24:48]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[maksims (do not use this acc), 2016-10-17 07:24:49]

Patchset #2 (id:160001) has been deleted

[commit-bot: I haz the power, 2016-10-17 07:25:04]

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

[maksims (do not use this acc), 2016-10-17 07:25:06]

Patchset #1 (id:40001) has been deleted

[maksims (do not use this acc), 2016-10-17 07:28:46]

maksim.sisov@intel.com changed reviewers:
+ boliu@chromium.org

[maksims (do not use this acc), 2016-10-17 07:28:47]

boliu@chromium.org: Please review changes in 

content/

eroman@, please see the fix.

https://codereview.chromium.org/2299963002/diff/180001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_factory_mojo.cc (right):

https://codereview.chromium.org/2299963002/diff/180001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:207:
job_->CompleteRequest(ERR_PAC_SCRIPT_TERMINATED);
Ok, this one introduced a reentrancy problem.
When ProxyService::CancelPacRequest() is called, the pac request is destroyed
along with the request it has sent to ProxeResolverMojo. 
Then ~RequestImpl() calls CompleteRequest and triggers the original callback to
a PacRequest object, which doesn't exist any more.

Previously, CancelRequest just erased a callback that |job_| had. Right now, it
is not needed, because RequestImpl just kills |job_|

[commit-bot: I haz the power, 2016-10-17 08:13:43]

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

[commit-bot: I haz the power, 2016-10-17 08:13:44]

Dry run: This issue passed the CQ dry run.

[boliu, 2016-10-17 15:27:59]

On 2016/10/17 07:28:47, maksims wrote:
> mailto:boliu@chromium.org: Please review changes in 
> 
> content/

rs lgtm

> 
> eroman@, please see the fix.
> 
>
https://codereview.chromium.org/2299963002/diff/180001/net/proxy/proxy_resolv...
> File net/proxy/proxy_resolver_factory_mojo.cc (right):
> 
>
https://codereview.chromium.org/2299963002/diff/180001/net/proxy/proxy_resolv...
> net/proxy/proxy_resolver_factory_mojo.cc:207:
> job_->CompleteRequest(ERR_PAC_SCRIPT_TERMINATED);
> Ok, this one introduced a reentrancy problem.
> When ProxyService::CancelPacRequest() is called, the pac request is destroyed
> along with the request it has sent to ProxeResolverMojo. 
> Then ~RequestImpl() calls CompleteRequest and triggers the original callback
to
> a PacRequest object, which doesn't exist any more.
> 
> Previously, CancelRequest just erased a callback that |job_| had. Right now,
it
> is not needed, because RequestImpl just kills |job_|

[eroman, 2016-10-17 18:29:43]

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

[eroman, 2016-10-17 18:29:44]

Sam, can you give the *mojo* files a look-over too ?
Thanks!

https://codereview.chromium.org/2299963002/diff/180001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_factory_mojo.cc (right):

https://codereview.chromium.org/2299963002/diff/180001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:207:
job_->CompleteRequest(ERR_PAC_SCRIPT_TERMINATED);
On 2016/10/17 07:28:47, maksims wrote:
> Ok, this one introduced a reentrancy problem.
> When ProxyService::CancelPacRequest() is called, the pac request is destroyed
> along with the request it has sent to ProxeResolverMojo. 
> Then ~RequestImpl() calls CompleteRequest and triggers the original callback
to
> a PacRequest object, which doesn't exist any more.
> 
> Previously, CancelRequest just erased a callback that |job_| had. Right now,
it
> is not needed, because RequestImpl just kills |job_|

Can you add a unit-test that exercises this?

[maksims (do not use this acc), 2016-10-18 08:20:57]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[maksims (do not use this acc), 2016-10-18 08:20:58]

https://codereview.chromium.org/2299963002/diff/180001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_factory_mojo.cc (right):

https://codereview.chromium.org/2299963002/diff/180001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:207:
job_->CompleteRequest(ERR_PAC_SCRIPT_TERMINATED);
On 2016/10/17 18:29:43, eroman wrote:
> On 2016/10/17 07:28:47, maksims wrote:
> > Ok, this one introduced a reentrancy problem.
> > When ProxyService::CancelPacRequest() is called, the pac request is
destroyed
> > along with the request it has sent to ProxeResolverMojo. 
> > Then ~RequestImpl() calls CompleteRequest and triggers the original callback
> to
> > a PacRequest object, which doesn't exist any more.
> > 
> > Previously, CancelRequest just erased a callback that |job_| had. Right now,
> it
> > is not needed, because RequestImpl just kills |job_|
> 
> Can you add a unit-test that exercises this?

Basically, there is a unittest for that. I've modified it a bit and now it tests
the cancellation more thoroughly.

[maksims (do not use this acc), 2016-10-18 08:21:51]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

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

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 09:54:34]

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

[commit-bot: I haz the power, 2016-10-18 09:54:35]

Dry run: This issue passed the CQ dry run.

[maksims (do not use this acc), 2016-10-19 07:01:15]

A gentle reminder.

[Sam McNally, 2016-10-19 07:46:18]

https://codereview.chromium.org/2299963002/diff/220001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_factory_mojo.cc (right):

https://codereview.chromium.org/2299963002/diff/220001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:157: class
ProxyResolverMojo::RequestImpl : public ProxyResolver::Request {
Can we merge RequestImpl and Job?

https://codereview.chromium.org/2299963002/diff/220001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:187: friend class
base::RefCounted<Job>;
Remove.

https://codereview.chromium.org/2299963002/diff/220001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:253: CompletionCallback callback =
callback_;
CompletionCallback callback = base::ResetAndReturn(&callback_);

https://codereview.chromium.org/2299963002/diff/220001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:255: if (resolver_)
Add binding_.Close(). That will prevent an OnConnectionError call happening
after a ReportResult call. Then |resolver_| and |callback| should never be null.

https://codereview.chromium.org/2299963002/diff/220001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:306: pending_jobs_.erase(job);
Why remove the DCHECK on erase()?

[maksims (do not use this acc), 2016-10-19 09:59:02]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 09:59:42]

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

[maksims (do not use this acc), 2016-10-19 10:02:50]

https://codereview.chromium.org/2299963002/diff/220001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_factory_mojo.cc (right):

https://codereview.chromium.org/2299963002/diff/220001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:157: class
ProxyResolverMojo::RequestImpl : public ProxyResolver::Request {
On 2016/10/19 07:46:18, Sam McNally wrote:
> Can we merge RequestImpl and Job?

 It's just a request that is dependent on a job. The logic would be ruined.

https://codereview.chromium.org/2299963002/diff/220001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:187: friend class
base::RefCounted<Job>;
On 2016/10/19 07:46:18, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/2299963002/diff/220001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:253: CompletionCallback callback =
callback_;
On 2016/10/19 07:46:18, Sam McNally wrote:
> CompletionCallback callback = base::ResetAndReturn(&callback_);

Done.

https://codereview.chromium.org/2299963002/diff/220001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:255: if (resolver_)
On 2016/10/19 07:46:18, Sam McNally wrote:
> Add binding_.Close(). That will prevent an OnConnectionError call happening
> after a ReportResult call. Then |resolver_| and |callback| should never be
null.

Resolver_ must be nulled here because cancellation is different now. When
RequestImpl dies ~Job is called, which must remove itself from resolver.

https://codereview.chromium.org/2299963002/diff/220001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:306: pending_jobs_.erase(job);
On 2016/10/19 07:46:18, Sam McNally wrote:
> Why remove the DCHECK on erase()?

I have no idea. It's just a reland. Originally, the code is not mine. I've just
fixed a crash after this cl was introduced.

DCHECK is placed back.

[commit-bot: I haz the power, 2016-10-19 13:07:57]

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

[commit-bot: I haz the power, 2016-10-19 13:07:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[eroman, 2016-10-19 23:29:35]

Thanks @makdisms for reviving this old change!

This looks good. I believe you have correctly identified and fixed the cause of
the crash for the original change (in patchset #2).

lgtm once Sam is satisfied with proxy_resolver_factory_mojo.cc.

https://codereview.chromium.org/2299963002/diff/220001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver.h (right):

https://codereview.chromium.org/2299963002/diff/220001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver.h:45: // |*request| is written to. Call
request_.reset() to cancel the request
"request_.reset()" --> "request->reset()"

https://codereview.chromium.org/2299963002/diff/220001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_factory_mojo.cc (right):

https://codereview.chromium.org/2299963002/diff/220001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:157: class
ProxyResolverMojo::RequestImpl : public ProxyResolver::Request {
On 2016/10/19 10:02:50, maksims wrote:
> On 2016/10/19 07:46:18, Sam McNally wrote:
> > Can we merge RequestImpl and Job?
> 
>  It's just a request that is dependent on a job. The logic would be ruined.

I agree with Sam -- RequestImpl here just owns and forwards to a Job.

We could for instance make Job inherit ProxyResolver::Request as well, and
remove the need for this class.

@maksims: If you aren't comfortable making that change as part of this
changelist, we can address it in a follow-up  (since there is already enough
going on in this old CL :)

https://codereview.chromium.org/2299963002/diff/220001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:306: pending_jobs_.erase(job);
On 2016/10/19 10:02:50, maksims wrote:
> On 2016/10/19 07:46:18, Sam McNally wrote:
> > Why remove the DCHECK on erase()?
> 
> I have no idea. It's just a reland. Originally, the code is not mine. I've
just
> fixed a crash after this cl was introduced.
> 
> DCHECK is placed back.

In fact, I think we should remove |pending_jobs_| all together.

The only purpose that |pending_jobs_| serves now is for some DCHECKs (to ensure
that requests don't outlive the ProxyResolverMojo)

I don't think those assertions are worth the extra code to implement them.

My suggestion is to delete:
 * ProxyResolverMojo::pending_jobs_
 * ProxyResolverMojo::RemoveJob
  * ProxyResolverMojo::Job::resolver_

Sam, are you OK with that?

[Sam McNally, 2016-10-19 23:33:51]

lgtm

https://codereview.chromium.org/2299963002/diff/220001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_factory_mojo.cc (right):

https://codereview.chromium.org/2299963002/diff/220001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:255: if (resolver_)
On 2016/10/19 10:02:50, maksims wrote:
> On 2016/10/19 07:46:18, Sam McNally wrote:
> > Add binding_.Close(). That will prevent an OnConnectionError call happening
> > after a ReportResult call. Then |resolver_| and |callback| should never be
> null.
> 
> Resolver_ must be nulled here because cancellation is different now. When
> RequestImpl dies ~Job is called, which must remove itself from resolver.

Sorry, I wasn't clear, I only meant that when entering this method, resolver_
would never be null. You're right that it does need to be nulled here to avoid a
double remove.

https://codereview.chromium.org/2299963002/diff/220001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:306: pending_jobs_.erase(job);
On 2016/10/19 23:29:34, eroman wrote:
> On 2016/10/19 10:02:50, maksims wrote:
> > On 2016/10/19 07:46:18, Sam McNally wrote:
> > > Why remove the DCHECK on erase()?
> > 
> > I have no idea. It's just a reland. Originally, the code is not mine. I've
> just
> > fixed a crash after this cl was introduced.
> > 
> > DCHECK is placed back.
> 
> In fact, I think we should remove |pending_jobs_| all together.
> 
> The only purpose that |pending_jobs_| serves now is for some DCHECKs (to
ensure
> that requests don't outlive the ProxyResolverMojo)
> 
> I don't think those assertions are worth the extra code to implement them.
> 
> My suggestion is to delete:
>  * ProxyResolverMojo::pending_jobs_
>  * ProxyResolverMojo::RemoveJob
>   * ProxyResolverMojo::Job::resolver_
> 
> Sam, are you OK with that?

Yes.

[maksims (do not use this acc), 2016-10-20 10:12:31]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 10:12:45]

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-20 11:03:05]

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

[commit-bot: I haz the power, 2016-10-20 11:03:06]

Dry run: This issue passed the CQ dry run.

[maksims (do not use this acc), 2016-10-20 11:12:17]

The CQ bit was checked by maksim.sisov@intel.com

[maksims (do not use this acc), 2016-10-20 11:12:17]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
sammc@chromium.org, eroman@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/2299963002/#ps260001 (title: "remove
fields proposed by eroman")

[commit-bot: I haz the power, 2016-10-20 11:12: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-10-20 11:20:20]

Description was changed from

==========
Reland "Change ProxyResolver::GetProxyForURL() to take a unique_ptr<Request>*"

Original issue's description:
> Change ProxyResolver::GetProxyForURL() to take a scoped_ptr<Request>* rather
than a RequestHandle*
>
> * ProxyResolver::GetProxyForURL() fills a |scoped_pointer<Request>*|
>   rather than a |void*|
> * ProxyResolver::CancelRequest(void*) has been removed. Requests
>   are instead cancelled by resetting the scoped_ptr<Request>.
>
> This makes for less error prone code as cancellation of
> requests is automatic when the
> scoped_ptr<Request> goes out of scope.
> ProxyResolver::GetLoadState() is removed and replaced
> by Request::GetLoadState().
>
> Also made some renaming, as there were similar class
> named Job or Request. Now they are all Job and this new thing
> is Request.
>
> Referencing by address to object in vector was not wise in
net/proxy/mojo_proxy_resolver_impl_unittest.cc which is now fixed by using
scoped_ptrs in that vector.
>
> BUG=478934
>
> Committed: https://crrev.com/a750e126346aa42df1b0cbc2ae6a58abbe7a5069
> Cr-Commit-Position: refs/heads/master@{#377856}

BUG=478934
==========

to

==========
Reland "Change ProxyResolver::GetProxyForURL() to take a unique_ptr<Request>*"

Original issue's description:
> Change ProxyResolver::GetProxyForURL() to take a scoped_ptr<Request>* rather
than a RequestHandle*
>
> * ProxyResolver::GetProxyForURL() fills a |scoped_pointer<Request>*|
>   rather than a |void*|
> * ProxyResolver::CancelRequest(void*) has been removed. Requests
>   are instead cancelled by resetting the scoped_ptr<Request>.
>
> This makes for less error prone code as cancellation of
> requests is automatic when the
> scoped_ptr<Request> goes out of scope.
> ProxyResolver::GetLoadState() is removed and replaced
> by Request::GetLoadState().
>
> Also made some renaming, as there were similar class
> named Job or Request. Now they are all Job and this new thing
> is Request.
>
> Referencing by address to object in vector was not wise in
net/proxy/mojo_proxy_resolver_impl_unittest.cc which is now fixed by using
scoped_ptrs in that vector.
>
> BUG=478934
>
> Committed: https://crrev.com/a750e126346aa42df1b0cbc2ae6a58abbe7a5069
> Cr-Commit-Position: refs/heads/master@{#377856}

BUG=478934
==========

[commit-bot: I haz the power, 2016-10-20 11:20:23]

Committed patchset #5 (id:260001)

[eroman, 2016-10-20 16:47:37]

for the record: committed patchset #5 LGTM

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

Description was changed from

==========
Reland "Change ProxyResolver::GetProxyForURL() to take a unique_ptr<Request>*"

Original issue's description:
> Change ProxyResolver::GetProxyForURL() to take a scoped_ptr<Request>* rather
than a RequestHandle*
>
> * ProxyResolver::GetProxyForURL() fills a |scoped_pointer<Request>*|
>   rather than a |void*|
> * ProxyResolver::CancelRequest(void*) has been removed. Requests
>   are instead cancelled by resetting the scoped_ptr<Request>.
>
> This makes for less error prone code as cancellation of
> requests is automatic when the
> scoped_ptr<Request> goes out of scope.
> ProxyResolver::GetLoadState() is removed and replaced
> by Request::GetLoadState().
>
> Also made some renaming, as there were similar class
> named Job or Request. Now they are all Job and this new thing
> is Request.
>
> Referencing by address to object in vector was not wise in
net/proxy/mojo_proxy_resolver_impl_unittest.cc which is now fixed by using
scoped_ptrs in that vector.
>
> BUG=478934
>
> Committed: https://crrev.com/a750e126346aa42df1b0cbc2ae6a58abbe7a5069
> Cr-Commit-Position: refs/heads/master@{#377856}

BUG=478934
==========

to

==========
Reland "Change ProxyResolver::GetProxyForURL() to take a unique_ptr<Request>*"

Original issue's description:
> Change ProxyResolver::GetProxyForURL() to take a scoped_ptr<Request>* rather
than a RequestHandle*
>
> * ProxyResolver::GetProxyForURL() fills a |scoped_pointer<Request>*|
>   rather than a |void*|
> * ProxyResolver::CancelRequest(void*) has been removed. Requests
>   are instead cancelled by resetting the scoped_ptr<Request>.
>
> This makes for less error prone code as cancellation of
> requests is automatic when the
> scoped_ptr<Request> goes out of scope.
> ProxyResolver::GetLoadState() is removed and replaced
> by Request::GetLoadState().
>
> Also made some renaming, as there were similar class
> named Job or Request. Now they are all Job and this new thing
> is Request.
>
> Referencing by address to object in vector was not wise in
net/proxy/mojo_proxy_resolver_impl_unittest.cc which is now fixed by using
scoped_ptrs in that vector.
>
> BUG=478934
>
> Committed: https://crrev.com/a750e126346aa42df1b0cbc2ae6a58abbe7a5069
> Cr-Commit-Position: refs/heads/master@{#377856}

BUG=478934

Committed: https://crrev.com/7e15726eca3718f99606e24c67c5794d9cb4a601
Cr-Commit-Position: refs/heads/master@{#426450}
==========

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

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