alternate ntp: fix crash in ContentsContainer destructor

chrome/browser/ui/views/frame/contents_container.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/frame/contents_container.h"
 
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
@@ skipping 80 matching lines @@
 
 ContentsContainer::~ContentsContainer() {
 }
 
 void ContentsContainer::MakeOverlayContentsActiveContents() {
   DCHECK(overlay_);
 
   active_ = overlay_;
   overlay_ = NULL;
   overlay_web_contents_ = NULL;
+  // Unregister from observing previous |overlay_web_contents_|.
+  registrar_.RemoveAll();
   // Since |overlay_| has been nuked, shadow view is not needed anymore.
   // Note that the previous |active_| will be deleted by caller (see
   // BrowserView::ActiveTabChanged()) after this call, hence removing the old
   // |active_| as a child, and making the new |active_| (which is the previous
   // |overlay_|) the first child.
   shadow_view_.reset();
   Layout();
 }
 
 void ContentsContainer::SetOverlay(views::WebView* overlay,
@@ skipping 27 matching lines @@
         RemoveChildView(shadow_view_.get());
       RemoveChildView(overlay_);
     }
     overlay_ = overlay;
     if (overlay_)
       AddChildView(overlay_);
   }
 
   if (overlay_web_contents_ != overlay_web_contents) {
 #if !defined(OS_WIN)
-    // Unregister from previous overlay web contents' render view host.
-    if (overlay_web_contents_)
-      registrar_.RemoveAll();
+    // Unregister from observing previous |overlay_web_contents_|.
+    registrar_.RemoveAll();
 #endif  // !defined(OS_WIN)
 
     overlay_web_contents_ = overlay_web_contents;
 
 #if !defined(OS_WIN)
     // Register to new overlay web contents' render view host.
     if (overlay_web_contents_) {
       content::RenderViewHost* rvh = overlay_web_contents_->GetRenderViewHost();
       DCHECK(rvh);
       content::NotificationSource source =
           content::Source<content::RenderWidgetHost>(rvh);
       registrar_.Add(this,

[sky, 2013/04/05 19:44:17]

Can we removeAll right here too?

[kuan, 2013/04/05 21:54:11]

i already RemoveAll @151, is that enough?

[sky, 2013/04/05 23:00:08]

Yes, that's fine.

           content::NOTIFICATION_RENDER_WIDGET_HOST_DID_UPDATE_BACKING_STORE,
           source);
     }
 #endif  // !defined(OS_WIN)
   }
 
   overlay_height_ = height;
   overlay_height_units_ = units;
   draw_drop_shadow_ = draw_drop_shadow;
 
@@ skipping 93 matching lines @@
 std::string ContentsContainer::GetClassName() const {
   return kViewClassName;
 }
 
 void ContentsContainer::Observe(int type,
                                 const content::NotificationSource& source,
                                 const content::NotificationDetails& details) {
   DCHECK_EQ(content::NOTIFICATION_RENDER_WIDGET_HOST_DID_UPDATE_BACKING_STORE,
             type);
   // Remove shadow view if it's not needed.
-  if (overlay_ && !draw_drop_shadow_)
+  if (overlay_ && !draw_drop_shadow_ && overlay_web_contents_ &&
+      (content::Source<content::RenderWidgetHost>(source)).ptr() ==

[sky, 2013/04/05 19:44:17]

Shouldn't this be a DCHECK?

[kuan, 2013/04/05 21:54:11]

this is the dilemma i have:
@159, GetRenderViewHost could return NULL; since my DCHECK is a nop in release
mode, i'll registering for all sources in that case.  i have 2 options:
a) register even if rvh is NULL, then i need to keep the if cond here.
b) only register if rvh is not NULL, then i can change this to DCHECK.

i'm not sure which option is better.

[sky, 2013/04/05 23:00:08]

Only register if rvh is non-null.

[kuan, 2013/04/05 23:10:54]

Done.

+          overlay_web_contents_->GetRenderViewHost()) {
     shadow_view_.reset();
+  }
 }