Commit e3da3b00 authored by Charlie Harrison's avatar Charlie Harrison Committed by Commit Bot

ContentSettingBubbleModel: ensure destruction on WebContentsDestroyed

ContentSettingBubbleContents owns the underlying bubble model. It is
also a WebContentsObserver. This CL destroys the bubble model in
WebContentsDestroyed, which allows the model to avoid paranoid
null checking throughout the code.

In the process, we also remove:
1. Notification observation about profile / web contents destruction.
   The profile should be alive as long as the web contents.

2. Storing the profile directly and accessing the raw ptr. Instead,
   just pull it off the web contents.

One subtle thing about this patch. On WebContentsDestroyed, we do not
synchronously destroy the underlying ContentSettingBubbleContents,
instead, we synchronously hide it and post a task which actually
deletes it. This means we need to be careful about properly null
checking the content_setting_bubble_model_. Thankfully, we can assume
that event listeners are not invoked on hidden widgets.

Bug: None
Change-Id: I0a1ff38a705ace8a9898a4c89f81d51d8474b4da
Reviewed-on: https://chromium-review.googlesource.com/c/1334097
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: default avatarBalazs Engedy <engedy@chromium.org>
Reviewed-by: default avatarAvi Drissman <avi@chromium.org>
Reviewed-by: default avatarRobert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610852}
parent 6b8f64da
......@@ -23,8 +23,6 @@
#include "chrome/common/custom_handlers/protocol_handler.h"
#include "components/content_settings/core/common/content_settings.h"
#include "components/content_settings/core/common/content_settings_types.h"
#include "content/public/browser/notification_observer.h"
#include "content/public/browser/notification_registrar.h"
#include "content/public/common/media_stream_request.h"
#include "ui/gfx/image/image.h"
#include "url/gurl.h"
......@@ -67,7 +65,7 @@ class ContentSettingFramebustBlockBubbleModel;
// This model provides data for ContentSettingBubble, and also controls
// the action triggered when the allow / block radio buttons are triggered.
class ContentSettingBubbleModel : public content::NotificationObserver {
class ContentSettingBubbleModel {
public:
typedef ContentSettingBubbleModelDelegate Delegate;
......@@ -167,20 +165,14 @@ class ContentSettingBubbleModel : public content::NotificationObserver {
static std::unique_ptr<ContentSettingBubbleModel>
CreateContentSettingBubbleModel(Delegate* delegate,
content::WebContents* web_contents,
Profile* profile,
ContentSettingsType content_type);
~ContentSettingBubbleModel() override;
virtual ~ContentSettingBubbleModel();
const BubbleContent& bubble_content() const { return bubble_content_; }
void set_owner(Owner* owner) { owner_ = owner; }
// content::NotificationObserver:
void Observe(int type,
const content::NotificationSource& source,
const content::NotificationDetails& details) override;
virtual void OnListItemClicked(int index, int event_flags) {}
virtual void OnCustomLinkClicked() {}
virtual void OnManageButtonClicked() {}
......@@ -221,13 +213,13 @@ class ContentSettingBubbleModel : public content::NotificationObserver {
}
protected:
ContentSettingBubbleModel(
Delegate* delegate,
content::WebContents* web_contents,
Profile* profile);
// |web_contents| must outlive this.
ContentSettingBubbleModel(Delegate* delegate,
content::WebContents* web_contents);
// Should always be non-nullptr.
content::WebContents* web_contents() const { return web_contents_; }
Profile* profile() const { return profile_; }
Profile* GetProfile() const;
Delegate* delegate() const { return delegate_; }
int selected_item() const { return owner_->GetSelectedRadioOption(); }
......@@ -274,12 +266,9 @@ class ContentSettingBubbleModel : public content::NotificationObserver {
private:
content::WebContents* web_contents_;
Profile* profile_;
Owner* owner_;
Delegate* delegate_;
BubbleContent bubble_content_;
// A registrar for listening for WEB_CONTENTS_DESTROYED notifications.
content::NotificationRegistrar registrar_;
// The service used to record Rappor metrics. Can be set for testing.
rappor::RapporServiceImpl* rappor_service_;
......@@ -291,7 +280,6 @@ class ContentSettingSimpleBubbleModel : public ContentSettingBubbleModel {
public:
ContentSettingSimpleBubbleModel(Delegate* delegate,
content::WebContents* web_contents,
Profile* profile,
ContentSettingsType content_type);
ContentSettingsType content_type() { return content_type_; }
......@@ -320,7 +308,6 @@ class ContentSettingRPHBubbleModel : public ContentSettingSimpleBubbleModel {
public:
ContentSettingRPHBubbleModel(Delegate* delegate,
content::WebContents* web_contents,
Profile* profile,
ProtocolHandlerRegistry* registry);
~ContentSettingRPHBubbleModel() override;
......@@ -345,8 +332,7 @@ class ContentSettingRPHBubbleModel : public ContentSettingSimpleBubbleModel {
class ContentSettingMediaStreamBubbleModel : public ContentSettingBubbleModel {
public:
ContentSettingMediaStreamBubbleModel(Delegate* delegate,
content::WebContents* web_contents,
Profile* profile);
content::WebContents* web_contents);
~ContentSettingMediaStreamBubbleModel() override;
......@@ -402,9 +388,9 @@ class ContentSettingMediaStreamBubbleModel : public ContentSettingBubbleModel {
class ContentSettingSubresourceFilterBubbleModel
: public ContentSettingBubbleModel {
public:
ContentSettingSubresourceFilterBubbleModel(Delegate* delegate,
content::WebContents* web_contents,
Profile* profile);
ContentSettingSubresourceFilterBubbleModel(
Delegate* delegate,
content::WebContents* web_contents);
~ContentSettingSubresourceFilterBubbleModel() override;
......@@ -429,8 +415,7 @@ class ContentSettingSubresourceFilterBubbleModel
class ContentSettingDownloadsBubbleModel : public ContentSettingBubbleModel {
public:
ContentSettingDownloadsBubbleModel(Delegate* delegate,
content::WebContents* web_contents,
Profile* profile);
content::WebContents* web_contents);
~ContentSettingDownloadsBubbleModel() override;
// ContentSettingBubbleModel overrides:
......@@ -452,7 +437,6 @@ class ContentSettingSingleRadioGroup : public ContentSettingSimpleBubbleModel {
public:
ContentSettingSingleRadioGroup(Delegate* delegate,
content::WebContents* web_contents,
Profile* profile,
ContentSettingsType content_type);
~ContentSettingSingleRadioGroup() override;
......@@ -482,16 +466,10 @@ class ContentSettingFramebustBlockBubbleModel
public UrlListManager::Observer {
public:
ContentSettingFramebustBlockBubbleModel(Delegate* delegate,
content::WebContents* web_contents,
Profile* profile);
content::WebContents* web_contents);
~ContentSettingFramebustBlockBubbleModel() override;
// content::NotificationObserver:
void Observe(int type,
const content::NotificationSource& source,
const content::NotificationDetails& details) override;
// ContentSettingBubbleModel:
void OnListItemClicked(int index, int event_flags) override;
ContentSettingFramebustBlockBubbleModel* AsFramebustBlockBubbleModel()
......
......@@ -68,7 +68,7 @@ IN_PROC_BROWSER_TEST_F(ContentSettingBubbleModelMixedScriptTest, MainFrame) {
ContentSettingBubbleModel::CreateContentSettingBubbleModel(
browser()->content_setting_bubble_model_delegate(),
browser()->tab_strip_model()->GetActiveWebContents(),
browser()->profile(), CONTENT_SETTINGS_TYPE_MIXEDSCRIPT));
CONTENT_SETTINGS_TYPE_MIXEDSCRIPT));
model->OnCustomLinkClicked();
// Wait for reload
......@@ -128,7 +128,7 @@ IN_PROC_BROWSER_TEST_F(ContentSettingsMixedScriptIgnoreCertErrorsTest,
ContentSettingBubbleModel::CreateContentSettingBubbleModel(
browser()->content_setting_bubble_model_delegate(),
browser()->tab_strip_model()->GetActiveWebContents(),
browser()->profile(), CONTENT_SETTINGS_TYPE_MIXEDSCRIPT));
CONTENT_SETTINGS_TYPE_MIXEDSCRIPT));
model->SetRapporServiceImplForTesting(&rappor_service);
model->OnCustomLinkClicked();
......@@ -187,8 +187,7 @@ class ContentSettingBubbleModelMediaStreamTest : public InProcessBrowserTest {
state, std::string(), std::string(), std::string(), std::string());
std::unique_ptr<ContentSettingBubbleModel> bubble(
new ContentSettingMediaStreamBubbleModel(
browser()->content_setting_bubble_model_delegate(), original_tab,
browser()->profile()));
browser()->content_setting_bubble_model_delegate(), original_tab));
// Click the manage button, which opens in a new tab or window. Wait until
// it loads.
......@@ -264,7 +263,7 @@ IN_PROC_BROWSER_TEST_F(ContentSettingBubbleModelPopupTest,
ContentSettingBubbleModel::CreateContentSettingBubbleModel(
browser()->content_setting_bubble_model_delegate(),
browser()->tab_strip_model()->GetActiveWebContents(),
browser()->profile(), CONTENT_SETTINGS_TYPE_POPUPS));
CONTENT_SETTINGS_TYPE_POPUPS));
std::unique_ptr<FakeOwner> owner =
FakeOwner::Create(*model, kDisallowButtonIndex);
......@@ -341,7 +340,7 @@ IN_PROC_BROWSER_TEST_F(ContentSettingBubbleModelMixedScriptOopifTest,
std::unique_ptr<ContentSettingBubbleModel> model(
ContentSettingBubbleModel::CreateContentSettingBubbleModel(
browser()->content_setting_bubble_model_delegate(), web_contents,
browser()->profile(), CONTENT_SETTINGS_TYPE_MIXEDSCRIPT));
CONTENT_SETTINGS_TYPE_MIXEDSCRIPT));
model->OnCustomLinkClicked();
// Wait for reload and verify that mixed content is allowed.
......
......@@ -126,8 +126,7 @@ class ContentSettingMediaImageModel : public ContentSettingImageModel {
std::unique_ptr<ContentSettingBubbleModel> CreateBubbleModelImpl(
ContentSettingBubbleModel::Delegate* delegate,
WebContents* web_contents,
Profile* profile) override;
WebContents* web_contents) override;
private:
DISALLOW_COPY_AND_ASSIGN(ContentSettingMediaImageModel);
......@@ -215,13 +214,9 @@ ContentSettingSimpleImageModel::ContentSettingSimpleImageModel(
std::unique_ptr<ContentSettingBubbleModel>
ContentSettingSimpleImageModel::CreateBubbleModelImpl(
ContentSettingBubbleModel::Delegate* delegate,
WebContents* web_contents,
Profile* profile) {
WebContents* web_contents) {
return ContentSettingBubbleModel::CreateContentSettingBubbleModel(
delegate,
web_contents,
profile,
content_type());
delegate, web_contents, content_type());
}
// static
......@@ -548,10 +543,9 @@ bool ContentSettingMediaImageModel::UpdateAndGetVisibility(
std::unique_ptr<ContentSettingBubbleModel>
ContentSettingMediaImageModel::CreateBubbleModelImpl(
ContentSettingBubbleModel::Delegate* delegate,
WebContents* web_contents,
Profile* profile) {
return std::make_unique<ContentSettingMediaStreamBubbleModel>(
delegate, web_contents, profile);
WebContents* web_contents) {
return std::make_unique<ContentSettingMediaStreamBubbleModel>(delegate,
web_contents);
}
// Blocked Framebust -----------------------------------------------------------
......@@ -573,10 +567,9 @@ bool ContentSettingFramebustBlockImageModel::UpdateAndGetVisibility(
std::unique_ptr<ContentSettingBubbleModel>
ContentSettingFramebustBlockImageModel::CreateBubbleModelImpl(
ContentSettingBubbleModel::Delegate* delegate,
WebContents* web_contents,
Profile* profile) {
WebContents* web_contents) {
return std::make_unique<ContentSettingFramebustBlockBubbleModel>(
delegate, web_contents, profile);
delegate, web_contents);
}
// Sensors ---------------------------------------------------------------------
......@@ -639,12 +632,12 @@ ContentSettingImageModel::ContentSettingImageModel(ImageType image_type)
std::unique_ptr<ContentSettingBubbleModel>
ContentSettingImageModel::CreateBubbleModel(
ContentSettingBubbleModel::Delegate* delegate,
content::WebContents* web_contents,
Profile* profile) {
content::WebContents* web_contents) {
DCHECK(web_contents);
UMA_HISTOGRAM_ENUMERATION(
"ContentSettings.ImagePressed", image_type(),
ContentSettingImageModel::ImageType::NUM_IMAGE_TYPES);
return CreateBubbleModelImpl(delegate, web_contents, profile);
return CreateBubbleModelImpl(delegate, web_contents);
}
// static
......
......@@ -72,8 +72,7 @@ class ContentSettingImageModel {
// Creates the model for the bubble that will be attached to this image.
std::unique_ptr<ContentSettingBubbleModel> CreateBubbleModel(
ContentSettingBubbleModel::Delegate* delegate,
content::WebContents* web_contents,
Profile* profile);
content::WebContents* web_contents);
// Whether the animation should be run for the given |web_contents|.
bool ShouldRunAnimation(content::WebContents* web_contents);
......@@ -106,8 +105,7 @@ class ContentSettingImageModel {
// Internal implementation by subclasses of bubble model creation.
virtual std::unique_ptr<ContentSettingBubbleModel> CreateBubbleModelImpl(
ContentSettingBubbleModel::Delegate* delegate,
content::WebContents* web_contents,
Profile* profile) = 0;
content::WebContents* web_contents) = 0;
void set_icon(const gfx::VectorIcon& icon, const gfx::VectorIcon& badge) {
icon_ = &icon;
......@@ -140,8 +138,7 @@ class ContentSettingSimpleImageModel : public ContentSettingImageModel {
// ContentSettingImageModel implementation.
std::unique_ptr<ContentSettingBubbleModel> CreateBubbleModelImpl(
ContentSettingBubbleModel::Delegate* delegate,
content::WebContents* web_contents,
Profile* profile) override;
content::WebContents* web_contents) override;
ContentSettingsType content_type() { return content_type_; }
......@@ -159,8 +156,7 @@ class ContentSettingFramebustBlockImageModel : public ContentSettingImageModel {
std::unique_ptr<ContentSettingBubbleModel> CreateBubbleModelImpl(
ContentSettingBubbleModel::Delegate* delegate,
content::WebContents* web_contents,
Profile* profile) override;
content::WebContents* web_contents) override;
private:
DISALLOW_COPY_AND_ASSIGN(ContentSettingFramebustBlockImageModel);
......
......@@ -54,11 +54,10 @@ IN_PROC_BROWSER_TEST_F(ContentSettingImageModelBrowserTest, CreateBubbleModel) {
ImageType::MIDI_SYSEX,
};
Profile* profile = browser()->profile();
for (auto type : content_settings_to_test) {
auto model = ContentSettingImageModel::CreateForContentType(type);
std::unique_ptr<ContentSettingBubbleModel> bubble(
model->CreateBubbleModel(nullptr, web_contents, profile));
model->CreateBubbleModel(nullptr, web_contents));
// All of the above content settings should create a
// ContentSettingSimpleBubbleModel that is tied to a particular setting,
......@@ -78,7 +77,7 @@ IN_PROC_BROWSER_TEST_F(ContentSettingImageModelBrowserTest, CreateBubbleModel) {
std::vector<std::unique_ptr<ContentSettingImageModel>> models =
ContentSettingImageModel::GenerateContentSettingImageModels();
for (auto& model : models) {
EXPECT_TRUE(model->CreateBubbleModel(nullptr, web_contents, profile));
EXPECT_TRUE(model->CreateBubbleModel(nullptr, web_contents));
EXPECT_TRUE(image_types.insert(model->image_type()).second);
}
}
......@@ -116,10 +115,8 @@ IN_PROC_BROWSER_TEST_F(ContentSettingImageModelBrowserTest,
browser()->tab_strip_model()->GetActiveWebContents();
auto model = ContentSettingImageModel::CreateForContentType(ImageType::ADS);
Profile* profile = browser()->profile();
std::unique_ptr<ContentSettingBubbleModel> bubble(model->CreateBubbleModel(
browser()->content_setting_bubble_model_delegate(), web_contents,
profile));
browser()->content_setting_bubble_model_delegate(), web_contents));
content::TestNavigationObserver observer(nullptr);
observer.StartWatchingNewWebContents();
......
......@@ -128,8 +128,7 @@ IN_PROC_BROWSER_TEST_F(FramebustBlockBrowserTest, ModelAllowsRedirection) {
// Simulate clicking on the second blocked URL.
ContentSettingFramebustBlockBubbleModel framebust_block_bubble_model(
browser()->content_setting_bubble_model_delegate(), GetWebContents(),
browser()->profile());
browser()->content_setting_bubble_model_delegate(), GetWebContents());
EXPECT_FALSE(clicked_index_.has_value());
EXPECT_FALSE(clicked_url_.has_value());
......@@ -166,8 +165,7 @@ IN_PROC_BROWSER_TEST_F(FramebustBlockBrowserTest, AllowRadioButtonSelected) {
// Create a content bubble and simulate clicking on the first radio button
// before closing it.
ContentSettingFramebustBlockBubbleModel framebust_block_bubble_model(
browser()->content_setting_bubble_model_delegate(), GetWebContents(),
browser()->profile());
browser()->content_setting_bubble_model_delegate(), GetWebContents());
std::unique_ptr<FakeOwner> owner = FakeOwner::Create(
framebust_block_bubble_model, kDisallowRadioButtonIndex);
......@@ -197,8 +195,7 @@ IN_PROC_BROWSER_TEST_F(FramebustBlockBrowserTest, DisallowRadioButtonSelected) {
// Create a content bubble and simulate clicking on the second radio button
// before closing it.
ContentSettingFramebustBlockBubbleModel framebust_block_bubble_model(
browser()->content_setting_bubble_model_delegate(), GetWebContents(),
browser()->profile());
browser()->content_setting_bubble_model_delegate(), GetWebContents());
std::unique_ptr<FakeOwner> owner =
FakeOwner::Create(framebust_block_bubble_model, kAllowRadioButtonIndex);
......@@ -223,8 +220,7 @@ IN_PROC_BROWSER_TEST_F(FramebustBlockBrowserTest, ManageButtonClicked) {
// Create a content bubble and simulate clicking on the second radio button
// before closing it.
ContentSettingFramebustBlockBubbleModel framebust_block_bubble_model(
browser()->content_setting_bubble_model_delegate(), GetWebContents(),
browser()->profile());
browser()->content_setting_bubble_model_delegate(), GetWebContents());
content::TestNavigationObserver navigation_observer(nullptr);
navigation_observer.StartWatchingNewWebContents();
......
......@@ -383,7 +383,8 @@ ContentSettingBubbleContents::~ContentSettingBubbleContents() {
}
void ContentSettingBubbleContents::WindowClosing() {
content_setting_bubble_model_->CommitChanges();
if (content_setting_bubble_model_)
content_setting_bubble_model_->CommitChanges();
}
gfx::Size ContentSettingBubbleContents::CalculatePreferredSize() const {
......@@ -424,6 +425,8 @@ void ContentSettingBubbleContents::OnNativeThemeChanged(
}
base::string16 ContentSettingBubbleContents::GetWindowTitle() const {
if (!content_setting_bubble_model_)
return base::string16();
return content_setting_bubble_model_->bubble_content().title;
}
......@@ -432,6 +435,7 @@ bool ContentSettingBubbleContents::ShouldShowCloseButton() const {
}
void ContentSettingBubbleContents::Init() {
DCHECK(content_setting_bubble_model_);
const ChromeLayoutProvider* provider = ChromeLayoutProvider::Get();
SetLayoutManager(std::make_unique<views::BoxLayout>(
views::BoxLayout::kVertical, gfx::Insets(),
......@@ -537,6 +541,7 @@ void ContentSettingBubbleContents::Init() {
}
views::View* ContentSettingBubbleContents::CreateExtraView() {
DCHECK(content_setting_bubble_model_);
const auto& bubble_content = content_setting_bubble_model_->bubble_content();
const auto* layout = ChromeLayoutProvider::Get();
std::vector<View*> extra_views;
......@@ -589,6 +594,9 @@ int ContentSettingBubbleContents::GetDialogButtons() const {
base::string16 ContentSettingBubbleContents::GetDialogButtonLabel(
ui::DialogButton button) const {
if (!content_setting_bubble_model_)
return base::string16();
const base::string16& done_text =
content_setting_bubble_model_->bubble_content().done_button_text;
return done_text.empty() ? l10n_util::GetStringUTF16(IDS_DONE) : done_text;
......@@ -620,11 +628,20 @@ void ContentSettingBubbleContents::OnVisibilityChanged(
}
void ContentSettingBubbleContents::WebContentsDestroyed() {
// Destroy the bubble model to ensure that the underlying WebContents outlives
// it.
content_setting_bubble_model_->CommitChanges();
content_setting_bubble_model_.reset();
// Closing the widget should synchronously hide it (and post a task to delete
// it). Subsequent event listener methods should not be invoked on hidden
// widgets.
GetWidget()->Close();
}
void ContentSettingBubbleContents::ButtonPressed(views::Button* sender,
const ui::Event& event) {
DCHECK(content_setting_bubble_model_);
if (sender == manage_checkbox_) {
content_setting_bubble_model_->OnManageCheckboxChecked(
manage_checkbox_->checked());
......@@ -645,6 +662,7 @@ void ContentSettingBubbleContents::ButtonPressed(views::Button* sender,
void ContentSettingBubbleContents::LinkClicked(views::Link* source,
int event_flags) {
DCHECK(content_setting_bubble_model_);
if (source == custom_link_) {
content_setting_bubble_model_->OnCustomLinkClicked();
GetWidget()->Close();
......@@ -656,6 +674,7 @@ void ContentSettingBubbleContents::LinkClicked(views::Link* source,
}
void ContentSettingBubbleContents::OnPerformAction(views::Combobox* combobox) {
DCHECK(content_setting_bubble_model_);
MediaComboboxModel* model =
static_cast<MediaComboboxModel*>(combobox->model());
content_setting_bubble_model_->OnMediaMenuClicked(
......
......@@ -7,7 +7,6 @@
#include <utility>
#include "base/strings/utf_string_conversions.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/themes/theme_properties.h"
#include "chrome/browser/ui/content_settings/content_setting_bubble_model.h"
#include "chrome/browser/ui/content_settings/content_setting_image_model.h"
......@@ -129,8 +128,7 @@ bool ContentSettingImageView::ShowBubble(const ui::Event& event) {
views::View* const anchor = parent();
bubble_view_ = new ContentSettingBubbleContents(
content_setting_image_model_->CreateBubbleModel(
delegate_->GetContentSettingBubbleModelDelegate(), web_contents,
Profile::FromBrowserContext(web_contents->GetBrowserContext())),
delegate_->GetContentSettingBubbleModelDelegate(), web_contents),
web_contents, anchor, views::BubbleBorder::TOP_RIGHT);
bubble_view_->SetHighlightedButton(this);
views::Widget* bubble_widget =
......
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