Skip to content

Commit

Permalink
Fix crash when automation infobar is closed
Browse files Browse the repository at this point in the history
When Chrome is started with --enable-automation switch (e.g., when it
is started by ChromeDriver), it display an infobar indicating the fact.
If the user attempts to close this infobar, it used to crash Chrome.
Closing other global infobars when --enable-automation switch is used
can also crash Chrome.

BUG=706735
NOPRESUBMIT=true
NOTRY=true

Review-Url: https://codereview.chromium.org/2807403002
Cr-Original-Commit-Position: refs/heads/master@{#464676}
Review-Url: https://codereview.chromium.org/2816373002
Cr-Commit-Position: refs/branch-heads/3071@{#53}
Cr-Branched-From: a106f0a-refs/heads/master@{#464641}
  • Loading branch information
JohnChen0 authored and Commit bot committed Apr 19, 2017
1 parent 4d5dad8 commit 338ccfc
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 8 deletions.
19 changes: 16 additions & 3 deletions chrome/browser/devtools/global_confirm_info_bar.cc
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,15 @@ base::string16 GlobalConfirmInfoBar::DelegateProxy::GetButtonLabel(

bool GlobalConfirmInfoBar::DelegateProxy::Accept() {
base::WeakPtr<GlobalConfirmInfoBar> info_bar = global_info_bar_;
if (info_bar)
// Remove the current InfoBar (the one whose Accept button is being clicked)
// from the control of GlobalConfirmInfoBar. This InfoBar will be closed by
// caller of this method, and we don't need GlobalConfirmInfoBar to close it.
// Furthermore, letting GlobalConfirmInfoBar close the current InfoBar can
// cause memory corruption when InfoBar animation is disabled.
if (info_bar) {
info_bar->OnInfoBarRemoved(info_bar_, false);
info_bar->delegate_->Accept();
}
// Could be destroyed after this point.
if (info_bar)
info_bar->Close();
Expand All @@ -86,8 +93,11 @@ bool GlobalConfirmInfoBar::DelegateProxy::Accept() {

bool GlobalConfirmInfoBar::DelegateProxy::Cancel() {
base::WeakPtr<GlobalConfirmInfoBar> info_bar = global_info_bar_;
if (info_bar)
// See comments in GlobalConfirmInfoBar::DelegateProxy::Accept().
if (info_bar) {
info_bar->OnInfoBarRemoved(info_bar_, false);
info_bar->delegate_->Cancel();
}
// Could be destroyed after this point.
if (info_bar)
info_bar->Close();
Expand Down Expand Up @@ -117,8 +127,11 @@ bool GlobalConfirmInfoBar::DelegateProxy::EqualsDelegate(

void GlobalConfirmInfoBar::DelegateProxy::InfoBarDismissed() {
base::WeakPtr<GlobalConfirmInfoBar> info_bar = global_info_bar_;
if (info_bar)
// See comments in GlobalConfirmInfoBar::DelegateProxy::Accept().
if (info_bar) {
info_bar->OnInfoBarRemoved(info_bar_, false);
info_bar->delegate_->InfoBarDismissed();
}
// Could be destroyed after this point.
if (info_bar)
info_bar->Close();
Expand Down
8 changes: 4 additions & 4 deletions chrome/browser/devtools/global_confirm_info_bar.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ class GlobalConfirmInfoBar : public TabStripModelObserver,
std::unique_ptr<ConfirmInfoBarDelegate> delegate);
void Close();

// infobars::InfoBarManager::Observer:
void OnInfoBarRemoved(infobars::InfoBar* info_bar, bool animate) override;
void OnManagerShuttingDown(infobars::InfoBarManager* manager) override;

private:
explicit GlobalConfirmInfoBar(
std::unique_ptr<ConfirmInfoBarDelegate> delegate);
Expand All @@ -44,10 +48,6 @@ class GlobalConfirmInfoBar : public TabStripModelObserver,
int index,
TabChangeType change_type) override;

// infobars::InfoBarManager::Observer:
void OnInfoBarRemoved(infobars::InfoBar* info_bar, bool animate) override;
void OnManagerShuttingDown(infobars::InfoBarManager* manager) override;

// Adds the info bar to the tab if it is missing.
void MaybeAddInfoBar(content::WebContents* web_contents);

Expand Down
7 changes: 6 additions & 1 deletion chrome/browser/extensions/api/debugger/debugger_apitest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -262,8 +262,13 @@ IN_PROC_BROWSER_TEST_F(DebuggerApiTest, InfoBar) {
EXPECT_EQ(1u, service2->infobar_count());
EXPECT_EQ(1u, service3->infobar_count());

// Closing infobar should cause detach and remove all infobars.
// Calling delegate()->InfoBarDismissed() on a global infobar should
// cause detach and removal of all infobars, except the one used to
// fetch the delegate (i.e., service2->infobar_at(0) itself).
// Afterwards, service2->infobar_at(0) must be explicitly removed.
// See InfoBarView::ButtonPressed for an example.
service2->infobar_at(0)->delegate()->InfoBarDismissed();
service2->infobar_at(0)->RemoveSelf();
EXPECT_EQ(0u, service1->infobar_count());
EXPECT_EQ(0u, service2->infobar_count());
EXPECT_EQ(0u, service3->infobar_count());
Expand Down

0 comments on commit 338ccfc

Please sign in to comment.