Change HungPluginTabHelper to listen for infobar changes through Observer style.

chrome/browser/ui/hung_plugin_tab_helper.cc

 // Copyright (c) 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/hung_plugin_tab_helper.h"
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/process/process.h"
 #include "base/rand_util.h"
 #include "build/build_config.h"
-#include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/common/chrome_version_info.h"
 #include "components/infobars/core/confirm_infobar_delegate.h"
 #include "components/infobars/core/infobar.h"
 #include "content/public/browser/browser_child_process_host_iterator.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/child_process_data.h"
-#include "content/public/browser/notification_details.h"
-#include "content/public/browser/notification_service.h"
 #include "content/public/browser/plugin_service.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/common/process_type.h"
 #include "content/public/common/result_codes.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 #include "grit/locale_settings.h"
 #include "grit/theme_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 
@@ skipping 221 matching lines @@
 
 HungPluginTabHelper::PluginState::~PluginState() {
 }
 
 
 // HungPluginTabHelper --------------------------------------------------------
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(HungPluginTabHelper);
 
 HungPluginTabHelper::HungPluginTabHelper(content::WebContents* contents)
-    : content::WebContentsObserver(contents) {
-  registrar_.Add(this, chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED,
-                 content::NotificationService::AllSources());
+    : content::WebContentsObserver(contents),
+      number_of_infobars_(0) {
 }
 
 HungPluginTabHelper::~HungPluginTabHelper() {

[tfarina, 2014/06/07 05:26:21]

Peter, I got the backtrace for this.

It is this (beautiful) thing:

Breakpoint 1, HungPluginTabHelper::~HungPluginTabHelper (this=0x3390eeb12bd0) at
../../chrome/browser/ui/hung_plugin_tab_helper.cc:265
265	HungPluginTabHelper::~HungPluginTabHelper() {
(gdb) bt
#0  HungPluginTabHelper::~HungPluginTabHelper (this=0x3390eeb12bd0) at
../../chrome/browser/ui/hung_plugin_tab_helper.cc:265
#1  0x0000555557acd58c in non-virtual thunk to
HungPluginTabHelper::~HungPluginTabHelper() ()
    at ../../chrome/browser/ui/hung_plugin_tab_helper.cc:267
#2  0x00007ffff5eeb837 in linked_ptr<base::SupportsUserData::Data>::depart
(this=0x3390eeaf4eb8) at ../../base/memory/linked_ptr.h:146
#3  0x00007ffff5eea965 in linked_ptr<base::SupportsUserData::Data>::~linked_ptr
(this=0x3390eeaf4eb8) at ../../base/memory/linked_ptr.h:84
#4  0x00007ffff5eeb41c in std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> >::~pair (this=0x3390eeaf4eb0)
    at
/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/bits/stl_pair.h:88
#5  0x00007ffff5eeb3ec in std::_Rb_tree_node<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > >::~_Rb_tree_node (
    this=0x3390eeaf4e90) at
/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/bits/stl_tree.h:130
troy<std::_Rb_tree_node<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > > > (this=0x7fffffff8200,
__p=0x3390eeaf4e90)
    at
/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/ext/new_allocator.h:114
#7  0x00007ffff5eeb32c in std::_Rb_tree<void const*, std::pair<void const*
const, linked_ptr<base::SupportsUserData::Data> >,
std::_Select1st<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > >, std::less<void const*>,
std::allocator<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > > >::_M_destroy_node
(this=0x7fffffff8200, __p=0x3390eeaf4e90)
    at
/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/bits/stl_tree.h:419
#8  0x00007ffff5eed2db in std::_Rb_tree<void const*, std::pair<void const*
const, linked_ptr<base::SupportsUserData::Data> >,
std::_Select1st<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > >, std::less<void const*>,
std::allocator<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > > >::_M_erase (this=0x7fffffff8200,
__x=0x3390eeaf4e90)
    at
/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/bits/stl_tree.h:1084
#9  0x00007ffff5eed2be in std::_Rb_tree<void const*, std::pair<void const*
const, linked_ptr<base::SupportsUserData::Data> >,
std::_Select1st<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > >, std::less<void const*>,
std::allocator<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > > >::_M_erase (this=0x7fffffff8200,
__x=0x3390ee68e100)
    at
/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/bits/stl_tree.h:1082
#10 0x00007ffff5eed265 in std::_Rb_tree<void const*, std::pair<void const*
const, linked_ptr<base::SupportsUserData::Data> >,
std::_Select1st<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > >, std::less<void const*>,
std::allocator<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > > >::~_Rb_tree (this=0x7fffffff8200)
    at
/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/bits/stl_tree.h:646
#11 0x00007ffff5eed235 in std::__cxx1998::map<void const*,
linked_ptr<base::SupportsUserData::Data>, std::less<void const*>,
std::allocator<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > > >::~map (this=0x7fffffff8200)
    at
/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/bits/stl_map.h:90
#12 0x00007ffff5eeaae1 in std::__debug::map<void const*,
linked_ptr<base::SupportsUserData::Data>, std::less<void const*>,
std::allocator<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > > >::~map (this=0x7fffffff8200)
    at
/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/debug/map.h:107
#13 0x00007ffff5eea30e in base::SupportsUserData::~SupportsUserData
(this=0x3390eda13230) at ../../base/supports_user_data.cc:43
---Type <return> to continue, or q <return> to quit---
#14 0x00007fffe93ec065 in content::WebContents::~WebContents
(this=0x3390eda13220) at ../../content/public/browser/web_contents.h:149
#15 0x00007fffe93ade4c in content::WebContentsImpl::~WebContentsImpl
(this=0x3390eda13220)
    at ../../content/browser/web_contents/web_contents_impl.cc:426
#16 0x00007fffe93ae1a9 in content::WebContentsImpl::~WebContentsImpl
(this=0x3390eda13220)
    at ../../content/browser/web_contents/web_contents_impl.cc:358
[...]

+  DCHECK_EQ(0, number_of_infobars_);

[Peter Kasting, 2014/06/03 18:41:45]

Are you sure we're guaranteed that the InfoBarManager is destroyed before the
HungPluginTabHelper?  Because if the order could go either way, this DCHECK
isn't right.

It would probably help me to know what the callstacks are that lead to these
objects' destruction, so it's clear that one must be destroyed later than the
other.

We should also have a comment here describing why this DCHECK holds.

 }
 
 void HungPluginTabHelper::PluginCrashed(const base::FilePath& plugin_path,
                                         base::ProcessId plugin_pid) {
   // TODO(brettw) ideally this would take the child process ID. When we do this
   // for NaCl plugins, we'll want to know exactly which process it was since
   // the path won't be useful.
   InfoBarService* infobar_service =
       InfoBarService::FromWebContents(web_contents());
   if (!infobar_service)
@@ skipping 34 matching lines @@
 
   base::string16 plugin_name =
       content::PluginService::GetInstance()->GetPluginDisplayNameByPath(
           plugin_path);
 
   linked_ptr<PluginState> state(new PluginState(plugin_path, plugin_name));
   hung_plugins_[plugin_child_id] = state;
   ShowBar(plugin_child_id, state.get());
 }
 
-void HungPluginTabHelper::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  DCHECK_EQ(chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED, type);
-  infobars::InfoBar* infobar =
-      content::Details<infobars::InfoBar::RemovedDetails>(details)->first;
+void HungPluginTabHelper::OnInfoBarRemoved(infobars::InfoBar* infobar,
+                                           bool animate) {
+  --number_of_infobars_;

[Peter Kasting, 2014/06/03 18:41:45]

This is not safe.  You get called on every kind of infobar removal, and you need
to be sure that you're being called about a hung plugin infobar before you act.

In other words, all this code needs to move inside the conditional below where
you find that this infobar is one of the infobars you're tracking.

[tfarina, 2014/06/07 05:26:21]

Done.

+

[Peter Kasting, 2014/06/03 18:41:45]

Nit: I'd avoid this newline (2 places)

[tfarina, 2014/06/07 05:26:21]

Done.

+  if (number_of_infobars_ == 0)
+    InfoBarService::FromWebContents(web_contents())->RemoveObserver(this);
+
   for (PluginStateMap::iterator i = hung_plugins_.begin();
        i != hung_plugins_.end(); ++i) {
     PluginState* state = i->second.get();
     if (state->infobar == infobar) {
       state->infobar = NULL;
 
       // Schedule the timer to re-show the infobar if the plugin continues to be
       // hung.
       state->timer.Start(FROM_HERE, state->next_reshow_delay,
           base::Bind(&HungPluginTabHelper::OnReshowTimer,
@@ skipping 64 matching lines @@
 
 void HungPluginTabHelper::ShowBar(int child_id, PluginState* state) {
   InfoBarService* infobar_service =
       InfoBarService::FromWebContents(web_contents());
   if (!infobar_service)
     return;
 
   DCHECK(!state->infobar);
   state->infobar = HungPluginInfoBarDelegate::Create(infobar_service, this,
                                                      child_id, state->name);
+
+  if (state->infobar) {
+    ++number_of_infobars_;
+
+    if (number_of_infobars_ == 1)
+      infobar_service->AddObserver(this);
+  }
 }
 
 void HungPluginTabHelper::CloseBar(PluginState* state) {
   InfoBarService* infobar_service =
       InfoBarService::FromWebContents(web_contents());
   if (infobar_service && state->infobar) {
     infobar_service->RemoveInfoBar(state->infobar);
     state->infobar = NULL;
   }
 }