Shut down Profile's URLRequestContextGetters safely.

Shut down Profile's URLRequestContextGetters safely.

Any URLFetchers using one of a Profile's URLRequestContextGetters
will now return an error on Profile shutdown, as opposed to causing
crashes in AssertNoURLRequests.

BUG=419095
Committed: https://crrev.com/2d09f7a2038dca3fa59c1d6ee4a15a98d9ff98ab
Cr-Commit-Position: refs/heads/master@{#329916}

[mmenke, 2015-05-12 20:45:09]

Patchset #2 (id:20001) has been deleted

[mmenke, 2015-05-12 20:45:41]

Patchset #3 (id:60001) has been deleted

[mmenke, 2015-05-12 20:46:01]

Patchset #3 (id:80001) has been deleted

[mmenke, 2015-05-13 15:18:54]

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

[mmenke, 2015-05-13 15:18:54]

I'd like better tests for main profile shutdown, but since we don't really
support multiple main profiles...  (The test fixture does have code to just
create a random bonus profile, which I may be able to use, but the idea makes me
shudder).

I'm not currently testing the isolated app contexts.  I suppose I could, but
they're a little weird, require temp directories (the teardown order of which
I'm not familiar with) and nothing accesses them directly through the profile. 
Could certainly work through all that, but not sure it's worth the effort.

[davidben, 2015-05-14 17:09:27]

lgtm with minor comments.

https://codereview.chromium.org/1124333010/diff/140001/chrome/browser/net/chr...
File chrome/browser/net/chrome_url_request_context_getter.h (right):

https://codereview.chromium.org/1124333010/diff/140001/chrome/browser/net/chr...
chrome/browser/net/chrome_url_request_context_getter.h:84: void
NotifyContextShuttingDown();
Maybe keep this called Invalidate or rename it to something else? I think having
a non-virtual override is probably better avoided. (They also do subtly
different things anyway. The base class's one is "By the way, I just made the
context start returning null. Please tell the observers". The subclass is "By
the way, that factory I constructed you with needs to be destroyed because it
references a dying ProfileIOData. Please deal."

https://codereview.chromium.org/1124333010/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_browsertest.cc (right):

https://codereview.chromium.org/1124333010/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_browsertest.cc:56: // to never complete.
This comment is a little confusing since the method is called WaitForCompletion.
By this you mean that the request may, instead of never completing, might get
canceled instead? Perhaps just write

"[...] and the ability to wait until a request completes or is canceled."

and drop the last sentence.

https://codereview.chromium.org/1124333010/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_browsertest.cc:221:
url_fetcher_delegate2.WaitForCompletion();
[Just to confirm, without this line, the test still passes, but we're not
actually testing the shutdown path?]

[mmenke, 2015-05-14 19:23:52]

https://codereview.chromium.org/1124333010/diff/140001/chrome/browser/net/chr...
File chrome/browser/net/chrome_url_request_context_getter.h (right):

https://codereview.chromium.org/1124333010/diff/140001/chrome/browser/net/chr...
chrome/browser/net/chrome_url_request_context_getter.h:84: void
NotifyContextShuttingDown();
On 2015/05/14 17:09:26, David Benjamin wrote:
> Maybe keep this called Invalidate or rename it to something else? I think
having
> a non-virtual override is probably better avoided. (They also do subtly
> different things anyway. The base class's one is "By the way, I just made the
> context start returning null. Please tell the observers". The subclass is "By
> the way, that factory I constructed you with needs to be destroyed because it
> references a dying ProfileIOData. Please deal."

Done - was on the fence about naming, happy to defer to your opinion.  Switched
back to Invalidate.

https://codereview.chromium.org/1124333010/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_browsertest.cc (right):

https://codereview.chromium.org/1124333010/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_browsertest.cc:56: // to never complete.
On 2015/05/14 17:09:27, David Benjamin wrote:
> This comment is a little confusing since the method is called
WaitForCompletion.
> By this you mean that the request may, instead of never completing, might get
> canceled instead? Perhaps just write
> 
> "[...] and the ability to wait until a request completes or is canceled."
> 
> and drop the last sentence.

Done - I think the last sentence is a bit unexpected, given the constructor,
though, so I added a comment to the constructor.

"|expected_request_status| is the expected final URLRequestStatus of the
fetcher, if it completes."

Seems a bit clearer that way.

https://codereview.chromium.org/1124333010/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_browsertest.cc:221:
url_fetcher_delegate2.WaitForCompletion();
On 2015/05/14 17:09:27, David Benjamin wrote:
> [Just to confirm, without this line, the test still passes, but we're not
> actually testing the shutdown path?]

Discussed this offline, clarified comment.

[mmenke, 2015-05-14 19:23:58]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2015-05-14 19:24:33]

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

[commit-bot: I haz the power, 2015-05-14 20:24:04]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2015-05-14 20:24:55]

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