[omnibox] Fix Views clipboard access issues on Linux.

Includes one behavioral change, no longer disabling the ability to paste
via keyboard shortcuts when the clipboard is empty. This change
simplifies the code, reducing the number of points of access to the
clipboard and removing the need for an auto-reset bool to control
whether the user is notified of restricted clipboard access. The removal
of the auto-reset bool is especially important as the scope-exiting
reset will always cause a memory access even if the view has been
deleted.

Bug: 1161149
Change-Id: I7a494c534e0efbf5d762a58fe6f7f39591cebb80
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2619204
Commit-Queue: Justin Donnelly <jdonnelly@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: manuk hovanesian <manukh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843159}

chrome/browser/ui/views/omnibox/omnibox_view_views.cc

@@ -789,9 +789,16 @@ void OmniboxViewViews::ExecuteCommand(int command_id, int event_flags) {
   DestroyTouchSelection();
   switch (command_id) {
     // These commands don't invoke the popup via OnBefore/AfterPossibleChange().
-    case IDC_PASTE_AND_GO:
-      model()->PasteAndGo(GetClipboardText(/*notify_if_restricted=*/true));
+    case IDC_PASTE_AND_GO: {
+      auto weak_this = weak_factory_.GetWeakPtr();
+      base::string16 text = GetClipboardText(/*notify_if_restricted=*/true);
+      // Because ReadText() in GetClipboardText() runs a nested message loop,
+      // |this| may be deleted before it returns. See https://crbug.com/1161149.
+      if (!weak_this)
+        return;
+      model()->PasteAndGo(text);
       return;
+    }
     case IDC_SHOW_FULL_URLS:
     case IDC_EDIT_SEARCH_ENGINES:
       location_bar_view_->command_updater()->ExecuteCommand(command_id);
@@ -933,8 +940,12 @@ base::string16 OmniboxViewViews::GetSelectedText() const {
 }
 
 void OmniboxViewViews::OnOmniboxPaste() {
-  const base::string16 text(GetClipboardText(/*notify_if_restricted=*/true));
-
+  auto weak_this = weak_factory_.GetWeakPtr();
+  base::string16 text = GetClipboardText(/*notify_if_restricted=*/true);
+  // Because ReadText() in GetClipboardText() runs a nested message loop, |this|
+  // may be deleted before it returns. See https://crbug.com/1161149.
+  if (!weak_this)
+    return;
   if (text.empty() ||
       // When the fakebox is focused, ignore pasted whitespace because if the
       // fakebox is hidden and there's only whitespace in the omnibox, it's
@@ -1466,10 +1477,9 @@ base::string16 OmniboxViewViews::GetLabelForCommandId(int command_id) const {
 
   // Don't paste-and-go data that was marked by its originator as confidential.
   constexpr size_t kMaxSelectionTextLength = 50;
-  const base::string16 clipboard_text =
-      IsClipboardDataMarkedAsConfidential()
-          ? base::string16()
-          : GetClipboardText(/*notify_if_restricted=*/false);
+  const base::string16 clipboard_text = IsClipboardDataMarkedAsConfidential()
+                                            ? base::string16()
+                                            : clipboard_text_;
 
   if (clipboard_text.empty())
     return l10n_util::GetStringUTF16(IDS_PASTE_AND_GO_EMPTY);
@@ -1977,12 +1987,10 @@ void OmniboxViewViews::OnBlur() {
 
 bool OmniboxViewViews::IsCommandIdEnabled(int command_id) const {
   if (command_id == Textfield::kPaste)
-    return !GetReadOnly() &&
-           !GetClipboardText(/*notify_if_restricted=*/false).empty();
+    return !GetReadOnly() && !clipboard_text_.empty();
   if (command_id == IDC_PASTE_AND_GO) {
     return !GetReadOnly() && !IsClipboardDataMarkedAsConfidential() &&
-           model()->CanPasteAndGo(
-               GetClipboardText(/*notify_if_restricted=*/false));
+           model()->CanPasteAndGo(clipboard_text_);
   }
 
   // Menu item is only shown when it is valid.
@@ -1993,6 +2001,35 @@ bool OmniboxViewViews::IsCommandIdEnabled(int command_id) const {
          location_bar_view_->command_updater()->IsCommandEnabled(command_id);
 }
 
+void OmniboxViewViews::ShowContextMenu(const gfx::Point& p,
+                                       ui::MenuSourceType source_type) {
+  // Because ReadText() in GetClipboardText() runs a nested message loop, |this|
+  // may be deleted before it returns. See https://crbug.com/1161149.
+  auto weak_this = weak_factory_.GetWeakPtr();
+
+  // We shouldn't notify the user if the clipboard is restricted because they're
+  // not necessarily trying to access the clipboard here. This value is only
+  // used to enable/disable and populate the text of context menu entries.
+  base::string16 text = GetClipboardText(/*notify_if_restricted=*/false);
+
+  // Avoid running any of the Views menu code by returning early if |this| has
+  // been deleted.
+  if (!weak_this)
+    return;
+
+  // Save the clipboard text for use in GetLabelForCommandId() and
+  // IsCommandIdEnabled(). It would be possible to access the clipboard again in
+  // those methods, similarly protected by checking a weak pointer to |this|.
+  // But saving it here avoids that additional complexity while also serving as
+  // an optimization in the case where the clipboard contains a very large
+  // amount of data and reading it can take seconds.
+  clipboard_text_ = text;
+
+  View::ShowContextMenu(p, source_type);
+
+  clipboard_text_.clear();
+}
+
 void OmniboxViewViews::DidStartNavigation(
     content::NavigationHandle* navigation) {
   if (!OmniboxFieldTrial::ShouldHidePathQueryRefOnInteraction() ||
@@ -2111,10 +2148,8 @@ bool OmniboxViewViews::IsTextEditCommandEnabled(
   switch (command) {
     case ui::TextEditCommand::MOVE_UP:
     case ui::TextEditCommand::MOVE_DOWN:
-      return !GetReadOnly();
     case ui::TextEditCommand::PASTE:
-      return !GetReadOnly() &&
-             !GetClipboardText(show_rejection_ui_if_any_).empty();
+      return !GetReadOnly();
     default:
       return Textfield::IsTextEditCommandEnabled(command);
   }
@@ -2126,8 +2161,6 @@ void OmniboxViewViews::ExecuteTextEditCommand(ui::TextEditCommand command) {
   // here, we need to deactivate touch text selection here, too.
   DestroyTouchSelection();
 
-  base::AutoReset<bool> show_rejection_ui(&show_rejection_ui_if_any_, true);
-
   if (!IsTextEditCommandEnabled(command))
     return;
 
@@ -2185,11 +2218,6 @@ bool OmniboxViewViews::HandleKeyEvent(views::Textfield* textfield,
   if (event.IsUnicodeKeyCode())
     return false;
 
-  // Show a notification if the clipboard is restricted by the rules of the
-  // data leak prevention policy. This state is used by the
-  // IsTextEditCommandEnabled(ui::TextEditCommand::PASTE) cases below.
-  base::AutoReset<bool> show_rejection_ui(&show_rejection_ui_if_any_, true);
-
   const bool shift = event.IsShiftDown();
   const bool control = event.IsControlDown();
   const bool alt = event.IsAltDown() || event.IsAltGrDown();

chrome/browser/ui/views/omnibox/omnibox_view_views.h

@@ -154,6 +154,10 @@ class OmniboxViewViews : public OmniboxView,
   base::string16 GetLabelForCommandId(int command_id) const override;
   bool IsCommandIdEnabled(int command_id) const override;
 
+  // views::View:
+  void ShowContextMenu(const gfx::Point& p,
+                       ui::MenuSourceType source_type) override;
+
   // content::WebContentsObserver:
   void DidStartNavigation(content::NavigationHandle* navigation) override;
   void DidFinishNavigation(content::NavigationHandle* navigation) override;
@@ -164,6 +168,7 @@ class OmniboxViewViews : public OmniboxView,
   OmniboxPopupContentsView* GetPopupContentsViewForTesting() const {
     return popup_view_.get();
   }
+  void set_clipboard_text(const base::string16 text) { clipboard_text_ = text; }
 
  protected:
   // Animates the URL to a given range of text, which could be a substring or
@@ -647,9 +652,6 @@ class OmniboxViewViews : public OmniboxView,
   // and gets a tap. So we use this variable to remember focus state before tap.
   bool select_all_on_gesture_tap_ = false;
 
-  // Whether the user should be notified if the clipboard is restricted.
-  bool show_rejection_ui_if_any_ = false;
-
   // Keep track of the word that would be selected if URL is unelided between
   // a single and double click. This is an edge case where the elided URL is
   // selected. On the double click, unelision is performed in between the first
@@ -696,6 +698,12 @@ class OmniboxViewViews : public OmniboxView,
 
   PrefChangeRegistrar pref_change_registrar_;
 
+  // Clipboard text stored between retrieval in ShowContextMenu() and use in
+  // GetLabelForCommandId() and IsCommandIdEnabled(). This is useful as an
+  // optimization and to avoid additional nested message loop clipboard access
+  // on Linux. See comments in ShowContextMenu() for more details.
+  base::string16 clipboard_text_;
+
   base::WeakPtrFactory<OmniboxViewViews> weak_factory_{this};
 };
 

chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc

@@ -949,13 +949,10 @@ TEST_F(OmniboxViewViewsTest, BlurNeverExitsKeywordMode) {
 }
 
 TEST_F(OmniboxViewViewsTest, PasteAndGoToUrlOrSearchCommand) {
-  ui::Clipboard* clipboard = ui::Clipboard::GetForCurrentThread();
-  ui::ClipboardBuffer clipboard_buffer = ui::ClipboardBuffer::kCopyPaste;
   command_updater()->UpdateCommandEnabled(IDC_OPEN_CURRENT_URL, true);
 
   // Test command is disabled for an empty clipboard.
-  clipboard->Clear(clipboard_buffer);
-  EXPECT_FALSE(omnibox_view()->IsCommandIdEnabled(IDC_PASTE_AND_GO));
+  omnibox_view()->set_clipboard_text(base::string16());
 
   // Test input that's a valid URL.
   base::string16 expected_text =
@@ -964,8 +961,7 @@ TEST_F(OmniboxViewViewsTest, PasteAndGoToUrlOrSearchCommand) {
 #else
       base::ASCIIToUTF16("Pa&ste and go to https://test.com");
 #endif
-  ui::ScopedClipboardWriter(clipboard_buffer)
-      .WriteText(base::ASCIIToUTF16("https://test.com/"));
+  omnibox_view()->set_clipboard_text(base::ASCIIToUTF16("https://test.com/"));
   base::string16 returned_text =
       omnibox_view()->GetLabelForCommandId(IDC_PASTE_AND_GO);
   EXPECT_TRUE(omnibox_view()->IsCommandIdEnabled(IDC_PASTE_AND_GO));
@@ -978,8 +974,7 @@ TEST_F(OmniboxViewViewsTest, PasteAndGoToUrlOrSearchCommand) {
 #else
       base::ASCIIToUTF16("Pa&ste and go to test.com");
 #endif
-  ui::ScopedClipboardWriter(clipboard_buffer)
-      .WriteText(base::ASCIIToUTF16("test.com"));
+  omnibox_view()->set_clipboard_text(base::ASCIIToUTF16("test.com"));
   returned_text = omnibox_view()->GetLabelForCommandId(IDC_PASTE_AND_GO);
   EXPECT_TRUE(omnibox_view()->IsCommandIdEnabled(IDC_PASTE_AND_GO));
   EXPECT_EQ(expected_text, returned_text);
@@ -993,8 +988,8 @@ TEST_F(OmniboxViewViewsTest, PasteAndGoToUrlOrSearchCommand) {
       base::WideToUTF16(
           L"Pa&ste and search for \x201Cthis is a test sentence\x201D");
 #endif
-  ui::ScopedClipboardWriter(clipboard_buffer)
-      .WriteText(base::ASCIIToUTF16("this is a test sentence"));
+  omnibox_view()->set_clipboard_text(
+      base::ASCIIToUTF16("this is a test sentence"));
   returned_text = omnibox_view()->GetLabelForCommandId(IDC_PASTE_AND_GO);
   EXPECT_TRUE(omnibox_view()->IsCommandIdEnabled(IDC_PASTE_AND_GO));
   EXPECT_EQ(expected_text, returned_text);