Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: crashes in RenderFrameImpl::OnSelectPopupMenuItem(s) #17659

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions patches/common/chromium/.patches
Expand Up @@ -97,3 +97,4 @@ set_proper_permissions_for_package_s_framework_directory.patch
intersection-observer.patch
keyboard-lock-service-impl.patch
make_--explicitly-allowed-ports_work_with_networkservice.patch
fix_crashes_in_renderframeimpl_onselectpopupmenuitem_s.patch
@@ -0,0 +1,105 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Kent Tamura <tkent@chromium.org>
Date: Thu, 20 Dec 2018 00:22:07 +0000
Subject: Fix crashes in RenderFrameImpl::OnSelectPopupMenuItem(s)

ExternalPopupMenu::DidSelectItem(s) can delete the RenderFrameImpl.
We need to reset external_popup_menu_ before calling it.

Bug: 912211
Change-Id: Ia9a628e144464a2ebb14ab77d3a693fd5cead6fc
Reviewed-on: https://chromium-review.googlesource.com/c/1381325
Commit-Queue: Kent Tamura <tkent@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618026}

diff --git a/content/renderer/external_popup_menu_browsertest.cc b/content/renderer/external_popup_menu_browsertest.cc
index d3cff681551c4961a4a47c48bac9155b22fd7524..0ea34d8809f1b191504d2cc833f74fef694230d3 100644
--- a/content/renderer/external_popup_menu_browsertest.cc
+++ b/content/renderer/external_popup_menu_browsertest.cc
@@ -12,6 +12,7 @@
#include "content/renderer/render_view_impl.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/public/platform/web_size.h"
+#include "third_party/blink/public/web/web_local_frame.h"
#include "third_party/blink/public/web/web_view.h"

// Tests for the external select popup menu (Mac specific).
@@ -153,6 +154,31 @@ TEST_F(ExternalPopupMenuRemoveTest, RemoveOnChange) {
EXPECT_FALSE(SimulateElementClick(kSelectID));
}

+// crbug.com/912211
+TEST_F(ExternalPopupMenuRemoveTest, RemoveFrameOnChange) {
+ LoadHTML(
+ "<style>* { margin: 0; } iframe { border: 0; }</style>"
+ "<body><iframe srcdoc=\""
+ "<style>* { margin: 0; }</style><select><option>opt1<option>opt2"
+ "\"></iframe>"
+ "<script>"
+ "onload = function() {"
+ " const frame = document.querySelector('iframe');"
+ " frame.contentDocument.querySelector('select').onchange = "
+ " () => { frame.remove(); };"
+ "};"
+ "</script>");
+ // Open a popup.
+ SimulatePointClick(gfx::Point(8, 8));
+ // Select something on the sub-frame, it causes the frame to be removed from
+ // the page.
+ auto* child_web_frame =
+ static_cast<blink::WebLocalFrame*>(frame()->GetWebFrame()->FirstChild());
+ static_cast<RenderFrameImpl*>(RenderFrame::FromWebFrame(child_web_frame))
+ ->OnSelectPopupMenuItem(1);
+ // The test passes if the test didn't crash and ASAN didn't complain.
+}
+
class ExternalPopupMenuDisplayNoneTest : public ExternalPopupMenuTest {
public:
ExternalPopupMenuDisplayNoneTest() {}
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 9571ea0233e84e3ab2f2159018efd12f18adcea9..7db2fa3591040b73a55ac620cbf35816afbd715f 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -6473,8 +6473,12 @@ void RenderFrameImpl::OnSelectPopupMenuItem(int selected_index) {
return;

blink::WebScopedUserGesture gesture(frame_);
- external_popup_menu_->DidSelectItem(selected_index);
- external_popup_menu_.reset();
+ // We need to reset |external_popup_menu_| before calling DidSelectItem(),
+ // which might delete |this|.
+ // See ExternalPopupMenuRemoveTest.RemoveFrameOnChange
+ std::unique_ptr<ExternalPopupMenu> popup;
+ popup.swap(external_popup_menu_);
+ popup->DidSelectItem(selected_index);
}
#else
void RenderFrameImpl::OnSelectPopupMenuItems(
@@ -6488,8 +6492,12 @@ void RenderFrameImpl::OnSelectPopupMenuItems(
return;

blink::WebScopedUserGesture gesture(frame_);
- external_popup_menu_->DidSelectItems(canceled, selected_indices);
- external_popup_menu_.reset();
+ // We need to reset |external_popup_menu_| before calling DidSelectItems(),
+ // which might delete |this|.
+ // See ExternalPopupMenuRemoveTest.RemoveFrameOnChange
+ std::unique_ptr<ExternalPopupMenu> popup;
+ popup.swap(external_popup_menu_);
+ popup->DidSelectItems(canceled, selected_indices);
}
#endif
#endif
diff --git a/content/renderer/render_frame_impl.h b/content/renderer/render_frame_impl.h
index cb5ffc56b005d8fb5492c1254bf533a370d42448..c9fea9da0cf2e24cd1c1581cbe939a736edd65bf 100644
--- a/content/renderer/render_frame_impl.h
+++ b/content/renderer/render_frame_impl.h
@@ -900,6 +900,7 @@ class CONTENT_EXPORT RenderFrameImpl
friend class RenderAccessibilityImplTest;
friend class TestRenderFrame;
FRIEND_TEST_ALL_PREFIXES(ExternalPopupMenuDisplayNoneTest, SelectItem);
+ FRIEND_TEST_ALL_PREFIXES(ExternalPopupMenuRemoveTest, RemoveFrameOnChange);
FRIEND_TEST_ALL_PREFIXES(ExternalPopupMenuRemoveTest, RemoveOnChange);
FRIEND_TEST_ALL_PREFIXES(ExternalPopupMenuTest, NormalCase);
FRIEND_TEST_ALL_PREFIXES(ExternalPopupMenuTest, ShowPopupThenNavigate);