Fix problems with render_widget_host_ being deleted out-of-sync with cocoa_view_.

Fix problems with render_widget_host_ being deleted out-of-sync with cocoa_view_.

Right after Destroy() is called, render_widget_host_ is deleted.  So
clear render_widget_host_ and guard calls to render_widget_host_.

http://crbug.com/14613
http://crbug.com/13514
http://crbug.com/12725

[Scott Hess - ex-Googler, 2009-07-02 22:22:48]

jrg because you owned the bug in question previously.  If you weren't working on
it ...

avi because you and I were the last people fixing this kind of thing.

This could also be partially addressed by calling out to the objects which may
have retained the view, and asking them to release.  That may not be a scalable
solution, and there is also still a window between Destroy() and when the
autorelease pool eventually let's the object go, so I think we'd want this
change either way.

[Avi (use Gerrit), 2009-07-02 23:07:03]

LG

http://codereview.chromium.org/155021/diff/1/3
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/155021/diff/1/3#newcode530
Line 530: if (!renderWidgetHostView_->render_widget_host_) return NO;
Are we allowed to one-line it like that?

[Scott Hess - ex-Googler, 2009-07-02 23:22:08]

http://codereview.chromium.org/155021/diff/1/3
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/155021/diff/1/3#newcode530
Line 530: if (!renderWidgetHostView_->render_widget_host_) return NO;
On 2009/07/02 23:07:04, Avi wrote:
> Are we allowed to one-line it like that?

Duh.  No.  Thanks for catching that!

[John Grabowski, 2009-07-06 16:34:55]

>> jrg because you owned the bug in question previously.

I think that was because a group of crashers were bulk-assigned to me.

In any case, LGTM with one comment.  How will you make sure future changes to
this class also perform the NULL check as needed?  Perhaps you can add a unit
test which sets render_widget_host_ to NULL, then calls all the methods, making
sure it doesn't blow up?

[Scott Hess - ex-Googler, 2009-07-06 17:06:57]

On 2009/07/06 16:34:55, John Grabowski wrote:
> >> jrg because you owned the bug in question previously.
> 
> I think that was because a group of crashers were bulk-assigned to me.
> 
> In any case, LGTM with one comment.  How will you make sure future changes to
> this class also perform the NULL check as needed?  Perhaps you can add a unit
> test which sets render_widget_host_ to NULL, then calls all the methods,
making
> sure it doesn't blow up?

I'll add that to my plate, but ... how will I make sure that the unittest calls
all the methods that some crazy future person adds?  At least will catch someone
removing the guard.

Your point has me thinking it might be more reasonable to track down all of the
places where this is being retained, deal with removing their reference, and
change the tests to DCHECK().  Unfortunately, I don't think the existing bugs
would have hit the DCHECKs in normal development.  Maybe both (DCHECK to
hopefully catch un-matched retains, if() guards because they often won't).

Would it make more sense to make render_widget_host_ private, and provide
forwarding methods to encapsulate the test?  Would need some refactoring, but it
probably wouldn't be too bad.  Then the Objective-C code can't make the mistake.

Another option might be to construct an "empty" RenderWidgetHost object for this
case.  Non-obvious how that would work, though.