Handle ERR_PAC_SCRIPT_TERMINATED in ProxyService.

Handle ERR_PAC_SCRIPT_TERMINATED in ProxyService.

If the out-of-process v8 pac resolver crashes, ProxyResolverMojo
returns ERR_PAC_SCRIPT_TERMINATED as its error. This changes
ProxyService to discard its ProxyResolver when that error is returned,
and if other requests were in progress, start reinitializing a new
ProxyResolver.

This also changes ProxyResolverMojo to correctly handle being deleted
while running a CompletionCallback.

BUG=467832
Committed: https://crrev.com/0a74fd9a8fb634aa6e7407afed9fa556fc8ebca2
Cr-Commit-Position: refs/heads/master@{#326960}

[Sam McNally, 2015-04-08 00:34:39]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2015-04-08 00:55:01]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2015-04-08 00:57:44]

Patchset #1 (id:40001) has been deleted

[Sam McNally, 2015-04-08 00:59:30]

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

[Anand Mistry (off Chromium), 2015-04-09 04:51:00]

lgtm

LGTM on the mojo part.

[eroman, 2015-04-15 00:56:52]

https://codereview.chromium.org/1069483003/diff/60001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_mojo.cc (right):

https://codereview.chromium.org/1069483003/diff/60001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo.cc:200: while (weak_this &&
!pending_jobs_.empty()) {
What is this needed for? This doesn't seem right, why would we be testing or
deletion of |this|? 

|this| being deleted is indicative of bigger problems no?

https://codereview.chromium.org/1069483003/diff/60001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_mojo.h (right):

https://codereview.chromium.org/1069483003/diff/60001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo.h:100: base::WeakPtrFactory<ProxyResolverMojo>
weak_factory_;
Please think about using WeakPointer less rather than more. It makes ownership
and lifetimes hard to reason about.

https://codereview.chromium.org/1069483003/diff/60001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/1069483003/diff/60001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1423: if (request->is_started()) {
why not reset the config whenever there are requests (regardless if they have
been started) ?

https://codereview.chromium.org/1069483003/diff/60001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1433: if (!config_.pac_mandatory()) {
config_ should not be tested and relied upon after calling ResetProxyConfig()
above. The state machine has been reset.

[Sam McNally, 2015-04-15 03:40:36]

Patchset #2 (id:80001) has been deleted

[Sam McNally, 2015-04-15 04:34:56]

Patchset #3 (id:120001) has been deleted

[Sam McNally, 2015-04-15 04:35:56]

https://codereview.chromium.org/1069483003/diff/60001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_mojo.cc (right):

https://codereview.chromium.org/1069483003/diff/60001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo.cc:200: while (weak_this &&
!pending_jobs_.empty()) {
On 2015/04/15 00:56:52, eroman wrote:
> What is this needed for? This doesn't seem right, why would we be testing or
> deletion of |this|? 
> 
> |this| being deleted is indicative of bigger problems no?

Done. GetProxyForUrl requests will no longer immediately fail if the main mojo
connection disconnects; they'll fail once their individual request connections
disconnect.

https://codereview.chromium.org/1069483003/diff/60001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_mojo.h (right):

https://codereview.chromium.org/1069483003/diff/60001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo.h:100: base::WeakPtrFactory<ProxyResolverMojo>
weak_factory_;
On 2015/04/15 00:56:52, eroman wrote:
> Please think about using WeakPointer less rather than more. It makes ownership
> and lifetimes hard to reason about.

Removed it.

https://codereview.chromium.org/1069483003/diff/60001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/1069483003/diff/60001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1423: if (request->is_started()) {
On 2015/04/15 00:56:52, eroman wrote:
> why not reset the config whenever there are requests (regardless if they have
> been started) ?

Done.

https://codereview.chromium.org/1069483003/diff/60001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1433: if (!config_.pac_mandatory()) {
On 2015/04/15 00:56:52, eroman wrote:
> config_ should not be tested and relied upon after calling ResetProxyConfig()
> above. The state machine has been reset.

Done.

[Sam McNally, 2015-04-24 01:25:03]

ping

[eroman, 2015-04-24 07:22:10]

lgtm

[Sam McNally, 2015-04-25 05:23:00]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2015-04-25 05:23:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
amistry@chromium.org, eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1069483003/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2015-04-25 05:23:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1069483003/160001

[commit-bot: I haz the power, 2015-04-25 05:26:46]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2015-04-25 05:27:45]

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