Commit 5adb5e2a authored by Avi Drissman's avatar Avi Drissman Committed by Commit Bot

Don't crash on infobar teardown.

There could be an ordering issue with the teardown
of a tab with an infobar after the infobar change
in r547527. Watch the "infobar manager gone" callback
and ensure a correct order.

For correctness, this makes the change in all the
other places that change used the infobar observer
except for InfoBarObserver, which is changed this
way in https://crrev.com/c/991468 .

BUG=354380, 828552

Change-Id: I21add007c5b2d725dc2985cbd268d3d4674978b6
Reviewed-on: https://chromium-review.googlesource.com/996372Reviewed-by: default avatarPeter Kasting <pkasting@chromium.org>
Reviewed-by: default avatarScott Violet <sky@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548246}
parent 450e9d1e
...@@ -13,6 +13,7 @@ ...@@ -13,6 +13,7 @@
#include "base/command_line.h" #include "base/command_line.h"
#include "base/macros.h" #include "base/macros.h"
#include "base/run_loop.h" #include "base/run_loop.h"
#include "base/scoped_observer.h"
#include "base/stl_util.h" #include "base/stl_util.h"
#include "base/strings/stringprintf.h" #include "base/strings/stringprintf.h"
#include "build/build_config.h" #include "build/build_config.h"
...@@ -213,7 +214,8 @@ class TranslateManagerRenderViewHostTest ...@@ -213,7 +214,8 @@ class TranslateManagerRenderViewHostTest
TranslateManagerRenderViewHostTest() TranslateManagerRenderViewHostTest()
: pref_callback_( : pref_callback_(
base::Bind(&TranslateManagerRenderViewHostTest::OnPreferenceChanged, base::Bind(&TranslateManagerRenderViewHostTest::OnPreferenceChanged,
base::Unretained(this))) {} base::Unretained(this))),
infobar_observer_(this) {}
#if !defined(USE_AURA) #if !defined(USE_AURA)
// Ensure that we are testing under the bubble UI. // Ensure that we are testing under the bubble UI.
...@@ -450,6 +452,10 @@ class TranslateManagerRenderViewHostTest ...@@ -450,6 +452,10 @@ class TranslateManagerRenderViewHostTest
removed_infobars_.insert(infobar->delegate()); removed_infobars_.insert(infobar->delegate());
} }
void OnManagerShuttingDown(infobars::InfoBarManager* manager) override {
infobar_observer_.Remove(manager);
}
MOCK_METHOD1(OnPreferenceChanged, void(const std::string&)); MOCK_METHOD1(OnPreferenceChanged, void(const std::string&));
protected: protected:
...@@ -476,11 +482,11 @@ class TranslateManagerRenderViewHostTest ...@@ -476,11 +482,11 @@ class TranslateManagerRenderViewHostTest
->translate_driver() ->translate_driver()
.set_translate_max_reload_attempts(0); .set_translate_max_reload_attempts(0);
infobar_service()->AddObserver(this); infobar_observer_.Add(infobar_service());
} }
virtual void TearDown() { virtual void TearDown() {
infobar_service()->RemoveObserver(this); infobar_observer_.Remove(infobar_service());
ChromeRenderViewHostTestHarness::TearDown(); ChromeRenderViewHostTestHarness::TearDown();
TranslateService::ShutdownForTesting(); TranslateService::ShutdownForTesting();
...@@ -542,6 +548,9 @@ class TranslateManagerRenderViewHostTest ...@@ -542,6 +548,9 @@ class TranslateManagerRenderViewHostTest
std::unique_ptr<MockTranslateBubbleFactory> bubble_factory_; std::unique_ptr<MockTranslateBubbleFactory> bubble_factory_;
FakePageImpl fake_page_; FakePageImpl fake_page_;
ScopedObserver<infobars::InfoBarManager, infobars::InfoBarManager::Observer>
infobar_observer_;
DISALLOW_COPY_AND_ASSIGN(TranslateManagerRenderViewHostTest); DISALLOW_COPY_AND_ASSIGN(TranslateManagerRenderViewHostTest);
}; };
......
...@@ -194,6 +194,11 @@ void HungPluginTabHelper::OnInfoBarRemoved(infobars::InfoBar* infobar, ...@@ -194,6 +194,11 @@ void HungPluginTabHelper::OnInfoBarRemoved(infobars::InfoBar* infobar,
} }
} }
void HungPluginTabHelper::OnManagerShuttingDown(
infobars::InfoBarManager* manager) {
infobar_observer_.Remove(manager);
}
void HungPluginTabHelper::KillPlugin(int child_id) { void HungPluginTabHelper::KillPlugin(int child_id) {
PluginStateMap::iterator found = hung_plugins_.find(child_id); PluginStateMap::iterator found = hung_plugins_.find(child_id);
DCHECK(found != hung_plugins_.end()); DCHECK(found != hung_plugins_.end());
......
...@@ -49,6 +49,7 @@ class HungPluginTabHelper ...@@ -49,6 +49,7 @@ class HungPluginTabHelper
// infobars::InfoBarManager::Observer: // infobars::InfoBarManager::Observer:
void OnInfoBarRemoved(infobars::InfoBar* infobar, bool animate) override; void OnInfoBarRemoved(infobars::InfoBar* infobar, bool animate) override;
void OnManagerShuttingDown(infobars::InfoBarManager* manager) override;
// Called by an infobar when the user selects to kill the plugin. // Called by an infobar when the user selects to kill the plugin.
void KillPlugin(int child_id); void KillPlugin(int child_id);
......
...@@ -74,14 +74,13 @@ void PPAPITestMessageHandler::Reset() { ...@@ -74,14 +74,13 @@ void PPAPITestMessageHandler::Reset() {
PPAPITestBase::InfoBarObserver::InfoBarObserver(PPAPITestBase* test_base) PPAPITestBase::InfoBarObserver::InfoBarObserver(PPAPITestBase* test_base)
: test_base_(test_base), : test_base_(test_base),
expecting_infobar_(false), expecting_infobar_(false),
should_accept_(false) { should_accept_(false),
GetInfoBarService()->AddObserver(this); infobar_observer_(this) {
infobar_observer_.Add(GetInfoBarService());
} }
PPAPITestBase::InfoBarObserver::~InfoBarObserver() { PPAPITestBase::InfoBarObserver::~InfoBarObserver() {
EXPECT_FALSE(expecting_infobar_) << "Missing an expected infobar"; EXPECT_FALSE(expecting_infobar_) << "Missing an expected infobar";
GetInfoBarService()->RemoveObserver(this);
} }
void PPAPITestBase::InfoBarObserver::ExpectInfoBarAndAccept( void PPAPITestBase::InfoBarObserver::ExpectInfoBarAndAccept(
...@@ -101,6 +100,11 @@ void PPAPITestBase::InfoBarObserver::OnInfoBarAdded( ...@@ -101,6 +100,11 @@ void PPAPITestBase::InfoBarObserver::OnInfoBarAdded(
base::Bind(&InfoBarObserver::VerifyInfoBarState, base::Unretained(this))); base::Bind(&InfoBarObserver::VerifyInfoBarState, base::Unretained(this)));
} }
void PPAPITestBase::InfoBarObserver::OnManagerShuttingDown(
infobars::InfoBarManager* manager) {
infobar_observer_.Remove(manager);
}
void PPAPITestBase::InfoBarObserver::VerifyInfoBarState() { void PPAPITestBase::InfoBarObserver::VerifyInfoBarState() {
InfoBarService* infobar_service = GetInfoBarService(); InfoBarService* infobar_service = GetInfoBarService();
EXPECT_EQ(expecting_infobar_ ? 1U : 0U, infobar_service->infobar_count()); EXPECT_EQ(expecting_infobar_ ? 1U : 0U, infobar_service->infobar_count());
......
...@@ -9,6 +9,7 @@ ...@@ -9,6 +9,7 @@
#include "base/compiler_specific.h" #include "base/compiler_specific.h"
#include "base/macros.h" #include "base/macros.h"
#include "base/scoped_observer.h"
#include "chrome/test/base/in_process_browser_test.h" #include "chrome/test/base/in_process_browser_test.h"
#include "components/infobars/core/infobar_manager.h" #include "components/infobars/core/infobar_manager.h"
#include "content/public/test/javascript_test_observer.h" #include "content/public/test/javascript_test_observer.h"
...@@ -66,6 +67,7 @@ class PPAPITestBase : public InProcessBrowserTest { ...@@ -66,6 +67,7 @@ class PPAPITestBase : public InProcessBrowserTest {
private: private:
// infobars::InfoBarManager::Observer: // infobars::InfoBarManager::Observer:
void OnInfoBarAdded(infobars::InfoBar* infobar) override; void OnInfoBarAdded(infobars::InfoBar* infobar) override;
void OnManagerShuttingDown(infobars::InfoBarManager* manager) override;
InfoBarService* GetInfoBarService(); InfoBarService* GetInfoBarService();
...@@ -74,6 +76,9 @@ class PPAPITestBase : public InProcessBrowserTest { ...@@ -74,6 +76,9 @@ class PPAPITestBase : public InProcessBrowserTest {
PPAPITestBase* test_base_; PPAPITestBase* test_base_;
bool expecting_infobar_; bool expecting_infobar_;
bool should_accept_; bool should_accept_;
ScopedObserver<infobars::InfoBarManager, infobars::InfoBarManager::Observer>
infobar_observer_;
}; };
// Runs the test for a tab given the tab that's already navigated to the // Runs the test for a tab given the tab that's already navigated to the
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment