Correct the order of destruction of ProfileImplIOData and ExtensionProcessManager. This was causi...

Correct the order of destruction of ProfileImplIOData and ExtensionProcessManager. This was causing obscure shutdown crashes. See bug for description of crashes.

Also add a DCHECK to help enforce that all RenderProcessHost instances must have been shutdown before ProfileImplIOData->ShutdownOnUIThread() is called.

BUG=91398
TEST=Chrome does not crash when extensions with background pages make resource requests during shutdown.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111762

[willchan no longer on Chromium, 2011-11-17 20:11:32]

LGTM

http://codereview.chromium.org/8591024/diff/6/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/8591024/diff/6/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_impl.h:213: // io_data_ needs to outlive
extension_process_manager_.
|io_data_| needs to outlive a lot of things. Rather than enumerate them here,
I'd rather we do the vice versa. At |extension_process_manager_| note that it
must be destroyed after |io_data|.

http://codereview.chromium.org/8591024/diff/6/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/8591024/diff/6/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_io_data.cc:545: DCHECK_EQ(0,
RenderProcessHost::AllHostsCount());
I'd love to upgrade this to a CHECK so we can catch regressions, since if this
is wrong, we're likely to crash later. That said, it's not clear to me that this
fix will fix ALL bugs, so it's probably best to leave it as a DCHECK_EQ right
now, but add a comment to upgrade it to a CHECK later, and do it maybe a week or
two later after we get dev channel results.

[willchan no longer on Chromium, 2011-11-17 20:12:00]

+mpcomplete since he's the most likely to shuffle around any extension stuff in
ProfileImpl.

[robertshield, 2011-11-17 21:00:42]

http://codereview.chromium.org/8591024/diff/6/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/8591024/diff/6/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_impl.h:213: // io_data_ needs to outlive
extension_process_manager_.
On 2011/11/17 20:11:32, willchan wrote:
> |io_data_| needs to outlive a lot of things. Rather than enumerate them here,
> I'd rather we do the vice versa. At |extension_process_manager_| note that it
> must be destroyed after |io_data|.

Done.

http://codereview.chromium.org/8591024/diff/6/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/8591024/diff/6/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_io_data.cc:545: DCHECK_EQ(0,
RenderProcessHost::AllHostsCount());
On 2011/11/17 20:11:32, willchan wrote:
> I'd love to upgrade this to a CHECK so we can catch regressions, since if this
> is wrong, we're likely to crash later. That said, it's not clear to me that
this
> fix will fix ALL bugs, so it's probably best to leave it as a DCHECK_EQ right
> now, but add a comment to upgrade it to a CHECK later, and do it maybe a week
or
> two later after we get dev channel results.

Good idea, comment added.

[willchan no longer on Chromium, 2011-11-18 16:52:58]

Looks like this is breaking downloads. There's probably some dependency ordering
there. The ordering in ProfileImpl is so fragile =/

PS: I didn't doublecheck the OffTheRecordProfile, so you should probably
doublecheck that there's no bug there that needs to be fixed too.

On 2011/11/17 21:00:42, robertshield wrote:
>
http://codereview.chromium.org/8591024/diff/6/chrome/browser/profiles/profile...
> File chrome/browser/profiles/profile_impl.h (right):
> 
>
http://codereview.chromium.org/8591024/diff/6/chrome/browser/profiles/profile...
> chrome/browser/profiles/profile_impl.h:213: // io_data_ needs to outlive
> extension_process_manager_.
> On 2011/11/17 20:11:32, willchan wrote:
> > |io_data_| needs to outlive a lot of things. Rather than enumerate them
here,
> > I'd rather we do the vice versa. At |extension_process_manager_| note that
it
> > must be destroyed after |io_data|.
> 
> Done.
> 
>
http://codereview.chromium.org/8591024/diff/6/chrome/browser/profiles/profile...
> File chrome/browser/profiles/profile_io_data.cc (right):
> 
>
http://codereview.chromium.org/8591024/diff/6/chrome/browser/profiles/profile...
> chrome/browser/profiles/profile_io_data.cc:545: DCHECK_EQ(0,
> RenderProcessHost::AllHostsCount());
> On 2011/11/17 20:11:32, willchan wrote:
> > I'd love to upgrade this to a CHECK so we can catch regressions, since if
this
> > is wrong, we're likely to crash later. That said, it's not clear to me that
> this
> > fix will fix ALL bugs, so it's probably best to leave it as a DCHECK_EQ
right
> > now, but add a comment to upgrade it to a CHECK later, and do it maybe a
week
> or
> > two later after we get dev channel results.
> 
> Good idea, comment added.

[robertshield, 2011-11-25 15:04:43]

The download breakage was due to me not having made the corresponding change to
OffTheRecordProfile. That's made now, but I had to remove the DCHECK :(

Turns out that RPH count is not necessarily 0 at the point at which a given
ProfileIOData is destroyed, only when the last ProfileIOData is destroyed.
Ideally what's needed is a way of checking if all of the RPHs associated with
_the current profile_ are destroyed when a given ProfileIOData is torn down.
That looks like a fair bit of plumbing that I'd rather do separately to get this
fix in and see how it behaves on canary.

What say you?


On 2011/11/18 16:52:58, willchan wrote:
> Looks like this is breaking downloads. There's probably some dependency
ordering
> there. The ordering in ProfileImpl is so fragile =/
> 
> PS: I didn't doublecheck the OffTheRecordProfile, so you should probably
> doublecheck that there's no bug there that needs to be fixed too.
> 
> On 2011/11/17 21:00:42, robertshield wrote:
> >
>
http://codereview.chromium.org/8591024/diff/6/chrome/browser/profiles/profile...
> > File chrome/browser/profiles/profile_impl.h (right):
> > 
> >
>
http://codereview.chromium.org/8591024/diff/6/chrome/browser/profiles/profile...
> > chrome/browser/profiles/profile_impl.h:213: // io_data_ needs to outlive
> > extension_process_manager_.
> > On 2011/11/17 20:11:32, willchan wrote:
> > > |io_data_| needs to outlive a lot of things. Rather than enumerate them
> here,
> > > I'd rather we do the vice versa. At |extension_process_manager_| note that
> it
> > > must be destroyed after |io_data|.
> > 
> > Done.
> > 
> >
>
http://codereview.chromium.org/8591024/diff/6/chrome/browser/profiles/profile...
> > File chrome/browser/profiles/profile_io_data.cc (right):
> > 
> >
>
http://codereview.chromium.org/8591024/diff/6/chrome/browser/profiles/profile...
> > chrome/browser/profiles/profile_io_data.cc:545: DCHECK_EQ(0,
> > RenderProcessHost::AllHostsCount());
> > On 2011/11/17 20:11:32, willchan wrote:
> > > I'd love to upgrade this to a CHECK so we can catch regressions, since if
> this
> > > is wrong, we're likely to crash later. That said, it's not clear to me
that
> > this
> > > fix will fix ALL bugs, so it's probably best to leave it as a DCHECK_EQ
> right
> > > now, but add a comment to upgrade it to a CHECK later, and do it maybe a
> week
> > or
> > > two later after we get dev channel results.
> > 
> > Good idea, comment added.

[willchan no longer on Chromium, 2011-11-28 18:52:07]

lgtm