372 lines
16 KiB
Diff
372 lines
16 KiB
Diff
From d0c1f5ee1f56c165bdf550c9e3be0d7313587b80 Mon Sep 17 00:00:00 2001
|
|
From: Elly Fong-Jones <ellyjones@chromium.org>
|
|
Date: Wed, 18 Jan 2023 22:33:11 +0000
|
|
Subject: [PATCH] media: untangle MediaRouterUI lifetimes
|
|
|
|
Currently, MediaRouterUI is owned by MediaItemUIDeviceSelectorView.
|
|
There is an observer method named "OnControllerInvalidated" which
|
|
MediaItemUIDeviceSelectorView reacts to by deleting the MediaRouterUI it
|
|
owns. However, OnControllerInvalidated can actually be called in two
|
|
different situations:
|
|
|
|
* From MediaRouterUI::TakeMediaRouteStarter(), in which case the
|
|
MediaRouterUI object is *not* being destroyed, but should be, because
|
|
it can't be safely used after TakeMediaRouteStarter() ends;
|
|
* From MediaRouterUI::~MediaRouterUI(), in which case the MediaRouterUI
|
|
object *is* being destroyed already and should not be.
|
|
|
|
In the second case, only the fact that libc++ nulls out unique_ptr
|
|
before destroying the pointed-to object saves us from a use-after-free;
|
|
under libstdc++, we UaF immediately by re-entering the destructor. Even
|
|
under libc++ though this is still very dangerous, because any observers
|
|
that happened to be registered after MediaItemUIDeviceSelectorView will
|
|
be invoked after the destruction of the object they're observing. Right
|
|
now there are no such other observers, but the fact remains that this
|
|
interface is basically a UaF timebomb.
|
|
|
|
This change separates "this object is about to be destroyed" (an
|
|
observable state) from "please destroy this object, it is no longer
|
|
useful" (a callback that is made to the object's owner) by:
|
|
|
|
1. Renaming OnControllerInvalidated to OnControllerDestroying, to make
|
|
it very clear what is happening to the object, and
|
|
2. Adding a RegisterDestructor method to CastDialogController, which
|
|
allows MediaItemUIDeviceSelectorView to pass a callback into
|
|
MediaRouterUI which MediaRouterUI can use to arrange for its own
|
|
destruction.
|
|
|
|
This is still a bit tangled and ungainly, but it's safe. A fuller
|
|
writeup is on the linked bug.
|
|
|
|
Fixed: 1407202
|
|
Change-Id: Id9410de1fbf2cb42f13957dde316b7c9259f192f
|
|
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4165967
|
|
Reviewed-by: Peter Kasting <pkasting@chromium.org>
|
|
Reviewed-by: Takumi Fujimoto <takumif@chromium.org>
|
|
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
|
|
Cr-Commit-Position: refs/heads/main@{#1094110}
|
|
---
|
|
|
|
diff --git a/chrome/browser/ui/media_router/cast_dialog_controller.h b/chrome/browser/ui/media_router/cast_dialog_controller.h
|
|
index 2a8de976..c3c0553 100644
|
|
--- a/chrome/browser/ui/media_router/cast_dialog_controller.h
|
|
+++ b/chrome/browser/ui/media_router/cast_dialog_controller.h
|
|
@@ -24,10 +24,12 @@
|
|
public:
|
|
virtual ~Observer() = default;
|
|
|
|
- virtual void OnModelUpdated(const CastDialogModel& model) = 0;
|
|
+ virtual void OnModelUpdated(const CastDialogModel& model) {}
|
|
|
|
- // Observer should drop its reference to the controller when this is called.
|
|
- virtual void OnControllerInvalidated() = 0;
|
|
+ // Notifies observers that the observed object is being destroyed. Observers
|
|
+ // MUST NOT try to destroy the observed object in response - to manage the
|
|
+ // lifetime of a CastDialogController, use RegisterDestructor() below.
|
|
+ virtual void OnControllerDestroying() {}
|
|
};
|
|
|
|
virtual ~CastDialogController() = default;
|
|
@@ -55,6 +57,16 @@
|
|
// intended that this API should only be used to transfer ownership to some
|
|
// new component that will want to start casting on this dialog box's behalf.
|
|
virtual std::unique_ptr<MediaRouteStarter> TakeMediaRouteStarter() = 0;
|
|
+
|
|
+ // Registers a callback for when the CastDialogController has given up
|
|
+ // ownership of its MediaRouteStarter and is no longer safe to use. The
|
|
+ // provided closure must destroy |this| or otherwise ensure it is never used
|
|
+ // again. This method can only be called once.
|
|
+ //
|
|
+ // TODO(https://crbug.com/1408494): It's awkward that CastDialogController has
|
|
+ // a state where it exists but is unsafe to use, and doubly awkward that we
|
|
+ // have to paper over that with this callback. Can that be fixed?
|
|
+ virtual void RegisterDestructor(base::OnceClosure destructor) = 0;
|
|
};
|
|
|
|
} // namespace media_router
|
|
diff --git a/chrome/browser/ui/media_router/media_router_ui.cc b/chrome/browser/ui/media_router/media_router_ui.cc
|
|
index 1865115f..644d131 100644
|
|
--- a/chrome/browser/ui/media_router/media_router_ui.cc
|
|
+++ b/chrome/browser/ui/media_router/media_router_ui.cc
|
|
@@ -83,6 +83,9 @@
|
|
MediaRouterUI::~MediaRouterUI() {
|
|
if (media_route_starter_)
|
|
DetachFromMediaRouteStarter();
|
|
+ for (CastDialogController::Observer& observer : observers_) {
|
|
+ observer.OnControllerDestroying();
|
|
+ }
|
|
}
|
|
|
|
// static
|
|
@@ -145,9 +148,6 @@
|
|
}
|
|
|
|
void MediaRouterUI::DetachFromMediaRouteStarter() {
|
|
- for (CastDialogController::Observer& observer : observers_)
|
|
- observer.OnControllerInvalidated();
|
|
-
|
|
media_route_starter()->RemovePresentationRequestSourceObserver(this);
|
|
media_route_starter()->RemoveMediaSinkWithCastModesObserver(this);
|
|
}
|
|
@@ -181,8 +181,16 @@
|
|
|
|
std::unique_ptr<MediaRouteStarter> MediaRouterUI::TakeMediaRouteStarter() {
|
|
DCHECK(media_route_starter_) << "MediaRouteStarter already taken!";
|
|
- DetachFromMediaRouteStarter();
|
|
- return std::move(media_route_starter_);
|
|
+ auto starter = std::move(media_route_starter_);
|
|
+ if (destructor_) {
|
|
+ std::move(destructor_).Run(); // May destroy `this`.
|
|
+ }
|
|
+ return starter;
|
|
+}
|
|
+
|
|
+void MediaRouterUI::RegisterDestructor(base::OnceClosure destructor) {
|
|
+ DCHECK(!destructor_);
|
|
+ destructor_ = std::move(destructor);
|
|
}
|
|
|
|
bool MediaRouterUI::CreateRoute(const MediaSink::Id& sink_id,
|
|
diff --git a/chrome/browser/ui/media_router/media_router_ui.h b/chrome/browser/ui/media_router/media_router_ui.h
|
|
index 5c2f14e..7afe775 100644
|
|
--- a/chrome/browser/ui/media_router/media_router_ui.h
|
|
+++ b/chrome/browser/ui/media_router/media_router_ui.h
|
|
@@ -100,8 +100,10 @@
|
|
void StopCasting(const std::string& route_id) override;
|
|
void ClearIssue(const Issue::Id& issue_id) override;
|
|
// Note that |MediaRouterUI| should not be used after |TakeMediaRouteStarter|
|
|
- // is called.
|
|
+ // is called. To enforce that, |TakeMediaRouteStarter| calls the destructor
|
|
+ // callback given to |RegisterDestructor| to destroy itself.
|
|
std::unique_ptr<MediaRouteStarter> TakeMediaRouteStarter() override;
|
|
+ void RegisterDestructor(base::OnceClosure destructor) override;
|
|
|
|
// Requests a route be created from the source mapped to
|
|
// |cast_mode|, to the sink given by |sink_id|.
|
|
@@ -337,6 +339,8 @@
|
|
raw_ptr<MediaRouter> router_;
|
|
raw_ptr<LoggerImpl> logger_;
|
|
|
|
+ base::OnceClosure destructor_;
|
|
+
|
|
// NOTE: Weak pointers must be invalidated before all other member variables.
|
|
// Therefore |weak_factory_| must be placed at the end.
|
|
base::WeakPtrFactory<MediaRouterUI> weak_factory_{this};
|
|
diff --git a/chrome/browser/ui/media_router/media_router_ui_unittest.cc b/chrome/browser/ui/media_router/media_router_ui_unittest.cc
|
|
index 2cc243d1..c33437b 100644
|
|
--- a/chrome/browser/ui/media_router/media_router_ui_unittest.cc
|
|
+++ b/chrome/browser/ui/media_router/media_router_ui_unittest.cc
|
|
@@ -80,11 +80,11 @@
|
|
}
|
|
|
|
MOCK_METHOD1(OnModelUpdated, void(const CastDialogModel& model));
|
|
- void OnControllerInvalidated() override {
|
|
+ void OnControllerDestroying() override {
|
|
controller_ = nullptr;
|
|
- OnControllerInvalidatedInternal();
|
|
+ OnControllerDestroyingInternal();
|
|
}
|
|
- MOCK_METHOD0(OnControllerInvalidatedInternal, void());
|
|
+ MOCK_METHOD0(OnControllerDestroyingInternal, void());
|
|
|
|
private:
|
|
raw_ptr<CastDialogController> controller_ = nullptr;
|
|
@@ -295,7 +295,7 @@
|
|
})));
|
|
NotifyUiOnRoutesUpdated({route});
|
|
|
|
- EXPECT_CALL(observer, OnControllerInvalidatedInternal());
|
|
+ EXPECT_CALL(observer, OnControllerDestroyingInternal());
|
|
ui_.reset();
|
|
}
|
|
|
|
diff --git a/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view.cc b/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view.cc
|
|
index 34dad46..d843bba 100644
|
|
--- a/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view.cc
|
|
+++ b/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view.cc
|
|
@@ -222,6 +222,11 @@
|
|
if (cast_controller) {
|
|
cast_controller_ = std::move(cast_controller);
|
|
cast_controller_->AddObserver(this);
|
|
+ cast_controller_->RegisterDestructor(
|
|
+ base::BindOnce(&MediaItemUIDeviceSelectorView::DestroyCastController,
|
|
+ // Unretained is safe: this callback is held by
|
|
+ // cast_controller_, which is owned by this object.
|
|
+ base::Unretained(this)));
|
|
}
|
|
}
|
|
|
|
@@ -499,10 +504,6 @@
|
|
observer.OnMediaItemUIDeviceSelectorUpdated(device_entry_ui_map_);
|
|
}
|
|
|
|
-void MediaItemUIDeviceSelectorView::OnControllerInvalidated() {
|
|
- cast_controller_.reset();
|
|
-}
|
|
-
|
|
void MediaItemUIDeviceSelectorView::OnDeviceSelected(int tag) {
|
|
auto it = device_entry_ui_map_.find(tag);
|
|
DCHECK(it != device_entry_ui_map_.end());
|
|
@@ -658,5 +659,9 @@
|
|
weak_ptr_factory_.GetWeakPtr()));
|
|
}
|
|
|
|
+void MediaItemUIDeviceSelectorView::DestroyCastController() {
|
|
+ cast_controller_.reset();
|
|
+}
|
|
+
|
|
BEGIN_METADATA(MediaItemUIDeviceSelectorView, views::View)
|
|
END_METADATA
|
|
diff --git a/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view.h b/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view.h
|
|
index e950565..222fc20 100644
|
|
--- a/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view.h
|
|
+++ b/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view.h
|
|
@@ -81,7 +81,6 @@
|
|
|
|
// media_router::CastDialogController::Observer
|
|
void OnModelUpdated(const media_router::CastDialogModel& model) override;
|
|
- void OnControllerInvalidated() override;
|
|
|
|
// MediaItemUIFooterView::Delegate
|
|
void OnDeviceSelected(int tag) override;
|
|
@@ -121,6 +120,7 @@
|
|
void RecordCastDeviceCount();
|
|
DeviceEntryUI* GetDeviceEntryUI(views::View* view) const;
|
|
void RegisterAudioDeviceCallbacks();
|
|
+ void DestroyCastController();
|
|
|
|
bool has_expand_button_been_shown_ = false;
|
|
bool have_devices_been_shown_ = false;
|
|
diff --git a/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view_unittest.cc b/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view_unittest.cc
|
|
index c3bcc6cc..6ae3dde8 100644
|
|
--- a/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view_unittest.cc
|
|
+++ b/chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view_unittest.cc
|
|
@@ -156,6 +156,7 @@
|
|
MOCK_METHOD1(ClearIssue, void(const media_router::Issue::Id& issue_id));
|
|
MOCK_METHOD0(TakeMediaRouteStarter,
|
|
std::unique_ptr<media_router::MediaRouteStarter>());
|
|
+ MOCK_METHOD1(RegisterDestructor, void(base::OnceClosure));
|
|
};
|
|
|
|
} // anonymous namespace
|
|
diff --git a/chrome/browser/ui/views/media_router/cast_dialog_coordinator_unittest.cc b/chrome/browser/ui/views/media_router/cast_dialog_coordinator_unittest.cc
|
|
index f6c80d6a..2dedc7e 100644
|
|
--- a/chrome/browser/ui/views/media_router/cast_dialog_coordinator_unittest.cc
|
|
+++ b/chrome/browser/ui/views/media_router/cast_dialog_coordinator_unittest.cc
|
|
@@ -40,6 +40,7 @@
|
|
MOCK_METHOD(void, StopCasting, (const std::string& route_id));
|
|
MOCK_METHOD(void, ClearIssue, (const Issue::Id& issue_id));
|
|
MOCK_METHOD(std::unique_ptr<MediaRouteStarter>, TakeMediaRouteStarter, ());
|
|
+ MOCK_METHOD(void, RegisterDestructor, (base::OnceClosure));
|
|
};
|
|
|
|
class CastDialogCoordinatorTest : public TestWithBrowserView {
|
|
diff --git a/chrome/browser/ui/views/media_router/cast_dialog_view.cc b/chrome/browser/ui/views/media_router/cast_dialog_view.cc
|
|
index e3c7dadb..711d081 100644
|
|
--- a/chrome/browser/ui/views/media_router/cast_dialog_view.cc
|
|
+++ b/chrome/browser/ui/views/media_router/cast_dialog_view.cc
|
|
@@ -125,9 +125,9 @@
|
|
observer.OnDialogModelUpdated(this);
|
|
}
|
|
|
|
-void CastDialogView::OnControllerInvalidated() {
|
|
+void CastDialogView::OnControllerDestroying() {
|
|
controller_ = nullptr;
|
|
- // We don't destroy the dialog here because if the invalidation was caused by
|
|
+ // We don't destroy the dialog here because if the destruction was caused by
|
|
// activating the toolbar icon in order to close the dialog, then it would
|
|
// cause the dialog to immediately open again.
|
|
}
|
|
diff --git a/chrome/browser/ui/views/media_router/cast_dialog_view.h b/chrome/browser/ui/views/media_router/cast_dialog_view.h
|
|
index d87fdda..d44d4e0 100644
|
|
--- a/chrome/browser/ui/views/media_router/cast_dialog_view.h
|
|
+++ b/chrome/browser/ui/views/media_router/cast_dialog_view.h
|
|
@@ -66,7 +66,7 @@
|
|
|
|
// CastDialogController::Observer:
|
|
void OnModelUpdated(const CastDialogModel& model) override;
|
|
- void OnControllerInvalidated() override;
|
|
+ void OnControllerDestroying() override;
|
|
|
|
// views::BubbleDialogDelegateView:
|
|
void OnPaint(gfx::Canvas* canvas) override;
|
|
diff --git a/chrome/browser/ui/views/media_router/cast_dialog_view_browsertest.cc b/chrome/browser/ui/views/media_router/cast_dialog_view_browsertest.cc
|
|
index 1c584120..a7af3c8 100644
|
|
--- a/chrome/browser/ui/views/media_router/cast_dialog_view_browsertest.cc
|
|
+++ b/chrome/browser/ui/views/media_router/cast_dialog_view_browsertest.cc
|
|
@@ -70,6 +70,7 @@
|
|
override {
|
|
return nullptr;
|
|
}
|
|
+ void RegisterDestructor(base::OnceClosure destructor) override {}
|
|
};
|
|
|
|
} // namespace
|
|
diff --git a/chrome/browser/ui/views/media_router/cast_dialog_view_unittest.cc b/chrome/browser/ui/views/media_router/cast_dialog_view_unittest.cc
|
|
index 5326467..988cb07a 100644
|
|
--- a/chrome/browser/ui/views/media_router/cast_dialog_view_unittest.cc
|
|
+++ b/chrome/browser/ui/views/media_router/cast_dialog_view_unittest.cc
|
|
@@ -91,6 +91,7 @@
|
|
MOCK_METHOD1(StopCasting, void(const std::string& route_id));
|
|
MOCK_METHOD1(ClearIssue, void(const Issue::Id& issue_id));
|
|
MOCK_METHOD0(TakeMediaRouteStarter, std::unique_ptr<MediaRouteStarter>());
|
|
+ MOCK_METHOD1(RegisterDestructor, void(base::OnceClosure));
|
|
};
|
|
|
|
class CastDialogViewTest : public ChromeViewsTestBase {
|
|
diff --git a/chrome/browser/ui/views/media_router/media_router_dialog_controller_views.cc b/chrome/browser/ui/views/media_router/media_router_dialog_controller_views.cc
|
|
index ad379b2..244d523 100644
|
|
--- a/chrome/browser/ui/views/media_router/media_router_dialog_controller_views.cc
|
|
+++ b/chrome/browser/ui/views/media_router/media_router_dialog_controller_views.cc
|
|
@@ -51,7 +51,7 @@
|
|
std::move(context));
|
|
}
|
|
|
|
- ShowGlobalMeidaControlsDialog(std::move(context));
|
|
+ ShowGlobalMediaControlsDialog(std::move(context));
|
|
return true;
|
|
}
|
|
|
|
@@ -155,9 +155,20 @@
|
|
initiator(), std::move(start_presentation_context_))
|
|
: MediaRouterUI::CreateWithDefaultMediaSourceAndMirroring(
|
|
initiator());
|
|
+ ui_->RegisterDestructor(
|
|
+ base::BindOnce(&MediaRouterDialogControllerViews::DestroyMediaRouterUI,
|
|
+ // Safe to use base::Unretained here: the callback being
|
|
+ // bound is held by the MediaRouterUI we are creating and
|
|
+ // owning, and ownership of |ui_| is never transferred
|
|
+ // away from this object.
|
|
+ base::Unretained(this)));
|
|
}
|
|
|
|
-void MediaRouterDialogControllerViews::ShowGlobalMeidaControlsDialog(
|
|
+void MediaRouterDialogControllerViews::DestroyMediaRouterUI() {
|
|
+ ui_.reset();
|
|
+}
|
|
+
|
|
+void MediaRouterDialogControllerViews::ShowGlobalMediaControlsDialog(
|
|
std::unique_ptr<StartPresentationContext> context) {
|
|
// Show the WebContents requesting a dialog.
|
|
initiator()->GetDelegate()->ActivateContents(initiator());
|
|
diff --git a/chrome/browser/ui/views/media_router/media_router_dialog_controller_views.h b/chrome/browser/ui/views/media_router/media_router_dialog_controller_views.h
|
|
index 0a5fdb1..7c97211 100644
|
|
--- a/chrome/browser/ui/views/media_router/media_router_dialog_controller_views.h
|
|
+++ b/chrome/browser/ui/views/media_router/media_router_dialog_controller_views.h
|
|
@@ -69,13 +69,14 @@
|
|
// MediaRouterUIService::Observer:
|
|
void OnServiceDisabled() override;
|
|
|
|
- // Initializes |ui_|.
|
|
+ // Initializes and destroys |ui_| respectively.
|
|
void InitializeMediaRouterUI();
|
|
+ void DestroyMediaRouterUI();
|
|
|
|
// If there exists a media button, show the GMC dialog anchored to the media
|
|
// button. Otherwise, show the dialog anchored to the top center of the web
|
|
// contents.
|
|
- void ShowGlobalMeidaControlsDialog(
|
|
+ void ShowGlobalMediaControlsDialog(
|
|
std::unique_ptr<StartPresentationContext> context);
|
|
|
|
// Returns the media button from the browser that initiates the request to
|