0

multipaste: Fetch clipboard history item from source of truth in views

The clipboard history menu model adapter supplies menu item views with a
snapshot of the item from when the item was first created. In order to
account for updates such as HTML preview rendering--while also ensuring
there is no UAF potential when items are removed from the menu--this CL
caches the item ID and a handle to the clipboard history item model in
every view.

Bug: b/267677307
Change-Id: I7730e493e1e2e87c5cc1e0c4575b4ca91ad51d30
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4262924
Reviewed-by: David Black <dmblack@google.com>
Commit-Queue: Colin Kincaid <ckincaid@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1107808}
This commit is contained in:
Colin Kincaid
2023-02-21 17:37:42 +00:00
committed by Chromium LUCI CQ
parent 753edf2fe2
commit 2363e3d1f8
12 changed files with 129 additions and 75 deletions

@ -284,6 +284,9 @@ ClipboardHistoryControllerImpl::~ClipboardHistoryControllerImpl() {
}
void ClipboardHistoryControllerImpl::Shutdown() {
if (IsMenuShowing()) {
context_menu_->Cancel();
}
nudge_controller_.reset();
}
@ -875,6 +878,7 @@ void ClipboardHistoryControllerImpl::PasteClipboardHistoryItem(
ui::KeyEvent ctrl_release(ui::ET_KEY_RELEASED, ui::VKEY_CONTROL, ui::EF_NONE);
host->DeliverEventToSink(&ctrl_release);
clipboard_history_util::RecordClipboardHistoryItemPasted(item);
base::UmaHistogramEnumeration("Ash.ClipboardHistory.PasteType", paste_type);
for (auto& observer : observers_)

@ -395,7 +395,8 @@ views::MenuItemView* ClipboardHistoryMenuModelAdapter::AppendMenuItem(
std::unique_ptr<ClipboardHistoryItemView> item_view =
ClipboardHistoryItemView::CreateFromClipboardHistoryItem(
GetItemFromCommandId(command_id), resource_manager_, container);
GetItemFromCommandId(command_id).id(), clipboard_history_,
resource_manager_, container);
item_view->Init();
item_views_by_command_id_.insert(std::make_pair(command_id, item_view.get()));
container->AddChildView(std::move(item_view));

@ -50,13 +50,14 @@ class FadeImageView : public views::ImageView,
public ui::ImplicitAnimationObserver,
public ClipboardHistoryResourceManager::Observer {
public:
FadeImageView(const ClipboardHistoryItem* clipboard_history_item,
const ClipboardHistoryResourceManager* resource_manager,
base::RepeatingClosure update_callback)
: views::ImageView(),
FadeImageView(
base::RepeatingCallback<const ClipboardHistoryItem*()> item_resolver,
const ClipboardHistoryResourceManager* resource_manager,
base::RepeatingClosure update_callback)
: item_resolver_(item_resolver),
resource_manager_(resource_manager),
clipboard_history_item_(*clipboard_history_item),
update_callback_(update_callback) {
DCHECK(item_resolver_);
resource_manager_->AddObserver(this);
SetImageFromModel();
DCHECK(update_callback_);
@ -74,8 +75,10 @@ class FadeImageView : public views::ImageView,
// ClipboardHistoryResourceManager::Observer:
void OnCachedImageModelUpdated(
const std::vector<base::UnguessableToken>& item_ids) override {
if (!base::Contains(item_ids, clipboard_history_item_.id()))
const auto* item = item_resolver_.Run();
if (!item || !base::Contains(item_ids, item->id())) {
return;
}
// Fade the old image out, then swap in the new image.
DCHECK_EQ(FadeAnimationState::kNoFadeAnimation, animation_state_);
@ -115,11 +118,11 @@ class FadeImageView : public views::ImageView,
void SetImageFromModel() {
// TODO(b/267677307): Make cached image an item field, set and updated
// directly by the resource manager.
const gfx::ImageSkia& image =
*(resource_manager_->GetImageModel(clipboard_history_item_)
.GetImage()
.ToImageSkia());
SetImage(image);
if (const auto* item = item_resolver_.Run()) {
const gfx::ImageSkia& image =
*(resource_manager_->GetImageModel(*item).GetImage().ToImageSkia());
SetImage(image);
}
// When fading in a new image, the ImageView's image has likely changed
// sizes.
@ -138,11 +141,12 @@ class FadeImageView : public views::ImageView,
// The current animation state.
FadeAnimationState animation_state_ = FadeAnimationState::kNoFadeAnimation;
// The resource manager, owned by ClipboardHistoryController.
const ClipboardHistoryResourceManager* const resource_manager_;
// Generates a *possibly null* pointer to the clipboard history item
// represented by this image.
base::RepeatingCallback<const ClipboardHistoryItem*()> item_resolver_;
// The ClipboardHistoryItem represented by this class.
const ClipboardHistoryItem clipboard_history_item_;
// Owned by `ClipboardHistoryController`.
const ClipboardHistoryResourceManager* const resource_manager_;
// Used to notify of image changes.
base::RepeatingClosure update_callback_;
@ -220,11 +224,17 @@ class ClipboardHistoryBitmapItemView::BitmapContentsView
}
std::unique_ptr<views::ImageView> BuildImageView() {
const auto* clipboard_history_item = container_->clipboard_history_item();
const auto* clipboard_history_item = container_->GetClipboardHistoryItem();
DCHECK(clipboard_history_item);
switch (container_->data_format_) {
case ui::ClipboardInternalFormat::kHtml:
return std::make_unique<FadeImageView>(
clipboard_history_item, container_->resource_manager_,
// `Unretained()` is safe because `container_` will ultimately own
// this `FadeImageView`.
base::BindRepeating(
&ClipboardHistoryBitmapItemView::GetClipboardHistoryItem,
base::Unretained(container_)),
container_->resource_manager_,
base::BindRepeating(&BitmapContentsView::UpdateImageViewSize,
weak_ptr_factory_.GetWeakPtr()));
case ui::ClipboardInternalFormat::kPng: {
@ -302,13 +312,14 @@ END_METADATA
// ClipboardHistoryBitmapItemView
ClipboardHistoryBitmapItemView::ClipboardHistoryBitmapItemView(
const ClipboardHistoryItem* clipboard_history_item,
const base::UnguessableToken& item_id,
const ClipboardHistory* clipboard_history,
const ClipboardHistoryResourceManager* resource_manager,
views::MenuItemView* container)
: ClipboardHistoryItemView(clipboard_history_item, container),
: ClipboardHistoryItemView(item_id, clipboard_history, container),
resource_manager_(resource_manager),
data_format_(*clipboard_history_util::CalculateMainFormat(
clipboard_history_item->data())) {
GetClipboardHistoryItem()->data())) {
switch (data_format_) {
case ui::ClipboardInternalFormat::kHtml:
SetAccessibleName(

@ -5,12 +5,13 @@
#ifndef ASH_CLIPBOARD_VIEWS_CLIPBOARD_HISTORY_BITMAP_ITEM_VIEW_H_
#define ASH_CLIPBOARD_VIEWS_CLIPBOARD_HISTORY_BITMAP_ITEM_VIEW_H_
#include "ash/clipboard/clipboard_history_item.h"
#include "ash/clipboard/views/clipboard_history_item_view.h"
#include "base/unguessable_token.h"
#include "ui/base/clipboard/clipboard_data.h"
#include "ui/base/metadata/metadata_header_macros.h"
namespace ash {
class ClipboardHistory;
class ClipboardHistoryResourceManager;
// The menu item showing a bitmap.
@ -18,7 +19,8 @@ class ClipboardHistoryBitmapItemView : public ClipboardHistoryItemView {
public:
METADATA_HEADER(ClipboardHistoryBitmapItemView);
ClipboardHistoryBitmapItemView(
const ClipboardHistoryItem* clipboard_history_item,
const base::UnguessableToken& item_id,
const ClipboardHistory* clipboard_history,
const ClipboardHistoryResourceManager* resource_manager,
views::MenuItemView* container);
ClipboardHistoryBitmapItemView(const ClipboardHistoryBitmapItemView& rhs) =

@ -23,9 +23,10 @@ constexpr auto kIconMargin = gfx::Insets::TLBR(0, 0, 0, 12);
namespace ash {
ClipboardHistoryFileItemView::ClipboardHistoryFileItemView(
const ClipboardHistoryItem* clipboard_history_item,
const base::UnguessableToken& item_id,
const ClipboardHistory* clipboard_history,
views::MenuItemView* container)
: ClipboardHistoryTextItemView(clipboard_history_item, container) {}
: ClipboardHistoryTextItemView(item_id, clipboard_history, container) {}
ClipboardHistoryFileItemView::~ClipboardHistoryFileItemView() = default;
std::unique_ptr<ClipboardHistoryFileItemView::ContentsView>
@ -33,12 +34,14 @@ ClipboardHistoryFileItemView::CreateContentsView() {
auto contents_view = ClipboardHistoryTextItemView::CreateContentsView();
// `file_icon` should be `contents_view`'s first child.
views::ImageView* file_icon = contents_view->AddChildViewAt(
std::make_unique<views::ImageView>(), /*index=*/0);
file_icon->SetImageSize(kIconSize);
file_icon->SetProperty(views::kMarginsKey, kIconMargin);
file_icon->SetImage(clipboard_history_util::GetIconForFileClipboardItem(
clipboard_history_item(), base::UTF16ToUTF8(text())));
if (const auto* item = GetClipboardHistoryItem()) {
views::ImageView* file_icon = contents_view->AddChildViewAt(
std::make_unique<views::ImageView>(), /*index=*/0);
file_icon->SetImageSize(kIconSize);
file_icon->SetProperty(views::kMarginsKey, kIconMargin);
file_icon->SetImage(clipboard_history_util::GetIconForFileClipboardItem(
item, base::UTF16ToUTF8(text())));
}
return contents_view;
}

@ -6,6 +6,7 @@
#define ASH_CLIPBOARD_VIEWS_CLIPBOARD_HISTORY_FILE_ITEM_VIEW_H_
#include "ash/clipboard/views/clipboard_history_text_item_view.h"
#include "base/unguessable_token.h"
#include "ui/base/metadata/metadata_header_macros.h"
namespace views {
@ -13,14 +14,15 @@ class MenuItemView;
}
namespace ash {
class ClipboardHistory;
// The menu item showing the copied file.
class ClipboardHistoryFileItemView : public ClipboardHistoryTextItemView {
public:
METADATA_HEADER(ClipboardHistoryFileItemView);
ClipboardHistoryFileItemView(
const ClipboardHistoryItem* clipboard_history_item,
views::MenuItemView* container);
ClipboardHistoryFileItemView(const base::UnguessableToken& item_id,
const ClipboardHistory* clipboard_history,
views::MenuItemView* container);
ClipboardHistoryFileItemView(const ClipboardHistoryFileItemView& rhs) =
delete;
ClipboardHistoryFileItemView& operator=(

@ -4,6 +4,7 @@
#include "ash/clipboard/views/clipboard_history_item_view.h"
#include "ash/clipboard/clipboard_history.h"
#include "ash/clipboard/clipboard_history_item.h"
#include "ash/clipboard/clipboard_history_util.h"
#include "ash/clipboard/views/clipboard_history_bitmap_item_view.h"
@ -15,6 +16,7 @@
#include "base/auto_reset.h"
#include "base/functional/bind.h"
#include "base/metrics/histogram_macros.h"
#include "base/unguessable_token.h"
#include "ui/accessibility/ax_node_data.h"
#include "ui/base/clipboard/clipboard_data.h"
#include "ui/base/metadata/metadata_impl_macros.h"
@ -28,6 +30,15 @@
namespace ash {
namespace {
using Action = clipboard_history_util::Action;
const ClipboardHistoryItem* GetClipboardHistoryItemImpl(
const base::UnguessableToken& item_id,
const ClipboardHistory* clipboard_history) {
const auto& items = clipboard_history->GetItems();
const auto& item_iter =
base::ranges::find(items, item_id, &ClipboardHistoryItem::id);
return item_iter == items.cend() ? nullptr : &(*item_iter);
}
} // namespace
ClipboardHistoryItemView::ContentsView::ContentsView(
@ -72,30 +83,37 @@ bool ClipboardHistoryItemView::ContentsView::DoesIntersectRect(
// static
std::unique_ptr<ClipboardHistoryItemView>
ClipboardHistoryItemView::CreateFromClipboardHistoryItem(
const ClipboardHistoryItem& item,
const base::UnguessableToken& item_id,
const ClipboardHistory* clipboard_history,
const ClipboardHistoryResourceManager* resource_manager,
views::MenuItemView* container) {
const auto display_format = item.display_format();
const auto* item = GetClipboardHistoryItemImpl(item_id, clipboard_history);
const auto display_format = item->display_format();
UMA_HISTOGRAM_ENUMERATION(
"Ash.ClipboardHistory.ContextMenu.DisplayFormatShown", display_format);
switch (display_format) {
case ClipboardHistoryItem::DisplayFormat::kText:
return std::make_unique<ClipboardHistoryTextItemView>(&item, container);
return std::make_unique<ClipboardHistoryTextItemView>(
item_id, clipboard_history, container);
case ClipboardHistoryItem::DisplayFormat::kPng:
case ClipboardHistoryItem::DisplayFormat::kHtml:
return std::make_unique<ClipboardHistoryBitmapItemView>(
&item, resource_manager, container);
item_id, clipboard_history, resource_manager, container);
case ClipboardHistoryItem::DisplayFormat::kFile:
return std::make_unique<ClipboardHistoryFileItemView>(&item, container);
return std::make_unique<ClipboardHistoryFileItemView>(
item_id, clipboard_history, container);
}
}
ClipboardHistoryItemView::~ClipboardHistoryItemView() = default;
ClipboardHistoryItemView::ClipboardHistoryItemView(
const ClipboardHistoryItem* clipboard_history_item,
const base::UnguessableToken& item_id,
const ClipboardHistory* clipboard_history,
views::MenuItemView* container)
: clipboard_history_item_(clipboard_history_item), container_(container) {}
: item_id_(item_id),
clipboard_history_(clipboard_history),
container_(container) {}
bool ClipboardHistoryItemView::AdvancePseudoFocus(bool reverse) {
if (pseudo_focus_ == PseudoFocus::kEmpty) {
@ -241,23 +259,9 @@ void ClipboardHistoryItemView::OnMouseClickOnDescendantCanceled() {
Activate(Action::kSelectItemHoveredByMouse, ui::EF_NONE);
}
void ClipboardHistoryItemView::MaybeRecordButtonPressedHistogram() const {
switch (action_) {
case Action::kDelete:
clipboard_history_util::RecordClipboardHistoryItemDeleted(
*clipboard_history_item_);
return;
case Action::kPaste:
clipboard_history_util::RecordClipboardHistoryItemPasted(
*clipboard_history_item_);
return;
case Action::kSelect:
case Action::kSelectItemHoveredByMouse:
return;
case Action::kEmpty:
NOTREACHED();
return;
}
const ClipboardHistoryItem* ClipboardHistoryItemView::GetClipboardHistoryItem()
const {
return GetClipboardHistoryItemImpl(item_id_, clipboard_history_);
}
gfx::Size ClipboardHistoryItemView::CalculatePreferredSize() const {
@ -284,7 +288,6 @@ void ClipboardHistoryItemView::Activate(Action action, int event_flags) {
DCHECK_NE(action_, action);
base::AutoReset<Action> action_to_take(&action_, action);
MaybeRecordButtonPressedHistogram();
views::MenuDelegate* delegate = container_->GetDelegate();
const int command_id = container_->GetCommand();

@ -7,6 +7,7 @@
#include "ash/ash_export.h"
#include "ash/clipboard/clipboard_history_util.h"
#include "base/unguessable_token.h"
#include "ui/base/metadata/metadata_header_macros.h"
#include "ui/views/view.h"
#include "ui/views/view_targeter_delegate.h"
@ -16,6 +17,7 @@ class MenuItemView;
} // namespace views
namespace ash {
class ClipboardHistory;
class ClipboardHistoryDeleteButton;
class ClipboardHistoryItem;
class ClipboardHistoryMainButton;
@ -27,7 +29,8 @@ class ASH_EXPORT ClipboardHistoryItemView : public views::View {
METADATA_HEADER(ClipboardHistoryItemView);
static std::unique_ptr<ClipboardHistoryItemView>
CreateFromClipboardHistoryItem(
const ClipboardHistoryItem& item,
const base::UnguessableToken& item_id,
const ClipboardHistory* clipboard_history,
const ClipboardHistoryResourceManager* resource_manager,
views::MenuItemView* container);
@ -106,18 +109,14 @@ class ASH_EXPORT ClipboardHistoryItemView : public views::View {
ClipboardHistoryItemView* const container_;
};
ClipboardHistoryItemView(const ClipboardHistoryItem* clipboard_history_item,
ClipboardHistoryItemView(const base::UnguessableToken& item_id,
const ClipboardHistory* clipboard_history,
views::MenuItemView* container);
// Maybe record histograms after the button is pressed.
void MaybeRecordButtonPressedHistogram() const;
// Creates the contents view.
virtual std::unique_ptr<ContentsView> CreateContentsView() = 0;
const ClipboardHistoryItem* clipboard_history_item() const {
return clipboard_history_item_;
}
const ClipboardHistoryItem* GetClipboardHistoryItem() const;
private:
// Indicates the child under pseudo focus, i.e. the view responding to the
@ -157,8 +156,11 @@ class ASH_EXPORT ClipboardHistoryItemView : public views::View {
// Updates `pseudo_focus_` and children visibility.
void SetPseudoFocus(PseudoFocus new_pseudo_focus);
// Owned by ClipboardHistoryMenuModelAdapter.
const ClipboardHistoryItem* const clipboard_history_item_;
// Unique identifier for the `ClipboardHistoryItem` this view represents.
const base::UnguessableToken item_id_;
// Owned by `ClipboardHistoryControllerImpl`.
const base::raw_ptr<const ClipboardHistory> clipboard_history_;
views::MenuItemView* const container_;

@ -57,10 +57,11 @@ END_METADATA
// ClipboardHistoryTextItemView
ClipboardHistoryTextItemView::ClipboardHistoryTextItemView(
const ClipboardHistoryItem* clipboard_history_item,
const base::UnguessableToken& item_id,
const ClipboardHistory* clipboard_history,
views::MenuItemView* container)
: ClipboardHistoryItemView(clipboard_history_item, container),
text_(clipboard_history_item->display_text()) {
: ClipboardHistoryItemView(item_id, clipboard_history, container),
text_(GetClipboardHistoryItem()->display_text()) {
SetAccessibleName(text_);
}

@ -6,6 +6,7 @@
#define ASH_CLIPBOARD_VIEWS_CLIPBOARD_HISTORY_TEXT_ITEM_VIEW_H_
#include "ash/clipboard/views/clipboard_history_item_view.h"
#include "base/unguessable_token.h"
#include "ui/base/metadata/metadata_header_macros.h"
namespace views {
@ -13,14 +14,15 @@ class MenuItemView;
} // namespace views
namespace ash {
class ClipboardHistory;
// The menu item showing the plain text.
class ClipboardHistoryTextItemView : public ClipboardHistoryItemView {
public:
METADATA_HEADER(ClipboardHistoryTextItemView);
ClipboardHistoryTextItemView(
const ClipboardHistoryItem* clipboard_history_item,
views::MenuItemView* container);
ClipboardHistoryTextItemView(const base::UnguessableToken& item_id,
const ClipboardHistory* clipboard_history,
views::MenuItemView* container);
ClipboardHistoryTextItemView(const ClipboardHistoryTextItemView& rhs) =
delete;
ClipboardHistoryItemView& operator=(const ClipboardHistoryTextItemView& rhs) =

@ -595,6 +595,8 @@ IN_PROC_BROWSER_TEST_F(ClipboardHistoryBrowserTest, HandleClickCancelEvent) {
// Verifies item deletion through the mouse click at the delete button.
IN_PROC_BROWSER_TEST_F(ClipboardHistoryBrowserTest,
DeleteItemByClickAtDeleteButton) {
base::HistogramTester histogram_tester;
// Write some things to the clipboard.
SetClipboardText("A");
SetClipboardText("B");
@ -612,6 +614,9 @@ IN_PROC_BROWSER_TEST_F(ClipboardHistoryBrowserTest,
EXPECT_TRUE(VerifyClipboardTextData({"B"}));
EXPECT_TRUE(VerifyClipboardBufferAndHistoryInSync());
histogram_tester.ExpectTotalCount(
"Ash.ClipboardHistory.ContextMenu.DisplayFormatDeleted", 1);
// Delete the last menu item. Verify that the menu is closed.
{
ScopedClipboardHistoryListUpdateWaiter scoped_waiter;
@ -620,6 +625,9 @@ IN_PROC_BROWSER_TEST_F(ClipboardHistoryBrowserTest,
EXPECT_FALSE(GetClipboardHistoryController()->IsMenuShowing());
EXPECT_TRUE(VerifyClipboardBufferAndHistoryInSync());
histogram_tester.ExpectTotalCount(
"Ash.ClipboardHistory.ContextMenu.DisplayFormatDeleted", 2);
// No menu shows because of the empty clipboard history.
ShowContextMenuViaAccelerator(/*wait_for_selection=*/false);
EXPECT_FALSE(GetClipboardHistoryController()->IsMenuShowing());
@ -1393,6 +1401,8 @@ IN_PROC_BROWSER_TEST_F(ClipboardHistoryTextfieldBrowserTest,
EXPECT_FALSE(GetClipboardHistoryController()->IsMenuShowing());
WaitForOperationConfirmed(/*success_expected=*/true);
EXPECT_EQ("C", base::UTF16ToUTF8(textfield_->GetText()));
histogram_tester.ExpectTotalCount(
"Ash.ClipboardHistory.ContextMenu.DisplayFormatPasted", 2);
textfield_->SetText(std::u16string());
EXPECT_TRUE(textfield_->GetText().empty());
@ -1407,6 +1417,8 @@ IN_PROC_BROWSER_TEST_F(ClipboardHistoryTextfieldBrowserTest,
EXPECT_FALSE(GetClipboardHistoryController()->IsMenuShowing());
WaitForOperationConfirmed(/*success_expected=*/true);
EXPECT_EQ("A", base::UTF16ToUTF8(textfield_->GetText()));
histogram_tester.ExpectTotalCount(
"Ash.ClipboardHistory.ContextMenu.DisplayFormatPasted", 3);
textfield_->SetText(std::u16string());
@ -1422,6 +1434,8 @@ IN_PROC_BROWSER_TEST_F(ClipboardHistoryTextfieldBrowserTest,
EXPECT_FALSE(GetClipboardHistoryController()->IsMenuShowing());
WaitForOperationConfirmed(/*success_expected=*/true);
EXPECT_EQ("A", base::UTF16ToUTF8(textfield_->GetText()));
histogram_tester.ExpectTotalCount(
"Ash.ClipboardHistory.ContextMenu.DisplayFormatPasted", 4);
}
IN_PROC_BROWSER_TEST_F(ClipboardHistoryTextfieldBrowserTest,

@ -67,6 +67,10 @@ chromium-metrics-reviews@google.com.
<summary>
The data format of an item deleted from the clipboard history menu. Recorded
when the user deletes an item.
Note: Prior to M112, this histogram erroneously recorded two samples when an
item was deleted by clicking the delete button. Deleting an item using the
Backspace key correctly recorded one sample.
</summary>
</histogram>
@ -77,6 +81,11 @@ chromium-metrics-reviews@google.com.
<summary>
The data format of an item pasted from the clipboard history menu. Recorded
when the user pastes an item.
Note: Prior to M112, this histogram erroneously recorded samples only when
an item was pasted by clicking or pressing Enter with an item selected.
Other methods of pasting, such as toggling the menu closed or using the
virtual keyboard, were not counted at all.
</summary>
</histogram>