Helper functions for tracking stale TabContentsDelegate's.

Helper functions for tracking stale TabContentsDelegate's.

BUG=85247
TEST=None


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

[cbentzel, 2011-06-23 18:15:22]

I'm wondering if we want something like this while tracking down crash causes.

[jam, 2011-06-23 18:18:42]

I defer to Scott.

[sky, 2011-06-23 18:20:08]

The bug is that TabContents delegate has been deleted. Unless I'm
missing something I don't think this would help track that down.

I'm going to add an id to each delegate so that we can know the type
of the last delegate. I'm also going to make browser CHECK that the
tabstrip is empty.

  -Scott

On Thu, Jun 23, 2011 at 11:15 AM,  <cbentzel@chromium.org> wrote:
> Reviewers: sky, tburkard, John Abd-El-Malek,
>
> Message:
> I'm wondering if we want something like this while tracking down crash
> causes.
>
> Description:
> Helper functions for tracking stale TabContentsDelegate's.
>
> BUG=85247
> TEST=None
>
>
> Please review this at http://codereview.chromium.org/7244009/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M content/browser/tab_contents/tab_contents.h
>  M content/browser/tab_contents/tab_contents.cc
>  M content/browser/tab_contents/tab_contents_delegate.h
>  M content/browser/tab_contents/tab_contents_delegate.cc
>
>
> Index: content/browser/tab_contents/tab_contents.cc
> diff --git a/content/browser/tab_contents/tab_contents.cc
> b/content/browser/tab_contents/tab_contents.cc
> index
>
cc805054f208b971cb56e8f7554f03984c3bdc26..43a46f78fde0f7999694d8b0f522dd349e986aed
> 100644
> --- a/content/browser/tab_contents/tab_contents.cc
> +++ b/content/browser/tab_contents/tab_contents.cc
> @@ -623,6 +623,19 @@ void TabContents::AddNewContents(TabContents*
> new_contents,
>                             user_gesture);
>  }
>
> +void TabContents::set_delegate(TabContentsDelegate* delegate) {
> +  if (delegate == delegate_)
> +    return;
> +
> +  if (delegate_)
> +    delegate_->Detach(this);
> +
> +  delegate_ = delegate;
> +
> +  if (delegate_)
> +    delegate_->Attach(this);
> +}
> +
>  gfx::NativeView TabContents::GetContentNativeView() const {
>   return view_->GetContentNativeView();
>  }
> Index: content/browser/tab_contents/tab_contents.h
> diff --git a/content/browser/tab_contents/tab_contents.h
> b/content/browser/tab_contents/tab_contents.h
> index
>
02814dd8cec6d8b915ae3ff45829953201c6450d..861a6a573e86e1cc43f99c334e72bf9cf47458f2
> 100644
> --- a/content/browser/tab_contents/tab_contents.h
> +++ b/content/browser/tab_contents/tab_contents.h
> @@ -98,7 +98,7 @@ class TabContents : public PageNavigator,
>   PropertyBag* property_bag() { return &property_bag_; }
>
>   TabContentsDelegate* delegate() const { return delegate_; }
> -  void set_delegate(TabContentsDelegate* d) { delegate_ = d; }
> +  void set_delegate(TabContentsDelegate* delegate);
>
>   // Gets the controller for this tab contents.
>   NavigationController& controller() { return controller_; }
> Index: content/browser/tab_contents/tab_contents_delegate.cc
> diff --git a/content/browser/tab_contents/tab_contents_delegate.cc
> b/content/browser/tab_contents/tab_contents_delegate.cc
> index
>
df709a7f3549d644f105f444c44fa39572fa83ec..e10aa4597fef82946d51a2ac0e9e2ada0fb535c9
> 100644
> --- a/content/browser/tab_contents/tab_contents_delegate.cc
> +++ b/content/browser/tab_contents/tab_contents_delegate.cc
> @@ -237,5 +237,16 @@ TabContentsDelegate::GetJavaScriptDialogCreator() {
>   return JavaScriptDialogCreatorStub::GetInstance();
>  }
>
> +void TabContentsDelegate::Attach(TabContents* tab_contents) {
> +  CHECK(attached_contents_.find(tab_contents) == attached_contents_.end());
> +  attached_contents_.insert(tab_contents);
> +}
> +
> +void TabContentsDelegate::Detach(TabContents* tab_contents) {
> +  CHECK(attached_contents_.find(tab_contents) != attached_contents_.end());
> +  attached_contents_.erase(tab_contents);
> +}
> +
>  TabContentsDelegate::~TabContentsDelegate() {
> +  CHECK(attached_contents_.empty());
>  }
> Index: content/browser/tab_contents/tab_contents_delegate.h
> diff --git a/content/browser/tab_contents/tab_contents_delegate.h
> b/content/browser/tab_contents/tab_contents_delegate.h
> index
>
0645128b9add09ea15037d25547eb3260f806efb..d41b00bd8a392f1f78150501885a8de0e95f9b96
> 100644
> --- a/content/browser/tab_contents/tab_contents_delegate.h
> +++ b/content/browser/tab_contents/tab_contents_delegate.h
> @@ -6,6 +6,7 @@
>  #define CONTENT_BROWSER_TAB_CONTENTS_TAB_CONTENTS_DELEGATE_H_
>  #pragma once
>
> +#include <set>
>  #include <string>
>
>  #include "base/basictypes.h"
> @@ -50,6 +51,9 @@ class TabContentsDelegate {
>     virtual ~MainFrameCommitDetails() {}
>   };
>
> +  void Attach(TabContents* source);
> +  void Detach(TabContents* source);
> +
>   // Opens a new URL inside the passed in TabContents (if source is 0 open
>   // in the current front-most tab), unless |disposition| indicates the url
>   // should be opened in a new tab or window.
> @@ -310,6 +314,9 @@ class TabContentsDelegate {
>
>  protected:
>   virtual ~TabContentsDelegate();
> +
> + private:
> +  std::set<TabContents*> attached_contents_;
>  };
>
>  #endif  // CONTENT_BROWSER_TAB_CONTENTS_TAB_CONTENTS_DELEGATE_H_
>
>
>

[cbentzel, 2011-06-23 18:22:19]

This will CHECK in the TabContentsDelegate destructor if any
TabContents have it as the current delegate pointer. So it will let us
know which TabContentsDelegate was the cause in the use-after-free
case in the callstack.

On Thu, Jun 23, 2011 at 2:20 PM, Scott Violet <sky@chromium.org> wrote:
> The bug is that TabContents delegate has been deleted. Unless I'm
> missing something I don't think this would help track that down.
>
> I'm going to add an id to each delegate so that we can know the type
> of the last delegate. I'm also going to make browser CHECK that the
> tabstrip is empty.
>
>  -Scott
>
> On Thu, Jun 23, 2011 at 11:15 AM,  <cbentzel@chromium.org> wrote:
>> Reviewers: sky, tburkard, John Abd-El-Malek,
>>
>> Message:
>> I'm wondering if we want something like this while tracking down crash
>> causes.
>>
>> Description:
>> Helper functions for tracking stale TabContentsDelegate's.
>>
>> BUG=85247
>> TEST=None
>>
>>
>> Please review this at http://codereview.chromium.org/7244009/
>>
>> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>>
>> Affected files:
>>  M content/browser/tab_contents/tab_contents.h
>>  M content/browser/tab_contents/tab_contents.cc
>>  M content/browser/tab_contents/tab_contents_delegate.h
>>  M content/browser/tab_contents/tab_contents_delegate.cc
>>
>>
>> Index: content/browser/tab_contents/tab_contents.cc
>> diff --git a/content/browser/tab_contents/tab_contents.cc
>> b/content/browser/tab_contents/tab_contents.cc
>> index
>>
cc805054f208b971cb56e8f7554f03984c3bdc26..43a46f78fde0f7999694d8b0f522dd349e986aed
>> 100644
>> --- a/content/browser/tab_contents/tab_contents.cc
>> +++ b/content/browser/tab_contents/tab_contents.cc
>> @@ -623,6 +623,19 @@ void TabContents::AddNewContents(TabContents*
>> new_contents,
>>                             user_gesture);
>>  }
>>
>> +void TabContents::set_delegate(TabContentsDelegate* delegate) {
>> +  if (delegate == delegate_)
>> +    return;
>> +
>> +  if (delegate_)
>> +    delegate_->Detach(this);
>> +
>> +  delegate_ = delegate;
>> +
>> +  if (delegate_)
>> +    delegate_->Attach(this);
>> +}
>> +
>>  gfx::NativeView TabContents::GetContentNativeView() const {
>>   return view_->GetContentNativeView();
>>  }
>> Index: content/browser/tab_contents/tab_contents.h
>> diff --git a/content/browser/tab_contents/tab_contents.h
>> b/content/browser/tab_contents/tab_contents.h
>> index
>>
02814dd8cec6d8b915ae3ff45829953201c6450d..861a6a573e86e1cc43f99c334e72bf9cf47458f2
>> 100644
>> --- a/content/browser/tab_contents/tab_contents.h
>> +++ b/content/browser/tab_contents/tab_contents.h
>> @@ -98,7 +98,7 @@ class TabContents : public PageNavigator,
>>   PropertyBag* property_bag() { return &property_bag_; }
>>
>>   TabContentsDelegate* delegate() const { return delegate_; }
>> -  void set_delegate(TabContentsDelegate* d) { delegate_ = d; }
>> +  void set_delegate(TabContentsDelegate* delegate);
>>
>>   // Gets the controller for this tab contents.
>>   NavigationController& controller() { return controller_; }
>> Index: content/browser/tab_contents/tab_contents_delegate.cc
>> diff --git a/content/browser/tab_contents/tab_contents_delegate.cc
>> b/content/browser/tab_contents/tab_contents_delegate.cc
>> index
>>
df709a7f3549d644f105f444c44fa39572fa83ec..e10aa4597fef82946d51a2ac0e9e2ada0fb535c9
>> 100644
>> --- a/content/browser/tab_contents/tab_contents_delegate.cc
>> +++ b/content/browser/tab_contents/tab_contents_delegate.cc
>> @@ -237,5 +237,16 @@ TabContentsDelegate::GetJavaScriptDialogCreator() {
>>   return JavaScriptDialogCreatorStub::GetInstance();
>>  }
>>
>> +void TabContentsDelegate::Attach(TabContents* tab_contents) {
>> +  CHECK(attached_contents_.find(tab_contents) == attached_contents_.end());
>> +  attached_contents_.insert(tab_contents);
>> +}
>> +
>> +void TabContentsDelegate::Detach(TabContents* tab_contents) {
>> +  CHECK(attached_contents_.find(tab_contents) != attached_contents_.end());
>> +  attached_contents_.erase(tab_contents);
>> +}
>> +
>>  TabContentsDelegate::~TabContentsDelegate() {
>> +  CHECK(attached_contents_.empty());
>>  }
>> Index: content/browser/tab_contents/tab_contents_delegate.h
>> diff --git a/content/browser/tab_contents/tab_contents_delegate.h
>> b/content/browser/tab_contents/tab_contents_delegate.h
>> index
>>
0645128b9add09ea15037d25547eb3260f806efb..d41b00bd8a392f1f78150501885a8de0e95f9b96
>> 100644
>> --- a/content/browser/tab_contents/tab_contents_delegate.h
>> +++ b/content/browser/tab_contents/tab_contents_delegate.h
>> @@ -6,6 +6,7 @@
>>  #define CONTENT_BROWSER_TAB_CONTENTS_TAB_CONTENTS_DELEGATE_H_
>>  #pragma once
>>
>> +#include <set>
>>  #include <string>
>>
>>  #include "base/basictypes.h"
>> @@ -50,6 +51,9 @@ class TabContentsDelegate {
>>     virtual ~MainFrameCommitDetails() {}
>>   };
>>
>> +  void Attach(TabContents* source);
>> +  void Detach(TabContents* source);
>> +
>>   // Opens a new URL inside the passed in TabContents (if source is 0 open
>>   // in the current front-most tab), unless |disposition| indicates the url
>>   // should be opened in a new tab or window.
>> @@ -310,6 +314,9 @@ class TabContentsDelegate {
>>
>>  protected:
>>   virtual ~TabContentsDelegate();
>> +
>> + private:
>> +  std::set<TabContents*> attached_contents_;
>>  };
>>
>>  #endif  // CONTENT_BROWSER_TAB_CONTENTS_TAB_CONTENTS_DELEGATE_H_
>>
>>
>>
>

[cbentzel, 2011-06-23 18:24:37]

For example, without your change I get

	base::debug::StackTrace::StackTrace() [0x7fb262a3cffe]
	logging::LogMessage::~LogMessage() [0x7fb262a6118e]
	TabContentsDelegate::~TabContentsDelegate() [0x7fb264ce3246]
	prerender::PrerenderContents::TabContentsDelegateImpl::~TabContentsDelegateImpl()
[0x7fb263915734]
	scoped_ptr<>::~scoped_ptr() [0x7fb263915783]
	prerender::PrerenderContents::~PrerenderContents() [0x7fb2639139b7]
	prerender::PrerenderManager::DeletePendingDeleteEntries() [0x7fb2633c48f0]
	prerender::PrerenderManager::AddPreload() [0x7fb2633c2ab0]
	prerender::HandleTag() [0x7fb2633c1dfd]
	DispatchToFunction<>() [0x7fb26342ac98]
	RunnableFunction<>::Run() [0x7fb26342ac10]
	(anonymous namespace)::TaskClosureAdapter::Run() [0x7fb262a6420d]
	base::internal::Invoker1<>::DoInvoke() [0x7fb262a67e06]
	base::Callback<>::Run() [0x7fb262a3fb89]
	MessageLoop::RunTask() [0x7fb262a66bd7]
	MessageLoop::DeferOrRunPendingTask() [0x7fb262a66d0d]
	MessageLoop::DoWork() [0x7fb262a67523]
	base::MessagePumpForUI::RunWithDispatcher() [0x7fb262a1a2c7]
	base::MessagePumpForUI::Run() [0x7fb262a1a77e]
	MessageLoop::RunInternal() [0x7fb262a669cb]
	MessageLoop::RunHandler() [0x7fb262a6687e]
	MessageLoopForUI::Run() [0x7fb262a67bbe]
	(anonymous namespace)::RunUIMessageLoop() [0x7fb26312a0b0]
	BrowserMain() [0x7fb26312d150]
	(anonymous namespace)::RunNamedProcessTypeMain() [0x7fb263087725]
	ChromeMain [0x7fb263088164]
	main [0x7fb263088da4]
	0x7fb25732bc4d
	0x7fb2630867e9

The concern is that we may have cases where we destroy the
TabContentsDelegate and immediately do a set_delegate(NULL) on the
TabContents that it references - that's fairly safe, and would
incorrectly trigger this check.


On Thu, Jun 23, 2011 at 2:22 PM, Chris Bentzel <cbentzel@chromium.org> wrote:
> This will CHECK in the TabContentsDelegate destructor if any
> TabContents have it as the current delegate pointer. So it will let us
> know which TabContentsDelegate was the cause in the use-after-free
> case in the callstack.
>
> On Thu, Jun 23, 2011 at 2:20 PM, Scott Violet <sky@chromium.org> wrote:
>> The bug is that TabContents delegate has been deleted. Unless I'm
>> missing something I don't think this would help track that down.
>>
>> I'm going to add an id to each delegate so that we can know the type
>> of the last delegate. I'm also going to make browser CHECK that the
>> tabstrip is empty.
>>
>>  -Scott
>>
>> On Thu, Jun 23, 2011 at 11:15 AM,  <cbentzel@chromium.org> wrote:
>>> Reviewers: sky, tburkard, John Abd-El-Malek,
>>>
>>> Message:
>>> I'm wondering if we want something like this while tracking down crash
>>> causes.
>>>
>>> Description:
>>> Helper functions for tracking stale TabContentsDelegate's.
>>>
>>> BUG=85247
>>> TEST=None
>>>
>>>
>>> Please review this at http://codereview.chromium.org/7244009/
>>>
>>> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>>>
>>> Affected files:
>>>  M content/browser/tab_contents/tab_contents.h
>>>  M content/browser/tab_contents/tab_contents.cc
>>>  M content/browser/tab_contents/tab_contents_delegate.h
>>>  M content/browser/tab_contents/tab_contents_delegate.cc
>>>
>>>
>>> Index: content/browser/tab_contents/tab_contents.cc
>>> diff --git a/content/browser/tab_contents/tab_contents.cc
>>> b/content/browser/tab_contents/tab_contents.cc
>>> index
>>>
cc805054f208b971cb56e8f7554f03984c3bdc26..43a46f78fde0f7999694d8b0f522dd349e986aed
>>> 100644
>>> --- a/content/browser/tab_contents/tab_contents.cc
>>> +++ b/content/browser/tab_contents/tab_contents.cc
>>> @@ -623,6 +623,19 @@ void TabContents::AddNewContents(TabContents*
>>> new_contents,
>>>                             user_gesture);
>>>  }
>>>
>>> +void TabContents::set_delegate(TabContentsDelegate* delegate) {
>>> +  if (delegate == delegate_)
>>> +    return;
>>> +
>>> +  if (delegate_)
>>> +    delegate_->Detach(this);
>>> +
>>> +  delegate_ = delegate;
>>> +
>>> +  if (delegate_)
>>> +    delegate_->Attach(this);
>>> +}
>>> +
>>>  gfx::NativeView TabContents::GetContentNativeView() const {
>>>   return view_->GetContentNativeView();
>>>  }
>>> Index: content/browser/tab_contents/tab_contents.h
>>> diff --git a/content/browser/tab_contents/tab_contents.h
>>> b/content/browser/tab_contents/tab_contents.h
>>> index
>>>
02814dd8cec6d8b915ae3ff45829953201c6450d..861a6a573e86e1cc43f99c334e72bf9cf47458f2
>>> 100644
>>> --- a/content/browser/tab_contents/tab_contents.h
>>> +++ b/content/browser/tab_contents/tab_contents.h
>>> @@ -98,7 +98,7 @@ class TabContents : public PageNavigator,
>>>   PropertyBag* property_bag() { return &property_bag_; }
>>>
>>>   TabContentsDelegate* delegate() const { return delegate_; }
>>> -  void set_delegate(TabContentsDelegate* d) { delegate_ = d; }
>>> +  void set_delegate(TabContentsDelegate* delegate);
>>>
>>>   // Gets the controller for this tab contents.
>>>   NavigationController& controller() { return controller_; }
>>> Index: content/browser/tab_contents/tab_contents_delegate.cc
>>> diff --git a/content/browser/tab_contents/tab_contents_delegate.cc
>>> b/content/browser/tab_contents/tab_contents_delegate.cc
>>> index
>>>
df709a7f3549d644f105f444c44fa39572fa83ec..e10aa4597fef82946d51a2ac0e9e2ada0fb535c9
>>> 100644
>>> --- a/content/browser/tab_contents/tab_contents_delegate.cc
>>> +++ b/content/browser/tab_contents/tab_contents_delegate.cc
>>> @@ -237,5 +237,16 @@ TabContentsDelegate::GetJavaScriptDialogCreator() {
>>>   return JavaScriptDialogCreatorStub::GetInstance();
>>>  }
>>>
>>> +void TabContentsDelegate::Attach(TabContents* tab_contents) {
>>> +  CHECK(attached_contents_.find(tab_contents) == attached_contents_.end());
>>> +  attached_contents_.insert(tab_contents);
>>> +}
>>> +
>>> +void TabContentsDelegate::Detach(TabContents* tab_contents) {
>>> +  CHECK(attached_contents_.find(tab_contents) != attached_contents_.end());
>>> +  attached_contents_.erase(tab_contents);
>>> +}
>>> +
>>>  TabContentsDelegate::~TabContentsDelegate() {
>>> +  CHECK(attached_contents_.empty());
>>>  }
>>> Index: content/browser/tab_contents/tab_contents_delegate.h
>>> diff --git a/content/browser/tab_contents/tab_contents_delegate.h
>>> b/content/browser/tab_contents/tab_contents_delegate.h
>>> index
>>>
0645128b9add09ea15037d25547eb3260f806efb..d41b00bd8a392f1f78150501885a8de0e95f9b96
>>> 100644
>>> --- a/content/browser/tab_contents/tab_contents_delegate.h
>>> +++ b/content/browser/tab_contents/tab_contents_delegate.h
>>> @@ -6,6 +6,7 @@
>>>  #define CONTENT_BROWSER_TAB_CONTENTS_TAB_CONTENTS_DELEGATE_H_
>>>  #pragma once
>>>
>>> +#include <set>
>>>  #include <string>
>>>
>>>  #include "base/basictypes.h"
>>> @@ -50,6 +51,9 @@ class TabContentsDelegate {
>>>     virtual ~MainFrameCommitDetails() {}
>>>   };
>>>
>>> +  void Attach(TabContents* source);
>>> +  void Detach(TabContents* source);
>>> +
>>>   // Opens a new URL inside the passed in TabContents (if source is 0 open
>>>   // in the current front-most tab), unless |disposition| indicates the url
>>>   // should be opened in a new tab or window.
>>> @@ -310,6 +314,9 @@ class TabContentsDelegate {
>>>
>>>  protected:
>>>   virtual ~TabContentsDelegate();
>>> +
>>> + private:
>>> +  std::set<TabContents*> attached_contents_;
>>>  };
>>>
>>>  #endif  // CONTENT_BROWSER_TAB_CONTENTS_TAB_CONTENTS_DELEGATE_H_
>>>
>>>
>>>
>>
>

[sky, 2011-06-23 18:27:32]

You're right, this is better.

You need to Detach from ~TabContents too though.

http://codereview.chromium.org/7244009/diff/1/content/browser/tab_contents/ta...
File content/browser/tab_contents/tab_contents_delegate.cc (right):

http://codereview.chromium.org/7244009/diff/1/content/browser/tab_contents/ta...
content/browser/tab_contents/tab_contents_delegate.cc:240: void
TabContentsDelegate::Attach(TabContents* tab_contents) {
Position should match header.

[tburkard, 2011-06-23 18:28:11]

Yes I'm not sure of how people might use this...

Might it be possible to do some code search to see whether this would be a
problem or not?

On 2011/06/23 18:24:37, cbentzel wrote:
> For example, without your change I get
> 
> 	base::debug::StackTrace::StackTrace() [0x7fb262a3cffe]
> 	logging::LogMessage::~LogMessage() [0x7fb262a6118e]
> 	TabContentsDelegate::~TabContentsDelegate() [0x7fb264ce3246]
>
	prerender::PrerenderContents::TabContentsDelegateImpl::~TabContentsDelegateImpl()
> [0x7fb263915734]
> 	scoped_ptr<>::~scoped_ptr() [0x7fb263915783]
> 	prerender::PrerenderContents::~PrerenderContents() [0x7fb2639139b7]
> 	prerender::PrerenderManager::DeletePendingDeleteEntries() [0x7fb2633c48f0]
> 	prerender::PrerenderManager::AddPreload() [0x7fb2633c2ab0]
> 	prerender::HandleTag() [0x7fb2633c1dfd]
> 	DispatchToFunction<>() [0x7fb26342ac98]
> 	RunnableFunction<>::Run() [0x7fb26342ac10]
> 	(anonymous namespace)::TaskClosureAdapter::Run() [0x7fb262a6420d]
> 	base::internal::Invoker1<>::DoInvoke() [0x7fb262a67e06]
> 	base::Callback<>::Run() [0x7fb262a3fb89]
> 	MessageLoop::RunTask() [0x7fb262a66bd7]
> 	MessageLoop::DeferOrRunPendingTask() [0x7fb262a66d0d]
> 	MessageLoop::DoWork() [0x7fb262a67523]
> 	base::MessagePumpForUI::RunWithDispatcher() [0x7fb262a1a2c7]
> 	base::MessagePumpForUI::Run() [0x7fb262a1a77e]
> 	MessageLoop::RunInternal() [0x7fb262a669cb]
> 	MessageLoop::RunHandler() [0x7fb262a6687e]
> 	MessageLoopForUI::Run() [0x7fb262a67bbe]
> 	(anonymous namespace)::RunUIMessageLoop() [0x7fb26312a0b0]
> 	BrowserMain() [0x7fb26312d150]
> 	(anonymous namespace)::RunNamedProcessTypeMain() [0x7fb263087725]
> 	ChromeMain [0x7fb263088164]
> 	main [0x7fb263088da4]
> 	0x7fb25732bc4d
> 	0x7fb2630867e9
> 
> The concern is that we may have cases where we destroy the
> TabContentsDelegate and immediately do a set_delegate(NULL) on the
> TabContents that it references - that's fairly safe, and would
> incorrectly trigger this check.
> 
> 
> On Thu, Jun 23, 2011 at 2:22 PM, Chris Bentzel <mailto:cbentzel@chromium.org>
wrote:
> > This will CHECK in the TabContentsDelegate destructor if any
> > TabContents have it as the current delegate pointer. So it will let us
> > know which TabContentsDelegate was the cause in the use-after-free
> > case in the callstack.
> >
> > On Thu, Jun 23, 2011 at 2:20 PM, Scott Violet <mailto:sky@chromium.org>
wrote:
> >> The bug is that TabContents delegate has been deleted. Unless I'm
> >> missing something I don't think this would help track that down.
> >>
> >> I'm going to add an id to each delegate so that we can know the type
> >> of the last delegate. I'm also going to make browser CHECK that the
> >> tabstrip is empty.
> >>
> >> &nbsp;-Scott
> >>
> >> On Thu, Jun 23, 2011 at 11:15 AM, mailto: <cbentzel@chromium.org> wrote:
> >>> Reviewers: sky, tburkard, John Abd-El-Malek,
> >>>
> >>> Message:
> >>> I'm wondering if we want something like this while tracking down crash
> >>> causes.
> >>>
> >>> Description:
> >>> Helper functions for tracking stale TabContentsDelegate's.
> >>>
> >>> BUG=85247
> >>> TEST=None
> >>>
> >>>
> >>> Please review this at http://codereview.chromium.org/7244009/
> >>>
> >>> SVN Base: svn://svn.chromium.org/chrome/trunk/src
> >>>
> >>> Affected files:
> >>> &nbsp;M content/browser/tab_contents/tab_contents.h
> >>> &nbsp;M content/browser/tab_contents/tab_contents.cc
> >>> &nbsp;M content/browser/tab_contents/tab_contents_delegate.h
> >>> &nbsp;M content/browser/tab_contents/tab_contents_delegate.cc
> >>>
> >>>
> >>> Index: content/browser/tab_contents/tab_contents.cc
> >>> diff --git a/content/browser/tab_contents/tab_contents.cc
> >>> b/content/browser/tab_contents/tab_contents.cc
> >>> index
> >>>
>
cc805054f208b971cb56e8f7554f03984c3bdc26..43a46f78fde0f7999694d8b0f522dd349e986aed
> >>> 100644
> >>> --- a/content/browser/tab_contents/tab_contents.cc
> >>> +++ b/content/browser/tab_contents/tab_contents.cc
> >>> @@ -623,6 +623,19 @@ void TabContents::AddNewContents(TabContents*
> >>> new_contents,
> >>> &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; user_gesture);
> >>> &nbsp;}
> >>>
> >>> +void TabContents::set_delegate(TabContentsDelegate* delegate) {
> >>> + &nbsp;if (delegate == delegate_)
> >>> + &nbsp; &nbsp;return;
> >>> +
> >>> + &nbsp;if (delegate_)
> >>> + &nbsp; &nbsp;delegate_->Detach(this);
> >>> +
> >>> + &nbsp;delegate_ = delegate;
> >>> +
> >>> + &nbsp;if (delegate_)
> >>> + &nbsp; &nbsp;delegate_->Attach(this);
> >>> +}
> >>> +
> >>> &nbsp;gfx::NativeView TabContents::GetContentNativeView() const {
> >>> &nbsp; return view_->GetContentNativeView();
> >>> &nbsp;}
> >>> Index: content/browser/tab_contents/tab_contents.h
> >>> diff --git a/content/browser/tab_contents/tab_contents.h
> >>> b/content/browser/tab_contents/tab_contents.h
> >>> index
> >>>
>
02814dd8cec6d8b915ae3ff45829953201c6450d..861a6a573e86e1cc43f99c334e72bf9cf47458f2
> >>> 100644
> >>> --- a/content/browser/tab_contents/tab_contents.h
> >>> +++ b/content/browser/tab_contents/tab_contents.h
> >>> @@ -98,7 +98,7 @@ class TabContents : public PageNavigator,
> >>> &nbsp; PropertyBag* property_bag() { return &property_bag_; }
> >>>
> >>> &nbsp; TabContentsDelegate* delegate() const { return delegate_; }
> >>> - &nbsp;void set_delegate(TabContentsDelegate* d) { delegate_ = d; }
> >>> + &nbsp;void set_delegate(TabContentsDelegate* delegate);
> >>>
> >>> &nbsp; // Gets the controller for this tab contents.
> >>> &nbsp; NavigationController& controller() { return controller_; }
> >>> Index: content/browser/tab_contents/tab_contents_delegate.cc
> >>> diff --git a/content/browser/tab_contents/tab_contents_delegate.cc
> >>> b/content/browser/tab_contents/tab_contents_delegate.cc
> >>> index
> >>>
>
df709a7f3549d644f105f444c44fa39572fa83ec..e10aa4597fef82946d51a2ac0e9e2ada0fb535c9
> >>> 100644
> >>> --- a/content/browser/tab_contents/tab_contents_delegate.cc
> >>> +++ b/content/browser/tab_contents/tab_contents_delegate.cc
> >>> @@ -237,5 +237,16 @@ TabContentsDelegate::GetJavaScriptDialogCreator() {
> >>> &nbsp; return JavaScriptDialogCreatorStub::GetInstance();
> >>> &nbsp;}
> >>>
> >>> +void TabContentsDelegate::Attach(TabContents* tab_contents) {
> >>> + &nbsp;CHECK(attached_contents_.find(tab_contents) ==
attached_contents_.end());
> >>> + &nbsp;attached_contents_.insert(tab_contents);
> >>> +}
> >>> +
> >>> +void TabContentsDelegate::Detach(TabContents* tab_contents) {
> >>> + &nbsp;CHECK(attached_contents_.find(tab_contents) !=
attached_contents_.end());
> >>> + &nbsp;attached_contents_.erase(tab_contents);
> >>> +}
> >>> +
> >>> &nbsp;TabContentsDelegate::~TabContentsDelegate() {
> >>> + &nbsp;CHECK(attached_contents_.empty());
> >>> &nbsp;}
> >>> Index: content/browser/tab_contents/tab_contents_delegate.h
> >>> diff --git a/content/browser/tab_contents/tab_contents_delegate.h
> >>> b/content/browser/tab_contents/tab_contents_delegate.h
> >>> index
> >>>
>
0645128b9add09ea15037d25547eb3260f806efb..d41b00bd8a392f1f78150501885a8de0e95f9b96
> >>> 100644
> >>> --- a/content/browser/tab_contents/tab_contents_delegate.h
> >>> +++ b/content/browser/tab_contents/tab_contents_delegate.h
> >>> @@ -6,6 +6,7 @@
> >>> &nbsp;#define CONTENT_BROWSER_TAB_CONTENTS_TAB_CONTENTS_DELEGATE_H_
> >>> &nbsp;#pragma once
> >>>
> >>> +#include <set>
> >>> &nbsp;#include <string>
> >>>
> >>> &nbsp;#include "base/basictypes.h"
> >>> @@ -50,6 +51,9 @@ class TabContentsDelegate {
> >>> &nbsp; &nbsp; virtual ~MainFrameCommitDetails() {}
> >>> &nbsp; };
> >>>
> >>> + &nbsp;void Attach(TabContents* source);
> >>> + &nbsp;void Detach(TabContents* source);
> >>> +
> >>> &nbsp; // Opens a new URL inside the passed in TabContents (if source is 0
open
> >>> &nbsp; // in the current front-most tab), unless |disposition| indicates
the url
> >>> &nbsp; // should be opened in a new tab or window.
> >>> @@ -310,6 +314,9 @@ class TabContentsDelegate {
> >>>
> >>> &nbsp;protected:
> >>> &nbsp; virtual ~TabContentsDelegate();
> >>> +
> >>> + private:
> >>> + &nbsp;std::set<TabContents*> attached_contents_;
> >>> &nbsp;};
> >>>
> >>> &nbsp;#endif &nbsp;//
CONTENT_BROWSER_TAB_CONTENTS_TAB_CONTENTS_DELEGATE_H_
> >>>
> >>>
> >>>
> >>
> >

[cbentzel, 2011-06-23 18:38:13]

Detaching in the destructor now.

I'd normally change set_delegate to SetDelegate, but I wanted to minimize
changes here in case we want to merge. I also wonder if this should be reverted
after tracking causes.

I couldn't find any unit tests for tab_contents/tab_contents_delegate. Are there
any?

Trybots should catch any of the false-crash cases, but I will inspect code as
well.

http://codereview.chromium.org/7244009/diff/1/content/browser/tab_contents/ta...
File content/browser/tab_contents/tab_contents_delegate.cc (right):

http://codereview.chromium.org/7244009/diff/1/content/browser/tab_contents/ta...
content/browser/tab_contents/tab_contents_delegate.cc:240: void
TabContentsDelegate::Attach(TabContents* tab_contents) {
On 2011/06/23 18:27:32, sky wrote:
> Position should match header.

Done.

[sky, 2011-06-23 18:51:27]

Not sure about test cases for TabContentsDelegate. There probably aren't, but
feel free to add something.

  -Scott

http://codereview.chromium.org/7244009/diff/2003/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/7244009/diff/2003/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:222: set_delegate(NULL);
You should do this last. It's possible some of these other methods trigger a
call to the delegate_. In fact it might even be possible for some of the value
objects in TabContents to trigger a call to the delegate_. Not sure.
Seems safest to use Detach at the end.

http://codereview.chromium.org/7244009/diff/2003/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:628: void
TabContents::set_delegate(TabContentsDelegate* delegate) {
Add a TODO about either removing debugging code, or renaming to SetDelegate.

http://codereview.chromium.org/7244009/diff/2003/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents_delegate.h (right):

http://codereview.chromium.org/7244009/diff/2003/content/browser/tab_contents...
content/browser/tab_contents/tab_contents_delegate.h:54: // Called when |this|
becomes the TabContentsDelegate for |source|.
I don't like seeing these in the public section. How about making TabContents a
friend and moving these to private?

[cbentzel, 2011-06-23 19:09:57]

http://codereview.chromium.org/7244009/diff/2003/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/7244009/diff/2003/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:222: set_delegate(NULL);
On 2011/06/23 18:51:27, sky wrote:
> You should do this last. It's possible some of these other methods trigger a
> call to the delegate_. In fact it might even be possible for some of the value
> objects in TabContents to trigger a call to the delegate_. Not sure.
> Seems safest to use Detach at the end.

Done.

http://codereview.chromium.org/7244009/diff/2003/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:628: void
TabContents::set_delegate(TabContentsDelegate* delegate) {
On 2011/06/23 18:51:27, sky wrote:
> Add a TODO about either removing debugging code, or renaming to SetDelegate.

Done.

http://codereview.chromium.org/7244009/diff/2003/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:629: if (delegate == delegate_)
Also, compacted vertical whitespace of set_delegate and reordered to appropriate
place in the file.

http://codereview.chromium.org/7244009/diff/2003/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents_delegate.h (right):

http://codereview.chromium.org/7244009/diff/2003/content/browser/tab_contents...
content/browser/tab_contents/tab_contents_delegate.h:54: // Called when |this|
becomes the TabContentsDelegate for |source|.
On 2011/06/23 18:51:27, sky wrote:
> I don't like seeing these in the public section. How about making TabContents
a
> friend and moving these to private?

Done.

[sky, 2011-06-23 19:44:47]

LGTM

[commit-bot: I haz the power, 2011-06-24 11:43:33]

Change committed as 90359

[cbentzel, 2011-06-24 12:13:02]

This triggered one ChromeOS stack trace on the main build bot, so I
reverted for now. I'll run ChromeOS tries next time I try to land
this.

	base::debug::StackTrace::StackTrace() [0x16027ae]
	logging::LogMessage::~LogMessage() [0x1621b8e]
	TabContentsDelegate::~TabContentsDelegate() [0x37b798e]
	chromeos::(anonymous
namespace)::EnrollmentDomView::~EnrollmentDomView() [0x90b2d2]
	views::View::~View() [0x1c415ae]
	chromeos::EnterpriseEnrollmentView::~EnterpriseEnrollmentView() [0x90b6a6]
	chromeos::ViewScreen<>::~ViewScreen() [0x90a635]
	chromeos::EnterpriseEnrollmentScreen::~EnterpriseEnrollmentScreen() [0x909127]
	scoped_ptr<>::~scoped_ptr() [0x94ec59]
	chromeos::WizardController::~WizardController() [0x94c878]
	scoped_ptr<>::~scoped_ptr() [0x90873b]
	chromeos::BaseLoginDisplayHost::~BaseLoginDisplayHost() [0x90720b]
	chromeos::ViewsLoginDisplayHost::~ViewsLoginDisplayHost() [0x946283]
	DeleteTask<>::Run() [0x530ac6]
	(anonymous namespace)::TaskClosureAdapter::Run() [0x1623e09]
	base::internal::Invoker1<>::DoInvoke() [0x16279ce]
	base::Callback<>::Run() [0x11561e9]
	MessageLoop::RunTask() [0x162679f]
	MessageLoop::DeferOrRunPendingTask() [0x16268d5]
	MessageLoop::DoWork() [0x16270eb]
	base::MessagePumpForUI::HandleDispatch() [0x1684ddd]
	(anonymous namespace)::WorkSourceDispatch() [0x168427b]
	0x2b44c71728c2
	0x2b44c7176748
	0x2b44c71768fc
	base::MessagePumpForUI::RunOnce() [0x1684bc9]
	base::MessagePumpForUI::RunWithDispatcher() [0x1684a62]
	MessageLoop::RunInternal() [0x1626563]
	MessageLoop::RunHandler() [0x1626446]
	MessageLoopForUI::Run() [0x1627786]
	ui_test_utils::RunMessageLoop() [0x15bfead]
	ui_test_utils::RegisterAndWait() [0x15c1675]
	ui_test_utils::WaitForNotification() [0x15c15de]
	chromeos::WizardInProcessBrowserTest::CleanUpOnMainThread() [0x530875]
	InProcessBrowserTest::RunTestOnMainThreadLoop() [0x15b728d]
	DispatchToMethod<>() [0x15b7a65]
	RunnableMethod<>::Run() [0x15b79a8]
	BrowserMain() [0x39eb397]
	InProcessBrowserTest::SetUp() [0x15b6a91]
	testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x19cea43]
	testing::internal::HandleExceptionsInMethodIfSupported<>() [0x19cbfae]
	testing::Test::Run() [0x19c1289]
	testing::TestInfo::Run() [0x19c1b02]
	testing::TestCase::Run() [0x19c21f8]
	testing::internal::UnitTestImpl::RunAllTests() [0x19c6fcf]
	testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x19cf88f]
	testing::internal::HandleExceptionsInMethodIfSupported<>() [0x19cc79f]
	testing::UnitTest::Run() [0x19c5aec]
	base::TestSuite::Run() [0x16969b7]
	main [0x7b0387]
	0x2b44ca9f9c4d
	0x436789


On Fri, Jun 24, 2011 at 7:43 AM,  <commit-bot@chromium.org> wrote:
> Change committed as 90359
>
> http://codereview.chromium.org/7244009/
>

[cbentzel, 2011-06-24 12:17:14]

Also, instead of CHECK'ing that the set is empty, we could just go
ahead and change the TabContentsDelegate destructor to
set_delegate(NULL) on any attached TabContents. Not sure if that's the
preferable approach - if it is, this is more of a permanent change
than what I have now to help catch violators.

On Fri, Jun 24, 2011 at 8:12 AM, Chris Bentzel <cbentzel@chromium.org> wrote:
> This triggered one ChromeOS stack trace on the main build bot, so I
> reverted for now. I'll run ChromeOS tries next time I try to land
> this.
>
>        base::debug::StackTrace::StackTrace() [0x16027ae]
>        logging::LogMessage::~LogMessage() [0x1621b8e]
>        TabContentsDelegate::~TabContentsDelegate() [0x37b798e]
>        chromeos::(anonymous
> namespace)::EnrollmentDomView::~EnrollmentDomView() [0x90b2d2]
>        views::View::~View() [0x1c415ae]
>        chromeos::EnterpriseEnrollmentView::~EnterpriseEnrollmentView()
[0x90b6a6]
>        chromeos::ViewScreen<>::~ViewScreen() [0x90a635]
>        chromeos::EnterpriseEnrollmentScreen::~EnterpriseEnrollmentScreen()
[0x909127]
>        scoped_ptr<>::~scoped_ptr() [0x94ec59]
>        chromeos::WizardController::~WizardController() [0x94c878]
>        scoped_ptr<>::~scoped_ptr() [0x90873b]
>        chromeos::BaseLoginDisplayHost::~BaseLoginDisplayHost() [0x90720b]
>        chromeos::ViewsLoginDisplayHost::~ViewsLoginDisplayHost() [0x946283]
>        DeleteTask<>::Run() [0x530ac6]
>        (anonymous namespace)::TaskClosureAdapter::Run() [0x1623e09]
>        base::internal::Invoker1<>::DoInvoke() [0x16279ce]
>        base::Callback<>::Run() [0x11561e9]
>        MessageLoop::RunTask() [0x162679f]
>        MessageLoop::DeferOrRunPendingTask() [0x16268d5]
>        MessageLoop::DoWork() [0x16270eb]
>        base::MessagePumpForUI::HandleDispatch() [0x1684ddd]
>        (anonymous namespace)::WorkSourceDispatch() [0x168427b]
>        0x2b44c71728c2
>        0x2b44c7176748
>        0x2b44c71768fc
>        base::MessagePumpForUI::RunOnce() [0x1684bc9]
>        base::MessagePumpForUI::RunWithDispatcher() [0x1684a62]
>        MessageLoop::RunInternal() [0x1626563]
>        MessageLoop::RunHandler() [0x1626446]
>        MessageLoopForUI::Run() [0x1627786]
>        ui_test_utils::RunMessageLoop() [0x15bfead]
>        ui_test_utils::RegisterAndWait() [0x15c1675]
>        ui_test_utils::WaitForNotification() [0x15c15de]
>        chromeos::WizardInProcessBrowserTest::CleanUpOnMainThread() [0x530875]
>        InProcessBrowserTest::RunTestOnMainThreadLoop() [0x15b728d]
>        DispatchToMethod<>() [0x15b7a65]
>        RunnableMethod<>::Run() [0x15b79a8]
>        BrowserMain() [0x39eb397]
>        InProcessBrowserTest::SetUp() [0x15b6a91]
>        testing::internal::HandleSehExceptionsInMethodIfSupported<>()
[0x19cea43]
>        testing::internal::HandleExceptionsInMethodIfSupported<>() [0x19cbfae]
>        testing::Test::Run() [0x19c1289]
>        testing::TestInfo::Run() [0x19c1b02]
>        testing::TestCase::Run() [0x19c21f8]
>        testing::internal::UnitTestImpl::RunAllTests() [0x19c6fcf]
>        testing::internal::HandleSehExceptionsInMethodIfSupported<>()
[0x19cf88f]
>        testing::internal::HandleExceptionsInMethodIfSupported<>() [0x19cc79f]
>        testing::UnitTest::Run() [0x19c5aec]
>        base::TestSuite::Run() [0x16969b7]
>        main [0x7b0387]
>        0x2b44ca9f9c4d
>        0x436789
>
>
> On Fri, Jun 24, 2011 at 7:43 AM,  <commit-bot@chromium.org> wrote:
>> Change committed as 90359
>>
>> http://codereview.chromium.org/7244009/
>>
>

[sky, 2011-06-24 15:37:37]

I'm not opposed to set_delegate(NULL) in ~TabContentsDelegate. Other
observers (TabContentsObserver for one) remove themselves in the
destructor.

Before we do that I would like to figure out who isn't probably
cleaning up now. It may be indicative of another problem that we would
be covering up.

  -Scott

On Fri, Jun 24, 2011 at 5:17 AM, Chris Bentzel <cbentzel@chromium.org> wrote:
> Also, instead of CHECK'ing that the set is empty, we could just go
> ahead and change the TabContentsDelegate destructor to
> set_delegate(NULL) on any attached TabContents. Not sure if that's the
> preferable approach - if it is, this is more of a permanent change
> than what I have now to help catch violators.
>
> On Fri, Jun 24, 2011 at 8:12 AM, Chris Bentzel <cbentzel@chromium.org> wrote:
>> This triggered one ChromeOS stack trace on the main build bot, so I
>> reverted for now. I'll run ChromeOS tries next time I try to land
>> this.
>>
>>        base::debug::StackTrace::StackTrace() [0x16027ae]
>>        logging::LogMessage::~LogMessage() [0x1621b8e]
>>        TabContentsDelegate::~TabContentsDelegate() [0x37b798e]
>>        chromeos::(anonymous
>> namespace)::EnrollmentDomView::~EnrollmentDomView() [0x90b2d2]
>>        views::View::~View() [0x1c415ae]
>>        chromeos::EnterpriseEnrollmentView::~EnterpriseEnrollmentView()
[0x90b6a6]
>>        chromeos::ViewScreen<>::~ViewScreen() [0x90a635]
>>        chromeos::EnterpriseEnrollmentScreen::~EnterpriseEnrollmentScreen()
[0x909127]
>>        scoped_ptr<>::~scoped_ptr() [0x94ec59]
>>        chromeos::WizardController::~WizardController() [0x94c878]
>>        scoped_ptr<>::~scoped_ptr() [0x90873b]
>>        chromeos::BaseLoginDisplayHost::~BaseLoginDisplayHost() [0x90720b]
>>        chromeos::ViewsLoginDisplayHost::~ViewsLoginDisplayHost() [0x946283]
>>        DeleteTask<>::Run() [0x530ac6]
>>        (anonymous namespace)::TaskClosureAdapter::Run() [0x1623e09]
>>        base::internal::Invoker1<>::DoInvoke() [0x16279ce]
>>        base::Callback<>::Run() [0x11561e9]
>>        MessageLoop::RunTask() [0x162679f]
>>        MessageLoop::DeferOrRunPendingTask() [0x16268d5]
>>        MessageLoop::DoWork() [0x16270eb]
>>        base::MessagePumpForUI::HandleDispatch() [0x1684ddd]
>>        (anonymous namespace)::WorkSourceDispatch() [0x168427b]
>>        0x2b44c71728c2
>>        0x2b44c7176748
>>        0x2b44c71768fc
>>        base::MessagePumpForUI::RunOnce() [0x1684bc9]
>>        base::MessagePumpForUI::RunWithDispatcher() [0x1684a62]
>>        MessageLoop::RunInternal() [0x1626563]
>>        MessageLoop::RunHandler() [0x1626446]
>>        MessageLoopForUI::Run() [0x1627786]
>>        ui_test_utils::RunMessageLoop() [0x15bfead]
>>        ui_test_utils::RegisterAndWait() [0x15c1675]
>>        ui_test_utils::WaitForNotification() [0x15c15de]
>>        chromeos::WizardInProcessBrowserTest::CleanUpOnMainThread() [0x530875]
>>        InProcessBrowserTest::RunTestOnMainThreadLoop() [0x15b728d]
>>        DispatchToMethod<>() [0x15b7a65]
>>        RunnableMethod<>::Run() [0x15b79a8]
>>        BrowserMain() [0x39eb397]
>>        InProcessBrowserTest::SetUp() [0x15b6a91]
>>        testing::internal::HandleSehExceptionsInMethodIfSupported<>()
[0x19cea43]
>>        testing::internal::HandleExceptionsInMethodIfSupported<>() [0x19cbfae]
>>        testing::Test::Run() [0x19c1289]
>>        testing::TestInfo::Run() [0x19c1b02]
>>        testing::TestCase::Run() [0x19c21f8]
>>        testing::internal::UnitTestImpl::RunAllTests() [0x19c6fcf]
>>        testing::internal::HandleSehExceptionsInMethodIfSupported<>()
[0x19cf88f]
>>        testing::internal::HandleExceptionsInMethodIfSupported<>() [0x19cc79f]
>>        testing::UnitTest::Run() [0x19c5aec]
>>        base::TestSuite::Run() [0x16969b7]
>>        main [0x7b0387]
>>        0x2b44ca9f9c4d
>>        0x436789
>>
>>
>> On Fri, Jun 24, 2011 at 7:43 AM,  <commit-bot@chromium.org> wrote:
>>> Change committed as 90359
>>>
>>> http://codereview.chromium.org/7244009/
>>>
>>
>

[cbentzel, 2011-06-24 15:42:29]

Yeah, I agree. I will keep working on this in between memory sheriff
and meetings today.

On Fri, Jun 24, 2011 at 11:37 AM, Scott Violet <sky@chromium.org> wrote:
> I'm not opposed to set_delegate(NULL) in ~TabContentsDelegate. Other
> observers (TabContentsObserver for one) remove themselves in the
> destructor.
>
> Before we do that I would like to figure out who isn't probably
> cleaning up now. It may be indicative of another problem that we would
> be covering up.
>
>  -Scott
>
> On Fri, Jun 24, 2011 at 5:17 AM, Chris Bentzel <cbentzel@chromium.org> wrote:
>> Also, instead of CHECK'ing that the set is empty, we could just go
>> ahead and change the TabContentsDelegate destructor to
>> set_delegate(NULL) on any attached TabContents. Not sure if that's the
>> preferable approach - if it is, this is more of a permanent change
>> than what I have now to help catch violators.
>>
>> On Fri, Jun 24, 2011 at 8:12 AM, Chris Bentzel <cbentzel@chromium.org> wrote:
>>> This triggered one ChromeOS stack trace on the main build bot, so I
>>> reverted for now. I'll run ChromeOS tries next time I try to land
>>> this.
>>>
>>>        base::debug::StackTrace::StackTrace() [0x16027ae]
>>>        logging::LogMessage::~LogMessage() [0x1621b8e]
>>>        TabContentsDelegate::~TabContentsDelegate() [0x37b798e]
>>>        chromeos::(anonymous
>>> namespace)::EnrollmentDomView::~EnrollmentDomView() [0x90b2d2]
>>>        views::View::~View() [0x1c415ae]
>>>        chromeos::EnterpriseEnrollmentView::~EnterpriseEnrollmentView()
[0x90b6a6]
>>>        chromeos::ViewScreen<>::~ViewScreen() [0x90a635]
>>>        chromeos::EnterpriseEnrollmentScreen::~EnterpriseEnrollmentScreen()
[0x909127]
>>>        scoped_ptr<>::~scoped_ptr() [0x94ec59]
>>>        chromeos::WizardController::~WizardController() [0x94c878]
>>>        scoped_ptr<>::~scoped_ptr() [0x90873b]
>>>        chromeos::BaseLoginDisplayHost::~BaseLoginDisplayHost() [0x90720b]
>>>        chromeos::ViewsLoginDisplayHost::~ViewsLoginDisplayHost() [0x946283]
>>>        DeleteTask<>::Run() [0x530ac6]
>>>        (anonymous namespace)::TaskClosureAdapter::Run() [0x1623e09]
>>>        base::internal::Invoker1<>::DoInvoke() [0x16279ce]
>>>        base::Callback<>::Run() [0x11561e9]
>>>        MessageLoop::RunTask() [0x162679f]
>>>        MessageLoop::DeferOrRunPendingTask() [0x16268d5]
>>>        MessageLoop::DoWork() [0x16270eb]
>>>        base::MessagePumpForUI::HandleDispatch() [0x1684ddd]
>>>        (anonymous namespace)::WorkSourceDispatch() [0x168427b]
>>>        0x2b44c71728c2
>>>        0x2b44c7176748
>>>        0x2b44c71768fc
>>>        base::MessagePumpForUI::RunOnce() [0x1684bc9]
>>>        base::MessagePumpForUI::RunWithDispatcher() [0x1684a62]
>>>        MessageLoop::RunInternal() [0x1626563]
>>>        MessageLoop::RunHandler() [0x1626446]
>>>        MessageLoopForUI::Run() [0x1627786]
>>>        ui_test_utils::RunMessageLoop() [0x15bfead]
>>>        ui_test_utils::RegisterAndWait() [0x15c1675]
>>>        ui_test_utils::WaitForNotification() [0x15c15de]
>>>        chromeos::WizardInProcessBrowserTest::CleanUpOnMainThread()
[0x530875]
>>>        InProcessBrowserTest::RunTestOnMainThreadLoop() [0x15b728d]
>>>        DispatchToMethod<>() [0x15b7a65]
>>>        RunnableMethod<>::Run() [0x15b79a8]
>>>        BrowserMain() [0x39eb397]
>>>        InProcessBrowserTest::SetUp() [0x15b6a91]
>>>        testing::internal::HandleSehExceptionsInMethodIfSupported<>()
[0x19cea43]
>>>        testing::internal::HandleExceptionsInMethodIfSupported<>()
[0x19cbfae]
>>>        testing::Test::Run() [0x19c1289]
>>>        testing::TestInfo::Run() [0x19c1b02]
>>>        testing::TestCase::Run() [0x19c21f8]
>>>        testing::internal::UnitTestImpl::RunAllTests() [0x19c6fcf]
>>>        testing::internal::HandleSehExceptionsInMethodIfSupported<>()
[0x19cf88f]
>>>        testing::internal::HandleExceptionsInMethodIfSupported<>()
[0x19cc79f]
>>>        testing::UnitTest::Run() [0x19c5aec]
>>>        base::TestSuite::Run() [0x16969b7]
>>>        main [0x7b0387]
>>>        0x2b44ca9f9c4d
>>>        0x436789
>>>
>>>
>>> On Fri, Jun 24, 2011 at 7:43 AM,  <commit-bot@chromium.org> wrote:
>>>> Change committed as 90359
>>>>
>>>> http://codereview.chromium.org/7244009/
>>>>
>>>
>>
>