forge/ayaports
1
0
Fork 0
Code Issues 43 Pull requests 11 Projects Releases Packages Wiki Activity Actions
ayaports/backports/electron/crbug-1407202-mediarouter-crash.patch

372 lines
16 KiB
Diff
Raw Blame History

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
Reference in a new issue View git blame Copy permalink