Make sure the "cancel leaked host resolver requests shutdown hack" gets run b...

Make sure the "cancel leaked host resolver requests shutdown hack" gets run before the message loop is destroyed.

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

[willchan no longer on Chromium, 2010-04-26 21:17:00]

http://codereview.chromium.org/1798001/diff/1/3
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/1798001/diff/1/3#newcode174
chrome/browser/io_thread.cc:174: void
IOThread::CleanUpAfterMessageLoopDestruction() {
You need to document why we do this.  It won't be clear why these two cleanup
steps are separated.  It'd be nice if there were a test that enforced it rather
than just documentation, but I don't necessarily see a good way to do that
offhand, so I don't expect you to do it (but it'd be nice if you thought of a
way!).

[willchan no longer on Chromium, 2010-04-26 21:17:09]

LGTM

On 2010/04/26 21:17:00, willchan wrote:
> http://codereview.chromium.org/1798001/diff/1/3
> File chrome/browser/io_thread.cc (right):
> 
> http://codereview.chromium.org/1798001/diff/1/3#newcode174
> chrome/browser/io_thread.cc:174: void
> IOThread::CleanUpAfterMessageLoopDestruction() {
> You need to document why we do this.  It won't be clear why these two cleanup
> steps are separated.  It'd be nice if there were a test that enforced it
rather
> than just documentation, but I don't necessarily see a good way to do that
> offhand, so I don't expect you to do it (but it'd be nice if you thought of a
> way!).

[eroman, 2010-04-26 21:54:46]

Done, I added a comment.

Regarding a test: I don't have a great solution, shutdown is just too darned
complicated!

I am somewhat satisfied by the fact that we hit this in chrome-bot reliability
tests, so I am leaning towards leaving that as the sole test and omitting an
explicit unit-test. I'll think about it some more though before giving up.

[eroman, 2010-04-26 22:37:57]

OK, I have re-worked this change.

Basically I restored it even closer to the original code, and added a one-off to
fix the original bug (39723) which I added CleanUpAfterMessageLoopDestruction().

I am a little scared about changing the shutdown code too much, since I want to
take this on 375.

I think the right fix is to address the TODO and make
CleanUpAfterMessageLoopDestruction() happen *prior* to deletion of the message
loop, but after the observers have been called. I had avoided this initially
since it requires changing message loop, but in light of this subtle bug it now
seems worthwhile.

[willchan no longer on Chromium, 2010-04-26 23:29:12]

I have to say that this looks much hackier.  So you're trying to defend against
further use of the ChromeNetLog in a destruction observer?  Do you know if this
happens?  I guess I'm fine with this as a one-off for the 375 branch, but
wouldn't want this type of solution to stick around for long.

If you plan to fix this in the future, then I'm fine with it as is.  Maybe track
it with a bug.  Up to you though.

LGTM.

On 2010/04/26 22:37:57, eroman wrote:
> OK, I have re-worked this change.
> 
> Basically I restored it even closer to the original code, and added a one-off
to
> fix the original bug (39723) which I added
CleanUpAfterMessageLoopDestruction().
> 
> I am a little scared about changing the shutdown code too much, since I want
to
> take this on 375.
> 
> I think the right fix is to address the TODO and make
> CleanUpAfterMessageLoopDestruction() happen *prior* to deletion of the message
> loop, but after the observers have been called. I had avoided this initially
> since it requires changing message loop, but in light of this subtle bug it
now
> seems worthwhile.

[eroman, 2010-04-26 23:53:08]

> I have to say that this looks much hackier.  So you're trying to defend
against
>further use of the ChromeNetLog in a destruction observer?

Correct.

> Do you know if this happens? 

Yes it happens  (see bug 39723).

> I guess I'm fine with this as a one-off for the 375 branch, but
> wouldn't want this type of solution to stick around for long.

I am unhappy with this solution as well. Plan to do the long-term fix described
in the TODO.