Always destruct url request context getter on io thread....

Spellchecker: Always destruct url request context getter on io thread.

To do this, we have to initiate downloads on the UI thread and don't hold onto a reference in the file thread.

BUG=27667
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=32129

[Evan Stade, 2009-11-14 00:34:34]

I broke this out from the other changelist because of deadlines.

[jam, 2009-11-14 00:40:44]

Can you coordinate with Eric?  He has a similar patch, although he also cleaned
up current users and update unit tests to avoid memory leaks.

[eroman, 2009-11-14 00:44:07]

This looks fine, but make sure to run with a memory checker -- there are some
unit-tests such as http_bridge_unittest which derive this, and may not have an
"IO" thread defined (so I imagine they would leak).

John is right, I was working on this same sort of change (but haven't updated it
with the latest code, or made it use the DeleteOnIOThread trait).
You can see http://codereview.chromium.org/332006 for that earlier draft.

[Evan Stade, 2009-11-14 02:15:27]

updated after conversation with Eric.

[jam, 2009-11-14 02:24:47]

http://codereview.chromium.org/387055/diff/3002/4003
File chrome/browser/spellcheck_host.h (right):

http://codereview.chromium.org/387055/diff/3002/4003#newcode114
Line 114: // it is only valid to use it on the UI thread.
Why?  I thought the whole point of URLRequestContextGetter is that you can hold
on to it on any thread, but only call it form the IO thread?

[Evan Stade, 2009-11-14 02:34:23]

http://codereview.chromium.org/387055/diff/3002/4003
File chrome/browser/spellcheck_host.h (right):

http://codereview.chromium.org/387055/diff/3002/4003#newcode114
Line 114: // it is only valid to use it on the UI thread.
On 2009/11/14 02:24:47, John Abd-El-Malek wrote:
> Why?  I thought the whole point of URLRequestContextGetter is that you can
hold
> on to it on any thread, but only call it form the IO thread?

Eric says there is the additional restriction that we can't let it fail to be
destroyed (on the io thread). So if we hold onto a reference that we don't
release till we are destroyed (on the file thread), we may force it to persist
too long.

[eroman, 2009-11-14 02:36:19]

Right.

Since the file thread is stopped AFTER the IO thread, and the release is
happening from the file thread, that means you may end up leaking since by the
time the release happens, the IO thread is already gone.

[jam, 2009-11-14 02:42:44]

On 2009/11/14 02:36:19, eroman wrote:
> Right.
> 
> Since the file thread is stopped AFTER the IO thread, and the release is
> happening from the file thread, that means you may end up leaking since by the
> time the release happens, the IO thread is already gone.

It seems fine to have leaks like this at shutdown, in order to simplify the
code, no?

[eroman, 2009-11-14 03:16:17]

Well in this case leaks can lead to other problems.

For example, since outstanding certificate verifier jobs, or outstanding host
resolver jobs are not cancelled, they may now complete after the IO thread is
torn down and try to post back to a destroyed IO loop, or otherwise access freed
memory.

Similarly, leaked objects in the context such as the cookie store, and disk
cache, will not run their destructors. This could cause their data to not get
flushed out to disk.

[Evan Stade, 2009-11-16 20:36:12]

so is that a lgtm?

[eroman, 2009-11-16 21:20:08]

lgtm

[darin (slow to review), 2009-11-16 22:08:37]

http://codereview.chromium.org/387055/diff/4008/4009
File chrome/browser/spellcheck_host.cc (right):

http://codereview.chromium.org/387055/diff/4008/4009#newcode220
Line 220: } else {
nit: no need for else after return