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

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}

[Olli Raula, 2015-11-13 05:46:31]

Description was changed from

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





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

to

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





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

[Olli Raula, 2015-11-17 11:38:01]

olli.raula@intel.com changed reviewers:
+ eroman@chromium.org

[Olli Raula, 2015-11-17 11:40:32]

Hi, it would be nice if you have some time to take a look and you have done
something similar before. I have still some problems, atleast mostly they are
because functions are called from invalid thread and Debugchecks crash the
program.

thanks

[eroman, 2015-11-17 18:39:55]

Sure I will take a look later today, and see if I have ideas about your crashes.

[Olli Raula, 2015-11-19 09:36:16]

Did you got any clue?

[eroman, 2015-11-24 01:21:00]

https://codereview.chromium.org/1439053002/diff/1/net/proxy/mock_proxy_resolv...
File net/proxy/mock_proxy_resolver.cc (right):

https://codereview.chromium.org/1439053002/diff/1/net/proxy/mock_proxy_resolv...
net/proxy/mock_proxy_resolver.cc:14: MockAsyncProxyResolver* resolver)
Doesn't the job already contain the resolver? (In which case not necessary to
have that parameter here).

https://codereview.chromium.org/1439053002/diff/1/net/proxy/mojo_proxy_resolv...
File net/proxy/mojo_proxy_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/1439053002/diff/1/net/proxy/mojo_proxy_resolv...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:118: if
(!resolver_->cancel_callback_.is_null()) {
I don't believe this is the right trigger anymore. RequestImpl will always reach
the destructor, but this only counts as "cancellation" if the request has not
yet completed.

If the request was already completed, should not call the cancellation callback.

https://codereview.chromium.org/1439053002/diff/1/net/proxy/multi_threaded_pr...
File net/proxy/multi_threaded_proxy_resolver.cc (right):

https://codereview.chromium.org/1439053002/diff/1/net/proxy/multi_threaded_pr...
net/proxy/multi_threaded_proxy_resolver.cc:247: if (job_->executor()) {
This doesn't seem correct. If the job was already completed, then job_ may have
already been freed.

Try running this with ASAN, will probably reveal a use-after-free.

(Note that the model changed. Before CancelRequest() was guaranteed to only be
called for oustanding requests, but generally destructors are called always,
even if the request does not need to be cancelled. So code needs to be
structured to so cancellation is only done when necessary).

https://codereview.chromium.org/1439053002/diff/1/net/proxy/proxy_resolver.h
File net/proxy/proxy_resolver.h (right):

https://codereview.chromium.org/1439053002/diff/1/net/proxy/proxy_resolver.h#...
net/proxy/proxy_resolver.h:44: // |*request| is written to, and can be passed to
CancelRequest().
Please update this comment.

https://codereview.chromium.org/1439053002/diff/1/net/proxy/proxy_resolver_fa...
File net/proxy/proxy_resolver_factory_mojo.cc (right):

https://codereview.chromium.org/1439053002/diff/1/net/proxy/proxy_resolver_fa...
net/proxy/proxy_resolver_factory_mojo.cc:121: base::ThreadChecker
thread_checker_;  // TODO this should be private
why not make it private here?

https://codereview.chromium.org/1439053002/diff/1/net/proxy/proxy_resolver_fa...
net/proxy/proxy_resolver_factory_mojo.cc:203: job_->Cancel();
Same issue on ownership -- Can't job_ already be deleted here?

[Olli Raula, 2015-12-18 14:13:48]

Hi, thanks for tips. I have done a lot of work and learned much. Now it is ready
for review. Could you check it. Are you available between Christmas and New
year?

[eroman, 2015-12-18 17:53:47]

I will be out around christmas/new year, but will probably manage to do a review
pass before then :)

[Olli Raula, 2015-12-21 06:50:19]

Description was changed from

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





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

to

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

Referencig by address to object in vector was not wice in
net/proxy/mojo_proxy_resolver_impl_unittest.cc




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

[Olli Raula, 2015-12-21 06:55:06]

Description was changed from

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

Referencig by address to object in vector was not wice in
net/proxy/mojo_proxy_resolver_impl_unittest.cc




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

to

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

[Olli Raula, 2015-12-21 10:55:24]

On 2015/12/18 17:53:47, eroman wrote:
> I will be out around christmas/new year, but will probably manage to do a
review
> pass before then :)

Ok, thanks, that would be nice because I am doing same thing to dns resolver so
comments here helps also there.

[Olli Raula, 2016-01-07 08:37:57]

Ping?

[Olli Raula, 2016-01-22 07:29:32]

Could you check this, please?

[eroman, 2016-01-22 21:58:07]

ops forgot, looking now!

[eroman, 2016-01-23 02:17:01]

I commented on two of the classes (MultiThreadProxyResolver and
ProxyResolverV8Tracing) so far, but those responses apply elsewhere too.

I would like to remove the dependency on WeakPtr.

Note that under the hood WeakPtr creates reference counted flag. This is
redundant given that the class we are adding this to is already reference
counted.

But the general cleanup is going to be great when complete, thanks!

https://codereview.chromium.org/1439053002/diff/80001/net/proxy/multi_threade...
File net/proxy/multi_threaded_proxy_resolver.cc (right):

https://codereview.chromium.org/1439053002/diff/80001/net/proxy/multi_threade...
net/proxy/multi_threaded_proxy_resolver.cc:123: void
RemovePendingJob(base::WeakPtr<Job> job) {
I believe this can be deleted based on my later suggestion.

https://codereview.chromium.org/1439053002/diff/80001/net/proxy/multi_threade...
net/proxy/multi_threaded_proxy_resolver.cc:157: class Job : public
base::SupportsWeakPtr<Job>,
I don't believe this should expose a weak pointer.

Since the type is already RefCountedThreadSafe it is easy to deal with shared
ownership by simply holding a refcount instead.

https://codereview.chromium.org/1439053002/diff/80001/net/proxy/multi_threade...
net/proxy/multi_threaded_proxy_resolver.cc:243: RequestImpl(base::WeakPtr<Job>
job, MultiThreadedProxyResolver* resolver)
Rather than a WeakPtr, can this instead just hold a strong reference to the Job?

As in, have a member:
 scoped_refptr<Job> job_;

There shouldn't be any reference cycles since callers own the
ProxyResolver::Request and must delete it before the ProxyResolver is deleted

https://codereview.chromium.org/1439053002/diff/80001/net/proxy/multi_threade...
net/proxy/multi_threaded_proxy_resolver.cc:246: ~RequestImpl() override {
We can simplify this to a single line:

job_->Cancel();

The RemovePendingJob() stuff can be rolled up into
MultiThreadedProxyResolver::OnExecutorReady by changing it to check for
cancelled jobs:

while (!pending_jobs_.empty()) {
  scoped_refptr<Job> job = pending_jobs_.front();
  pending_jobs_.pop_front();

  if (!job->was_cancelled()) {
    executor->StartJob(job.get());
    return;
  }
}

https://codereview.chromium.org/1439053002/diff/80001/net/proxy/multi_threade...
net/proxy/multi_threaded_proxy_resolver.cc:267: MultiThreadedProxyResolver*
resolver_;
With the suggestion above, this member is no longer needed, and RequestImpl is
nothing more than a light wrapper around the Job.

https://codereview.chromium.org/1439053002/diff/80001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/1439053002/diff/80001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.cc:949: : job_(job) {}
Same comment as for MultiThreadedProxyResolver. I don't think we want a weak
pointer here, can instead hold a strong reference.

https://codereview.chromium.org/1439053002/diff/80001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.cc:952: if (job_)
Shouldn't need this check once making job_ a strong reference.

https://codereview.chromium.org/1439053002/diff/80001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.cc:971: if (request)
I wonder if we actually use this check (should at some mandate that |request !=
nullptr|.
Shouldn't matter for this CL though since the Job is kept alive by the executor
already.

[Olli Raula, 2016-02-15 10:54:07]

Thanks for review, now those comments are addressed to everywhere. Some thougs:

also refptr is removed from some places. From mockasyncproxyresolver I tried to
do that also(patch set6) but there is some bug in windows compiler so I had to
use scoped_refptr there for now.

Can you check it againg!

[Olli Raula, 2016-02-18 08:29:06]

Hi, could you please check this as I am in a hurry to finish this.

[eroman, 2016-02-24 01:23:07]

Thanks for driving this patch forward!

I am working on review comments right now, hope to post them soon.

On Thu, Feb 18, 2016 at 12:29 AM, <olli.raula@intel.com> wrote:

> Hi, could you please check this as I am in a hurry to finish this.
> https://codereview.chromium.org/1439053002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[eroman, 2016-02-24 03:08:07]

Great work on this change!

The latest patchset is looking very close. Just want to see the responses to
these comments before I approve.

I know this is some very subtle code, thanks for working through all the cases
:)

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/mock_proxy_r...
File net/proxy/mock_proxy_resolver.cc (right):

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/mock_proxy_r...
net/proxy/mock_proxy_resolver.cc:20: job_->Resolver()->AddCancelledJob(job_);
At a minimum add a comment explaining that AddCancelledJob() checks to see if
the request was already completed (otherwise this looks incorrect to casual
readers).

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/mock_proxy_r...
net/proxy/mock_proxy_resolver.cc:42: // May delete |this|.
Remove this comment, as I don't believe it is true anymore.

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/mock_proxy_r...
net/proxy/mock_proxy_resolver.cc:60: // Test code completes the request by
calling request->CompleteNow().
Please update this comment.

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/mock_proxy_r...
File net/proxy/mock_proxy_resolver.h (right):

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/mock_proxy_r...
net/proxy/mock_proxy_resolver.h:37: MockAsyncProxyResolver* Resolver() const {
return resolver_; };
can call this resolver()

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/mock_proxy_r...
net/proxy/mock_proxy_resolver.h:54: RequestImpl(scoped_refptr<Job> job);
mark as "explicit"

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/mock_proxy_r...
net/proxy/mock_proxy_resolver.h:61: // TODO This should be scoped_ptr but
windows compiler have some bug.
What is the bug that happens?

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/mojo_proxy_r...
File net/proxy/mojo_proxy_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/mojo_proxy_r...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:118: if (job_) {
Is it ever the case that !job_ ?

From reading the code, it looks like pending_jobs_ is never shrunk, nor is job_
here reset to null after "completing" the request. Instead code completes the
request by simply running the callback, and leaving the request in
pending_requests_.

Seems like that is how this code previously worked, just want to understand if I
am missing something here.

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/multi_thread...
File net/proxy/multi_threaded_proxy_resolver.cc (right):

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/multi_thread...
net/proxy/multi_threaded_proxy_resolver.cc:235: RequestImpl(scoped_refptr<Job>
job) : job_(job) {}
You can do: job_(std::move(job)) to avoid a spurious AddRef.

Also please mark this constructor as "explicit" per style guide.

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

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver.h:45: // |scoped_ptr<request>*| is written to and can
be used for cancellation with
How about rephrasing this:

|*request| is written to. Call request->reset() to cancel. The
scoped_ptr<Request> must not outlive the ProxyResolver that assigned it.

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

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:154: RequestImpl(scoped_ptr<Job> job);
explicit

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:179: ProxyResolverMojo* GetResolver();
resolver()

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:246: bool
ProxyResolverMojo::Job::IsCallback() {
Is this used anywhere? I believe you can delete.

Otherwise as far as naming, HasCallback() or WasCancelled() would be better.

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:262: if (resolver_)
Your refactor here looks correct (good job reading through this file!)

But on a side-note, I find the (existing) code quite confusing!

resolver_->RemoveJob() may call back into |this| to invoke
PacScriptTerminated(), which may then invoke hte callback. Even more confusing
is ReportResult() below, which calls RemoveJob() but must first call
callback_.Reset() otherwise it will dispatch to the callback.

I think we can improve this by eliminating the re-entrancy.

For instance introducing a Job::CompleteRequest(int) method:

void Job::CompleteRequest(int result) {
  if (resolver_)
    resolver_->RemoveJob(this);
  resolver_ = nullptr;

  if (!callback_.is_null())
    callback_.Run(result);
  callback_.Reset();
}

And changing RemoveJob() to not do any re-entrancy into |job|:

ProxyResolverMojo::RemoveJob(Job* job) {
  DCHECK(....);
  pending_jobs_.erase(job);
}

This means all the places that would otherwise call resolver_->RemoveJob(this),
possibly doing a null check or re-setting callback_, can now just call
CompleteRequest() with the expected result.

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:263: resolver_->RemoveJob(this);
Based on suggestion above, this would become:

   CompleteRequest(ERR_PAC_SCRIPT_TERMINATED);

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:277: CompletionCallback callback =
callback_;
Based on the suggestion above, the remainder of this function would be replaced
by:

  CompleteRequest(error);

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_v8_tracing.cc (left):

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing.cc:948: if (request)
Was this intentionally removed? If nothing depends on it and you did mean to
remove it, then please update the documentation for ProxyResolverV8Tracing which
I believe says this can be nullptr.

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing.cc:319: RequestImpl(scoped_refptr<Job> job);
explicit

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing.cc:392: // (f) The script execution
completed entirely, and posted a task to the
There is another possibility - that the job already completed successfully. (see
later comment for suggestion).

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing.cc:445: if (callback_.is_null())
I don't think this change should go here.

(I presume this was added because Job::Cancel() might be called on a request
that has already completed, in which case it should not be completed a second
time.)

Instead I think we should do the check in Job::Cancel(), since cancellation is
already fairly complicated.

In Job::Cancel() at the top we can do:

  // If the job already completed, there is nothing to be cancelled.
  if (callback_.is_null())
    return;

And then remove this change to ReleaseCallback()

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing.cc:948: : job_(job) {}
std::move()

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_v8_tracing.h (right):

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing.h:22: class Request : public
ProxyResolver::Request {};
Why is this needed? Can it just be declared in the .cc file?

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing.h:55: // written to, and can be passed to
CancelRequest().
Please update the comment referencing CancelRequest().

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_servic...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_servic...
net/proxy/proxy_service_unittest.cc:246: // Given a jobmap  and a list of
nit: Can you re-wrap this comment, and remove the extra space after jobmap?

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_servic...
net/proxy/proxy_service_unittest.cc:247: // target URLs |_urls|, asserts that
the set of URLs of the jobs appearing
_urls --> urls (looks like the previous comment was wrong).

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_servic...
net/proxy/proxy_service_unittest.cc:248: // in |list| is exactly the set of URLs
in |_urls|.
_urls --> urls (looks like the previous comment was wrong)

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_servic...
net/proxy/proxy_service_unittest.cc:249: JobMap GetJobsForURLs(const JobMap map,
const std::vector<GURL>& urls) {
const JobMap map --> const JobMap& map

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_servic...
net/proxy/proxy_service_unittest.cc:250: int a = urls.size();
write this as either:
   size_t a = urls.size();
or
   auto a = urls.size();

(size_t --> int can be a truncating operation otherwise and may trigger compiler
warnings)

[Olli Raula, 2016-02-24 13:06:25]

Thanks for reading it through!

now they should be fixed. I hope you have time to check this asap because I lost
access to this account this Friday.

[eroman, 2016-02-24 19:20:29]

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/mock_proxy_r...
File net/proxy/mock_proxy_resolver.h (right):

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/mock_proxy_r...
net/proxy/mock_proxy_resolver.h:54: RequestImpl(scoped_refptr<Job> job);
On 2016/02/24 03:08:06, eroman wrote:
> mark as "explicit"

This comment was not addressed.

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/mock_proxy_r...
net/proxy/mock_proxy_resolver.h:61: // TODO This should be scoped_ptr but
windows compiler have some bug.
On 2016/02/24 03:08:06, eroman wrote:
> What is the bug that happens?

What is the response to this comment?

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_v8_tracing.cc (left):

https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing.cc:948: if (request)
On 2016/02/24 03:08:06, eroman wrote:
> Was this intentionally removed? If nothing depends on it and you did mean to
> remove it, then please update the documentation for ProxyResolverV8Tracing
which
> I believe says this can be nullptr.

This comment was not addressed.

https://codereview.chromium.org/1439053002/diff/160001/net/proxy/mojo_proxy_r...
File net/proxy/mojo_proxy_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/1439053002/diff/160001/net/proxy/mojo_proxy_r...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:118: job_->cancelled = true;
This does change the meaning from before:

Previously cancelled was only set when the request was actually cancelled,
whereas now it is set every time the RequestImpl is destroyed.

This also means that the "cancel_callback_" is run always, even when the request
was not truly cancelled.

To address this, I suggest the following changes:

  (1) Rename Job::callback --> Job::callback_, and make it private (to avoid
code calling it directly).
  (2) Instead of test code calling job->callback.Run(XXX), replace those with
job->Complete(XXX), where the new method is resets the callback after running
it:

   void Job::Complete(int result) {
      DCHECK(!callback_.is_null())
      callback_.Run(result);
      callback_.Reset();
    }

This allows distinguishing between requests that were completed vs those that
did not:

   bool Job::WasCompleted() {
     return callback_.is_null();
   }

Now this code in ~RequestImpl() can distinguish between an actual cancellation,
versus destruction of an already completed request:

   if (job_->WasCompleted())
     return;
   
   job_->cancelled = true;
   if (!resolver_->cancel_callback_.is_null()) {
      ...
   }

https://codereview.chromium.org/1439053002/diff/160001/net/proxy/mojo_proxy_r...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:143: // Wait until the mock
resolver has received a CancelRequest call.
This comment is no longer up to date.

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

https://codereview.chromium.org/1439053002/diff/160001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:174: void PacScriptTerminated();
Delete this (unused)

https://codereview.chromium.org/1439053002/diff/160001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:205: job_->CompleteRequest(0);
Instead of 0, this should be ERR_PAC_SCRIPT_TERMINATED right?

Since this represents the actual cancellation case.

https://codereview.chromium.org/1439053002/diff/160001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:234: void
ProxyResolverMojo::Job::PacScriptTerminated() {
Delete this (unused)

https://codereview.chromium.org/1439053002/diff/160001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_factory_mojo.cc:256:
CompleteRequest(ERR_PAC_SCRIPT_TERMINATED);
Thanks, this is easier to follow!

[Olli Raula, 2016-02-25 08:39:46]

olli.raula@intel.com changed reviewers:
+ davidben@chromium.org

[Olli Raula, 2016-02-25 08:43:27]

Thanks. Added davidben for content(or could we use tbr for that?)

sorry for missing some of the comments, hopefully now I saw all.
some comments below


>
https://codereview.chromium.org/1439053002/diff/120001/net/proxy/mock_proxy_r...
> net/proxy/mock_proxy_resolver.h:61: // TODO This should be scoped_ptr but
> windows compiler have some bug.
> On 2016/02/24 03:08:06, eroman wrote:
> > What is the bug that happens?

I added more info to comment but here is whole error log from buildbot:

ProxyServiceTest.BypassDoesntApplyToPac (run #1):
[ RUN      ] ProxyServiceTest.BypassDoesntApplyToPac
Backtrace:
	net::MockAsyncProxyResolver::RequestImpl::`scalar deleting destructor'
[0x014B809E+30]
	net::ProxyService::PacRequest::QueryDidComplete [0x0126E95A+122]
	net::ProxyService::PacRequest::QueryComplete [0x0126E8A1+17]
	base::internal::Invoker<base::IndexSequence<0>,base::internal::BindState<base::internal::RunnableAdapter<void
(__thiscall net::ViewCacheHelper::*)(int)>,void __cdecl(net::ViewCacheHelper
*,int),base::internal::UnretainedWrapper<net::ViewCacheHelper> >,bas
[0x00F39FA3+19]
	net::MockAsyncProxyResolver::Job::CompleteNow [0x014B8178+40]
	net::ProxyServiceTest_BypassDoesntApplyToPac_Test::TestBody [0x00A29F6B+2251]
	testing::Test::Run [0x01233120+224]
	testing::TestInfo::Run [0x01233450+240]
	testing::TestCase::Run [0x0123329A+266]
	testing::internal::UnitTestImpl::RunAllTests [0x012338B9+665]
	testing::UnitTest::Run [0x012335F6+214]
	base::TestSuite::Run [0x016BB8F9+201]
	base::`anonymous namespace'::LaunchUnitTestsInternal [0x016B94D9+713]
	base::LaunchUnitTests [0x016B91F8+104]
	main [0x0103EF8F+127]
	__tmainCRTStartup [0x017F860F+254] (f:\ddctools\crt\crtw32\startup\crt0.c:255)
	BaseThreadInitThunk [0x7665336A+18]
	RtlInitializeExceptionChain [0x774B9882+99]
	RtlInitializeExceptionChain [0x774B9855+54]


 
>
https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_resolv...
> File net/proxy/proxy_resolver_v8_tracing.cc (left):
> 
>
https://codereview.chromium.org/1439053002/diff/120001/net/proxy/proxy_resolv...
> net/proxy/proxy_resolver_v8_tracing.cc:948: if (request)
> On 2016/02/24 03:08:06, eroman wrote:
> > Was this intentionally removed? If nothing depends on it and you did mean to
> > remove it, then please update the documentation for ProxyResolverV8Tracing
> which
> > I believe says this can be nullptr.

I Changed everything so that there have to be request. Comment fixed

[eroman, 2016-02-25 16:19:00]

eroman@chromium.org changed reviewers:
- davidben@chromium.org

[eroman, 2016-02-25 16:19:01]

You can TBR David for the content change
(content/browser/resolve_proxy_msg_helper_unittest.cc) once I have approved the
rest (Moving him to cc list for now)

As far as that TODO about compiler bug, I am pretty sure that isn't the root
problem, and is likely indicative of lifetime problem instead.
I will take a look at that backtrace and get back to you...

[eroman, 2016-02-25 16:54:32]

OK so I can't tell exactly what goes wrong, but I am pretty sure it has to do
with the re-entrancy into the Job (the request is deleted while executing the
completion callback)

What happens in that test case is:
  (1) A job owned by a request is completed
  (2) The job calls its completion callback
  (3) The completion callback destroys the request (that is currently
dispatching)
  (4) Crash happens around here

Using patchset 5 as my guide (is this the code you had problems with?), it looks
like the job is removed from the pending list before dispatching the completion
callback, so then when ~RequestImpl() it should not be going down the
cancellation path, which is good. (The cancellation path would transfer the job
to the cancelled vector).

Instead it will destroy the Job. This _should_ be OK, because even though
Job::CompleteNow is still on the callstack, it made a copy of the callback and
does not use any further member variables after the  deletion.

The crash in the "scalar" destructor means it is likely crashing somewhere in
~Job which is the only real work that it does.

So yeah, I am kind of stumped too from just looking at the code. Deleting while
execution in that object is still on the stack is of course a scary pattern, but
the previous code was doing that too.

[eroman, 2016-02-25 17:25:34]

Ah! I have an idea:

The destructor in your failing patchset was doing this:
   job_->Resolver()->AddCancelledJob(std::move(job_));

My C++ isn't strong enough to say this for certain, but probably the order of
argument evaluation here is unspecified, and happening in a different order on
windows.

Specifically, if std::move(job_) is evaluated before job_->Resolver(), then
job_->Resolver() will dereference an invalid scoped_ptr<>, and probably end up
calling nullptr->AddCancelledJob() or some such thing.

This would explain why when you changed things to use a scoped_refptr the
problem went away, since std::move was no longer being used.

[eroman, 2016-02-25 17:51:58]

I confirmed with C++ expert rsleevi, and the theory above is correct.

On Thu, Feb 25, 2016 at 9:25 AM, <eroman@chromium.org> wrote:

> Ah! I have an idea:
>
> The destructor in your failing patchset was doing this:
> job_->Resolver()->AddCancelledJob(std::move(job_));
>
> My C++ isn't strong enough to say this for certain, but probably the order
> of
> argument evaluation here is unspecified, and happening in a different
> order on
> windows.
>
> Specifically, if std::move(job_) is evaluated before job_->Resolver(), then
> job_->Resolver() will dereference an invalid scoped_ptr<>, and probably
> end up
> calling nullptr->AddCancelledJob() or some such thing.
>
> This would explain why when you changed things to use a scoped_refptr the
> problem went away, since std::move was no longer being used.
>
> https://codereview.chromium.org/1439053002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[eroman, 2016-02-25 17:53:44]

lgtm!

https://codereview.chromium.org/1439053002/diff/180001/net/proxy/mock_proxy_r...
File net/proxy/mock_proxy_resolver.h (right):

https://codereview.chromium.org/1439053002/diff/180001/net/proxy/mock_proxy_r...
net/proxy/mock_proxy_resolver.h:27: class Job : public base::RefCounted<Job> {
Based on our understanding of the failure, this can be changed back to being
non-refcounted.

I imagine you don't have time to make that change before losing access on
Friday, so feel free to leave this as-is and just add a TODO:

TODO(eroman): no need for this to be RefCounted.

https://codereview.chromium.org/1439053002/diff/180001/net/proxy/mock_proxy_r...
net/proxy/mock_proxy_resolver.h:42: virtual ~Job();
No need for this to be virtual.

https://codereview.chromium.org/1439053002/diff/180001/net/proxy/mock_proxy_r...
net/proxy/mock_proxy_resolver.h:61: // TODO This should be scoped_ptr but
windows compiler have some bug.
Remove this comment.

The problem as described in earlier comment was function argument evaluation
order difference between Windows compiler, and is easily solved.

https://codereview.chromium.org/1439053002/diff/180001/net/proxy/proxy_servic...
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/1439053002/diff/180001/net/proxy/proxy_servic...
net/proxy/proxy_service.cc:834: // This state is cleared when resolve_job_ is
set to nullptr below.
nit: "is set to nullptr below." --> "is reset below."

[davidben, 2016-02-25 18:12:29]

davidben@chromium.org changed reviewers:
+ davidben@chromium.org

[davidben, 2016-02-25 18:12:30]

content lgtm

[Olli Raula, 2016-02-26 09:03:22]

The CQ bit was checked by olli.raula@intel.com

[Olli Raula, 2016-02-26 09:03:23]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1439053002/#ps200001
(title: "Restore scoped_ptr to mock and nits")

[commit-bot: I haz the power, 2016-02-26 09:03:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1439053002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1439053002/200001

[commit-bot: I haz the power, 2016-02-26 09:19:04]

Description was changed from

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

to

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

[commit-bot: I haz the power, 2016-02-26 09:19:05]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-02-26 09:20:25]

Description was changed from

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

to

==========
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}
==========

[commit-bot: I haz the power, 2016-02-26 09:20:27]

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

[Olli Raula, 2016-02-26 10:20:23]

Thanks a lot! I fixed that refptr.

[eroman, 2016-02-26 19:04:01]

patchset 11 LGTM

[eroman, 2016-02-29 18:49:00]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/1745133002/ by eroman@chromium.org.

The reason for reverting is: Top crasher on Canary has PAC script cancellation
on callstack. Very likely a bug or interaction with this CL.

Reverting until I have a chance to fully investigate..