NetworkService: Destroy URLLoaders when a NetworkContext is destroyed.

NetworkService:  Destroy URLLoaders when a NetworkContext is destroyed.

URLRequests can't outlast the URLRequestContext that spawned them, so
the current code can cause crashes on NetworkContext destruction.

BUG=735995
Review-Url: https://codereview.chromium.org/2951293002
Cr-Commit-Position: refs/heads/master@{#481760}
Committed: https://chromium.googlesource.com/chromium/src/+/28bcb85f5c7f1e56f57a0b26f9a878196adedb3f

[mmenke, 2017-06-22 17:12:01]

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

[commit-bot: I haz the power, 2017-06-22 17:12:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2017-06-22 17:31:41]

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

[commit-bot: I haz the power, 2017-06-22 17:32:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2017-06-22 17:58:49]

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

[mmenke, 2017-06-22 17:58:50]

https://codereview.chromium.org/2951293002/diff/20001/content/network/url_loa...
File content/network/url_loader_unittest.cc (right):

https://codereview.chromium.org/2951293002/diff/20001/content/network/url_loa...
content/network/url_loader_unittest.cc:88: TRAFFIC_ANNOTATION_FOR_TESTS);
The fact that we create these on the stack seems weird to me, since if we
deleted the client before these, we'd run into crashes, since the client
normally owns them.  I'm thinking I'll switch to creating most of these through
the NetworkServiceURLLoaderFactoryImpl, and making sure they're cleaned up in a
followup CL.

[mmenke, 2017-06-22 18:00:26]

The CQ bit was unchecked by mmenke@chromium.org

[jam, 2017-06-22 20:53:10]

what if NetworkContext's destructor is calling Cleanup, which is modifying the
url_loaders_ that's being iterated.

There's a comment in ~NetworkContext(): "The loaders can be deregistering
themselves in Cleanup() so iterate over a copy." but the code isn't actually
doing that (oops). so probably change that to be a copy? with that, lgtm

[mmenke, 2017-06-22 20:58:10]

On 2017/06/22 20:53:10, jam wrote:
> what if NetworkContext's destructor is calling Cleanup, which is modifying the
> url_loaders_ that's being iterated.
> 
> There's a comment in ~NetworkContext(): "The loaders can be deregistering
> themselves in Cleanup() so iterate over a copy." but the code isn't actually
> doing that (oops). so probably change that to be a copy? with that, lgtm

Isn't that what in_shutdown_ is there to prevent?  Or am I missing something?

[mmenke, 2017-06-22 20:59:53]

On 2017/06/22 20:58:10, mmenke wrote:
> On 2017/06/22 20:53:10, jam wrote:
> > what if NetworkContext's destructor is calling Cleanup, which is modifying
the
> > url_loaders_ that's being iterated.
> > 
> > There's a comment in ~NetworkContext(): "The loaders can be deregistering
> > themselves in Cleanup() so iterate over a copy." but the code isn't actually
> > doing that (oops). so probably change that to be a copy? with that, lgtm
> 
> Isn't that what in_shutdown_ is there to prevent?  Or am I missing something?

I'll go ahead and switch to using a copy - it's less efficient, but I like the
approach a bit better.

[mmenke, 2017-06-22 21:02:57]

On 2017/06/22 20:59:53, mmenke wrote:
> On 2017/06/22 20:58:10, mmenke wrote:
> > On 2017/06/22 20:53:10, jam wrote:
> > > what if NetworkContext's destructor is calling Cleanup, which is modifying
> the
> > > url_loaders_ that's being iterated.
> > > 
> > > There's a comment in ~NetworkContext(): "The loaders can be deregistering
> > > themselves in Cleanup() so iterate over a copy." but the code isn't
actually
> > > doing that (oops). so probably change that to be a copy? with that, lgtm
> > 
> > Isn't that what in_shutdown_ is there to prevent?  Or am I missing
something?
> 
> I'll go ahead and switch to using a copy - it's less efficient, but I like the
> approach a bit better.

Actually, don't need a copy - can just repeatedly remove the first object.

[mmenke, 2017-06-22 21:07:55]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-06-22 21:07:56]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2951293002/#ps40001
(title: "Response to comment, change why destruction is safe")

[commit-bot: I haz the power, 2017-06-22 21:08:28]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-06-22 21:31:25]

On 2017/06/22 20:58:10, mmenke wrote:
> On 2017/06/22 20:53:10, jam wrote:
> > what if NetworkContext's destructor is calling Cleanup, which is modifying
the
> > url_loaders_ that's being iterated.
> > 
> > There's a comment in ~NetworkContext(): "The loaders can be deregistering
> > themselves in Cleanup() so iterate over a copy." but the code isn't actually
> > doing that (oops). so probably change that to be a copy? with that, lgtm
> 
> Isn't that what in_shutdown_ is there to prevent?  Or am I missing something?

my bad, I forgot about that. either patchset 2 or 3 are fine with me, whatever
you prefer.

[mmenke, 2017-06-22 21:34:51]

On 2017/06/22 21:31:25, jam wrote:
> On 2017/06/22 20:58:10, mmenke wrote:
> > On 2017/06/22 20:53:10, jam wrote:
> > > what if NetworkContext's destructor is calling Cleanup, which is modifying
> the
> > > url_loaders_ that's being iterated.
> > > 
> > > There's a comment in ~NetworkContext(): "The loaders can be deregistering
> > > themselves in Cleanup() so iterate over a copy." but the code isn't
actually
> > > doing that (oops). so probably change that to be a copy? with that, lgtm
> > 
> > Isn't that what in_shutdown_ is there to prevent?  Or am I missing
something?
> 
> my bad, I forgot about that. either patchset 2 or 3 are fine with me, whatever
> you prefer.

Not a problem.  I'm going with 3 (Less code, and I prefer not to have a variable
just for this state, if we can avoid it)

[commit-bot: I haz the power, 2017-06-23 00:00:52]

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

[commit-bot: I haz the power, 2017-06-23 00:00:53]

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

[mmenke, 2017-06-23 00:01:40]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2017-06-23 00:02:13]

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, 2017-06-23 01:14:42]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498176100538870,
"parent_rev": "8a6d55e0006ac6a5efa1cc985af2e3fdde42bb24", "commit_rev":
"28bcb85f5c7f1e56f57a0b26f9a878196adedb3f"}

[commit-bot: I haz the power, 2017-06-23 01:14:57]

Description was changed from

==========
NetworkService:  Destroy URLLoaders when a NetworkContext is destroyed.

URLRequests can't outlast the URLRequestContext that spawned them, so
the current code can cause crashes on NetworkContext destruction.

BUG=735995
==========

to

==========
NetworkService:  Destroy URLLoaders when a NetworkContext is destroyed.

URLRequests can't outlast the URLRequestContext that spawned them, so
the current code can cause crashes on NetworkContext destruction.

BUG=735995

Review-Url: https://codereview.chromium.org/2951293002
Cr-Commit-Position: refs/heads/master@{#481760}
Committed:
https://chromium.googlesource.com/chromium/src/+/28bcb85f5c7f1e56f57a0b26f9a8...
==========

[commit-bot: I haz the power, 2017-06-23 01:14:58]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/28bcb85f5c7f1e56f57a0b26f9a8...