ProxyResolverMac: invalidate resolver source.

ProxyResolverMac: invalidate resolver source.

Instead of calling CFRunLoopRemoveSource, call CFRunLoopSourceInvalidate. Since
the runloop is bound to the thread and threads are reused between resolutions,
if the internals of the proxy resolver create any deferred tasks on the runloop,
those tasks could refer to the removed-and-destroyed source when the runloop is
next run. Instead, we call Invalidate, which should prevent any deferred tasks
from running.

BUG=166387
Committed: https://crrev.com/824907ba81b615ccbbc35c51bc9b937f3d99a5f2
Cr-Commit-Position: refs/heads/master@{#318233}

[Elly Fong-Jones, 2015-02-25 22:15:22]

ellyjones@chromium.org changed reviewers:
+ davidben@chromium.org, rsesek@chromium.org

[Elly Fong-Jones, 2015-02-25 22:15:35]

rsesek: PTAL?
davidben: net owner review :)

[davidben, 2015-02-25 22:38:38]

Disclaimer: I don't know anything about OS X and CFRunLoop.

Do we not want these deferred tasks to run? Perhaps they're posting cleanup
tasks asynchronously for whatever odd reason.

https://codereview.chromium.org/959803004/diff/1/net/proxy/proxy_resolver_mac.cc
File net/proxy/proxy_resolver_mac.cc (right):

https://codereview.chromium.org/959803004/diff/1/net/proxy/proxy_resolver_mac...
net/proxy/proxy_resolver_mac.cc:107: // runloop source we need to release
despite its name.)
This seems to date to https://crrev.com/4293, before GetProxyForURL got a
CompletionCallback and everything. Perhaps we should make this asynchronous with
a CFRunLoop and everything now..

[davidben, 2015-02-25 22:41:25]

https://codereview.chromium.org/959803004/diff/1/net/proxy/proxy_resolver_mac.cc
File net/proxy/proxy_resolver_mac.cc (right):

https://codereview.chromium.org/959803004/diff/1/net/proxy/proxy_resolver_mac...
net/proxy/proxy_resolver_mac.cc:107: // runloop source we need to release
despite its name.)
On 2015/02/25 22:38:38, David Benjamin wrote:
> This seems to date to https://crrev.com/4293, before GetProxyForURL got a
> CompletionCallback and everything. Perhaps we should make this asynchronous
with
> a CFRunLoop and everything now..

Looks like the async bits were added in http://codereview.chromium.org/149525

[davidben, 2015-02-25 23:11:20]

Elly and I chatted out of band. The async thing is potentially an optimization
we could do (maybe run more of these in parallel without threads or something),
but doesn't really do anything about this. You still need to know what to do
with the source when the operation is done.

The API is documented as:
https://developer.apple.com/library/mac/documentation/CoreFoundation/Referenc...
"NOTE: If you want to terminate the request before completion, you should
invalidate the run loop source." which isn't quiiiite what we're doing, but it
does suggest that invalidating the source randomly is a supported operation.

Looking at WebKit, they use this API too.
https://trac.webkit.org/browser/trunk/Source/WebCore/platform/network/cf/Prox...

In fact they RemoveSource /and/ Invalidate. That's probably as close to a
canonical answer as we're going to get. Invalidate is documented as removing
itself implicitly, so just calling that lgtm.

[Robert Sesek, 2015-02-25 23:35:03]

LGTM

On 2015/02/25 23:11:20, David Benjamin wrote:
> In fact they RemoveSource /and/ Invalidate. That's probably as close to a
> canonical answer as we're going to get. Invalidate is documented as removing
> itself implicitly, so just calling that lgtm.

Yes, CFRunLoopSourceInvalidate() should be sufficient. It copies all the run
loop modes and removes the source from each, after marking the source as
invalid.

[Elly Fong-Jones, 2015-02-26 14:55:30]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2015-02-26 14:56:06]

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

[commit-bot: I haz the power, 2015-02-26 15:28:53]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-02-26 15:29:39]

Patchset 1 (id:??) landed as
https://crrev.com/824907ba81b615ccbbc35c51bc9b937f3d99a5f2
Cr-Commit-Position: refs/heads/master@{#318233}