Makes it so when NSViewControllers go away that they remove their views from ...

Makes it so when NSViewControllers go away that they remove their views from the
view hierarchy before they are gone. The fix to bookmark_bar_controller.mm is the
only one required to fix the bug, but the others are future proofing against the 
same issue.

BUG=24734
TEST=Make sure we can start up with just the bookmark bar showing and quit without crashing.

[dmac, 2009-10-13 19:30:48]

Hey folks...

I can't think why this would cause problems, and can think of several reasons
why not having it could cause potential problems in the future so let me know
what you think.

In the case in the bug the controllers had all been let go, but the window had
some final computations it was doing (before it was closed) one of which caused
the window to ask about the views opaqueness. This in turn caused the
bookmarkbar to reference it's controller which of course was gone. I can't think
of any reason we would want the views to still be part of the hierarchy if their
controllers are gone, and since we are trying to keep weak references between
views and their controllers, I felt it was best just to remove them from the
view hierarchy. Other NSViewController subclasses already had similar code in
their dealloc (eg download_item_controller.mm) assumedly because they had
already run into similar issues.

[Avi (use Gerrit), 2009-10-13 19:34:31]

http://codereview.chromium.org/267082/diff/1/2
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/267082/diff/1/2#newcode93
Line 93: // Remove our view from it's superview so it doesn't attempt to
reference
its superview

http://codereview.chromium.org/267082/diff/1/9
File chrome/browser/cocoa/download_shelf_controller.mm (right):

http://codereview.chromium.org/267082/diff/1/9#newcode104
Line 104: // Remove our view from it's superview so it doesn't attempt to
reference
ditto

http://codereview.chromium.org/267082/diff/1/6
File chrome/browser/cocoa/extension_shelf_controller.mm (right):

http://codereview.chromium.org/267082/diff/1/6#newcode332
Line 332: // Remove our view from it's superview so it doesn't attempt to
reference
and the same here

http://codereview.chromium.org/267082/diff/1/4
File chrome/browser/cocoa/find_bar_cocoa_controller.mm (right):

http://codereview.chromium.org/267082/diff/1/4#newcode53
Line 53: // Remove our view from it's superview so it doesn't attempt to
reference
you get the idea

[rohitrao (ping after 24h), 2009-10-13 19:47:04]

I can't decide if this is the correct fix or just a spot-treatment of one of the
symptoms.  Maybe the right thing to do is figure out why the controllers are
getting destroyed before the window and stop that from happening?  I'm worried
that removing the views from the hierarchy isn't actually going to fix the
underlying problem, or that we'll still have pointers to these controllers
sitting somewhere else.

[dmac, 2009-10-13 19:56:56]

On 2009/10/13 19:47:04, rohitrao wrote:
> I can't decide if this is the correct fix or just a spot-treatment of one of
the
> symptoms.  Maybe the right thing to do is figure out why the controllers are
> getting destroyed before the window and stop that from happening?  I'm worried
> that removing the views from the hierarchy isn't actually going to fix the
> underlying problem, or that we'll still have pointers to these controllers
> sitting somewhere else.

Rohit, the problem is that according to Apple window destruction is
non-deterministic. I have a radar out on this very problem (5851458). Quote from
feedback in the bug:

"Unfortunately, the time of deallocation of the window is non-deterministic. 
You cannot assume that the window will be deallocated when your application's
last retain is released.  "

Avi: new version up with all the possessive pronouns fixed.

[rohitrao (ping after 24h), 2009-10-13 20:02:11]

> Rohit, the problem is that according to Apple window destruction is
> non-deterministic.

Bleh =(  Is there a good place to add a comment saying that all controllers need
to call removeFromSuperview in dealloc?  Maybe above the declarations in
browser_window_controller.h, although I'm not sure it'll actually get noticed
there?  I'm going to bet we'll forget to call this when we add the next
controller ;)

[pink (ping after 24hrs), 2009-10-13 20:06:53]

(+mark because he's been involved in the discussions since we first
discovered this "problem")

We've had this manifest itself in several different ways (the
ThreadSanitizer folks initially wanted to fix this in the unit tests a
few months ago), but we haven't yet moved on it for reasons stated
previously (mostly because it seemed like spot fixes for a larger
problem and that we were destined to f- it up again later with the
next controller that got added).

It seems like all of Cocoa would have this problem, no? Why are we
getting bit by it? Perhaps it's because we keep the window around
longer than "normal" for objc-c to ensure our C++ dtors get called
(see comment in browser_window_controller.h).

Also, is it just NSViewControllers specifically, or is it any view
that has a weak reference back to something that could have already
gone away?

On Tue, Oct 13, 2009 at 4:02 PM,  <rohitrao@chromium.org> wrote:
>> Rohit, the problem is that according to Apple window destruction is
>> non-deterministic.
>
> Bleh =3D( =A0Is there a good place to add a comment saying that all contr=
ollers
> need
> to call removeFromSuperview in dealloc? =A0Maybe above the declarations i=
n
> browser_window_controller.h, although I'm not sure it'll actually get
> noticed
> there? =A0I'm going to bet we'll forget to call this when we add the next
> controller ;)
>
> http://codereview.chromium.org/267082
>



--=20
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[Mark Mentovai, 2009-10-13 20:34:32]

And +stuartmorgan too, he's been involved also.

The only thing i have to contribute is:

yes, this is a problem
yes, it bites
specifically, it can bite us

Mark

Mike Pinkerton wrote:
> (+mark because he's been involved in the discussions since we first
> discovered this "problem")
>
> We've had this manifest itself in several different ways (the
> ThreadSanitizer folks initially wanted to fix this in the unit tests a
> few months ago), but we haven't yet moved on it for reasons stated
> previously (mostly because it seemed like spot fixes for a larger
> problem and that we were destined to f- it up again later with the
> next controller that got added).
>
> It seems like all of Cocoa would have this problem, no? Why are we
> getting bit by it? Perhaps it's because we keep the window around
> longer than "normal" for objc-c to ensure our C++ dtors get called
> (see comment in browser_window_controller.h).
>
> Also, is it just NSViewControllers specifically, or is it any view
> that has a weak reference back to something that could have already
> gone away?
>
> On Tue, Oct 13, 2009 at 4:02 PM, =A0<rohitrao@chromium.org> wrote:
>>> Rohit, the problem is that according to Apple window destruction is
>>> non-deterministic.
>>
>> Bleh =3D( =A0Is there a good place to add a comment saying that all cont=
rollers
>> need
>> to call removeFromSuperview in dealloc? =A0Maybe above the declarations =
in
>> browser_window_controller.h, although I'm not sure it'll actually get
>> noticed
>> there? =A0I'm going to bet we'll forget to call this when we add the nex=
t
>> controller ;)
>>
>> http://codereview.chromium.org/267082
>>
>
>
>
> --
> Mike Pinkerton
> Mac Weenie
> pinkerton@google.com
>

[Mark Mentovai, 2009-10-13 20:35:40]

Forget it, Stuart, I'm thinking about exceptions, and I'm not quite
sure why.  This thread is about something else.

I wrote:
> And +stuartmorgan too, he's been involved also.
>
> The only thing i have to contribute is:
>
> yes, this is a problem
> yes, it bites
> specifically, it can bite us
>
> Mark
>
> Mike Pinkerton wrote:
>> (+mark because he's been involved in the discussions since we first
>> discovered this "problem")
>>
>> We've had this manifest itself in several different ways (the
>> ThreadSanitizer folks initially wanted to fix this in the unit tests a
>> few months ago), but we haven't yet moved on it for reasons stated
>> previously (mostly because it seemed like spot fixes for a larger
>> problem and that we were destined to f- it up again later with the
>> next controller that got added).
>>
>> It seems like all of Cocoa would have this problem, no? Why are we
>> getting bit by it? Perhaps it's because we keep the window around
>> longer than "normal" for objc-c to ensure our C++ dtors get called
>> (see comment in browser_window_controller.h).
>>
>> Also, is it just NSViewControllers specifically, or is it any view
>> that has a weak reference back to something that could have already
>> gone away?
>>
>> On Tue, Oct 13, 2009 at 4:02 PM, =A0<rohitrao@chromium.org> wrote:
>>>> Rohit, the problem is that according to Apple window destruction is
>>>> non-deterministic.
>>>
>>> Bleh =3D( =A0Is there a good place to add a comment saying that all con=
trollers
>>> need
>>> to call removeFromSuperview in dealloc? =A0Maybe above the declarations=
 in
>>> browser_window_controller.h, although I'm not sure it'll actually get
>>> noticed
>>> there? =A0I'm going to bet we'll forget to call this when we add the ne=
xt
>>> controller ;)
>>>
>>> http://codereview.chromium.org/267082
>>>
>>
>>
>>
>> --
>> Mike Pinkerton
>> Mac Weenie
>> pinkerton@google.com
>>
>

[pink (ping after 24hrs), 2009-10-15 14:05:19]

What confuses me here is that the original cl comment mentions:

"This in turn caused the
bookmarkbar to reference it's controller which of course was gone."

I see no code in bookmark_bar_view.mm that talks to its controller. In
addition, my original question remains: Why is this only
NSViewControllers, not all controllers (eg, TabStripController)?

I see this crash very frequently, especially with tab dragging and
quitting the app, so it's imperative we get something landed soon, but
I don't understand enough about what's going on to rubber-stamp it.

jonc: our dev build this week is likely going to ship with this
crasher, so we should be prepared for a sharp jump in uma crashes.

On Tue, Oct 13, 2009 at 4:35 PM, Mark Mentovai <mark@chromium.org> wrote:
> Forget it, Stuart, I'm thinking about exceptions, and I'm not quite
> sure why. =A0This thread is about something else.
>
> I wrote:
>> And +stuartmorgan too, he's been involved also.
>>
>> The only thing i have to contribute is:
>>
>> yes, this is a problem
>> yes, it bites
>> specifically, it can bite us
>>
>> Mark
>>
>> Mike Pinkerton wrote:
>>> (+mark because he's been involved in the discussions since we first
>>> discovered this "problem")
>>>
>>> We've had this manifest itself in several different ways (the
>>> ThreadSanitizer folks initially wanted to fix this in the unit tests a
>>> few months ago), but we haven't yet moved on it for reasons stated
>>> previously (mostly because it seemed like spot fixes for a larger
>>> problem and that we were destined to f- it up again later with the
>>> next controller that got added).
>>>
>>> It seems like all of Cocoa would have this problem, no? Why are we
>>> getting bit by it? Perhaps it's because we keep the window around
>>> longer than "normal" for objc-c to ensure our C++ dtors get called
>>> (see comment in browser_window_controller.h).
>>>
>>> Also, is it just NSViewControllers specifically, or is it any view
>>> that has a weak reference back to something that could have already
>>> gone away?
>>>
>>> On Tue, Oct 13, 2009 at 4:02 PM, =A0<rohitrao@chromium.org> wrote:
>>>>> Rohit, the problem is that according to Apple window destruction is
>>>>> non-deterministic.
>>>>
>>>> Bleh =3D( =A0Is there a good place to add a comment saying that all co=
ntrollers
>>>> need
>>>> to call removeFromSuperview in dealloc? =A0Maybe above the declaration=
s in
>>>> browser_window_controller.h, although I'm not sure it'll actually get
>>>> noticed
>>>> there? =A0I'm going to bet we'll forget to call this when we add the n=
ext
>>>> controller ;)
>>>>
>>>> http://codereview.chromium.org/267082
>>>>
>>>
>>>
>>>
>>> --
>>> Mike Pinkerton
>>> Mac Weenie
>>> pinkerton@google.com
>>>
>>
>



--=20
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[pink (ping after 24hrs), 2009-10-15 14:33:00]

Ah ha, it's bookmark_bar_toolbar_view.mm that has the back reference
that I always see.

Dave: you mentioned running into some problems yesterday, anything I
can do to help?

On Thu, Oct 15, 2009 at 10:03 AM, Mike Pinkerton <pinkerton@chromium.org> w=
rote:
> What confuses me here is that the original cl comment mentions:
>
> "This in turn caused the
> bookmarkbar to reference it's controller which of course was gone."
>
> I see no code in bookmark_bar_view.mm that talks to its controller. In
> addition, my original question remains: Why is this only
> NSViewControllers, not all controllers (eg, TabStripController)?
>
> I see this crash very frequently, especially with tab dragging and
> quitting the app, so it's imperative we get something landed soon, but
> I don't understand enough about what's going on to rubber-stamp it.
>
> jonc: our dev build this week is likely going to ship with this
> crasher, so we should be prepared for a sharp jump in uma crashes.
>
> On Tue, Oct 13, 2009 at 4:35 PM, Mark Mentovai <mark@chromium.org> wrote:
>> Forget it, Stuart, I'm thinking about exceptions, and I'm not quite
>> sure why. =A0This thread is about something else.
>>
>> I wrote:
>>> And +stuartmorgan too, he's been involved also.
>>>
>>> The only thing i have to contribute is:
>>>
>>> yes, this is a problem
>>> yes, it bites
>>> specifically, it can bite us
>>>
>>> Mark
>>>
>>> Mike Pinkerton wrote:
>>>> (+mark because he's been involved in the discussions since we first
>>>> discovered this "problem")
>>>>
>>>> We've had this manifest itself in several different ways (the
>>>> ThreadSanitizer folks initially wanted to fix this in the unit tests a
>>>> few months ago), but we haven't yet moved on it for reasons stated
>>>> previously (mostly because it seemed like spot fixes for a larger
>>>> problem and that we were destined to f- it up again later with the
>>>> next controller that got added).
>>>>
>>>> It seems like all of Cocoa would have this problem, no? Why are we
>>>> getting bit by it? Perhaps it's because we keep the window around
>>>> longer than "normal" for objc-c to ensure our C++ dtors get called
>>>> (see comment in browser_window_controller.h).
>>>>
>>>> Also, is it just NSViewControllers specifically, or is it any view
>>>> that has a weak reference back to something that could have already
>>>> gone away?
>>>>
>>>> On Tue, Oct 13, 2009 at 4:02 PM, =A0<rohitrao@chromium.org> wrote:
>>>>>> Rohit, the problem is that according to Apple window destruction is
>>>>>> non-deterministic.
>>>>>
>>>>> Bleh =3D( =A0Is there a good place to add a comment saying that all c=
ontrollers
>>>>> need
>>>>> to call removeFromSuperview in dealloc? =A0Maybe above the declaratio=
ns in
>>>>> browser_window_controller.h, although I'm not sure it'll actually get
>>>>> noticed
>>>>> there? =A0I'm going to bet we'll forget to call this when we add the =
next
>>>>> controller ;)
>>>>>
>>>>> http://codereview.chromium.org/267082
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Mike Pinkerton
>>>> Mac Weenie
>>>> pinkerton@google.com
>>>>
>>>
>>
>
>
>
> --
> Mike Pinkerton
> Mac Weenie
> pinkerton@google.com
>



--=20
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[Nico, 2009-10-24 01:20:30]

See also http://codereview.chromium.org/193109 which did this for a different
controller. Back then I asked if we should just add this to all controllers, and
people seemed unenthusiastic. *shrug*

Anyway, I'm probably going to steal the bookmark_bar_controller part, because it
makes a CL of mine not pass the trybots.

[Nico, 2009-10-24 01:30:42]

On 2009/10/24 01:20:30, Nico wrote:
> See also http://codereview.chromium.org/193109 which did this for a different
> controller. Back then I asked if we should just add this to all controllers,
and
> people seemed unenthusiastic. *shrug*
> 
> Anyway, I'm probably going to steal the bookmark_bar_controller part, because
it
> makes a CL of mine not pass the trybots.

Here's the (short) discussion:
http://code.google.com/p/chromium/issues/detail?id=13111#c17

[dmac, 2009-10-24 22:52:35]

On 2009/10/24 01:30:42, Nico wrote:
> On 2009/10/24 01:20:30, Nico wrote:
> > See also http://codereview.chromium.org/193109 which did this for a
different
> > controller. Back then I asked if we should just add this to all controllers,
> and
> > people seemed unenthusiastic. *shrug*
> > 
> > Anyway, I'm probably going to steal the bookmark_bar_controller part,
because
> it
> > makes a CL of mine not pass the trybots.
> 
> Here's the (short) discussion:
> http://code.google.com/p/chromium/issues/detail?id=13111#c17

Nico... once all my changes land, the removing of the views should no longer be
necessary. If you are going to land something, please mark it with a
TODO(dmaclach): Remove, and file a bug for me to remove it.