Skip to content

Commit

Permalink
[mac] Mojofy TextInputClientMsg_StringForRange
Browse files Browse the repository at this point in the history 
This CL replaces TextInputClientMsg_StringForRange and
TextInputClientReplyMsg_StringForRange IPC messages with
GetStringForRange Mojo method in the LocalFrame interface.
GetStringForRange gets a range and returns the word in the range.

It cleans up text_input_client_observer.{cc,h} and
text_input_client_message_filter.{cc,h} and replaces
TestTextInputClientMessageFilter with TextInputTestLocalFrame
to support testing GetStringForRange.

Bug: 1007365
Change-Id: I7ef37d71e1642cc4c05e1f942d97acc1ea1e4cd9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2269628
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Julie Kim <jkim@igalia.com>
Cr-Commit-Position: refs/heads/master@{#785645}
  • Loading branch information
@jkim-julie
jkim-julie authored and Commit Bot committed Jul 7, 2020
1 parent 310dbdf commit a4bc40d
Show file tree
Hide file tree
Showing 41 changed files with 366 additions and 874 deletions.
61 changes: 5 additions & 56 deletions chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
#include "build/build_config.h"
#include "chrome/app/chrome_command_ids.h"
#include "chrome/browser/apps/platform_apps/app_browsertest_util.h"
#include "chrome/browser/chrome_content_browser_client.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.h"
#include "chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h"
Expand Down Expand Up @@ -69,49 +68,6 @@ using guest_view::TestGuestViewManager;
using guest_view::TestGuestViewManagerFactory;

#if defined(OS_MACOSX)
// The original TextInputClientMessageFilter is added during the initialization
// phase of RenderProcessHost. The only chance we have to add the test filter
// (so that it can receive the TextInputClientMac incoming IPC messages) is
// during the call to RenderProcessWillLaunch() on ContentBrowserClient. This
// class provides that for testing. The class also replaces the current client
// and will reset the client back to the original one upon destruction.
class BrowserClientForTextInputClientMac : public ChromeContentBrowserClient {
public:
BrowserClientForTextInputClientMac()
: old_client_(content::SetBrowserClientForTesting(this)) {}
~BrowserClientForTextInputClientMac() override {
content::SetBrowserClientForTesting(old_client_);
}

// ContentBrowserClient overrides.
void RenderProcessWillLaunch(
content::RenderProcessHost* process_host) override {
ChromeContentBrowserClient::RenderProcessWillLaunch(process_host);
filters_.push_back(
new content::TestTextInputClientMessageFilter(process_host));
}

// Retrieves the registered filter for the given RenderProcessHost. It will
// return false if the RenderProcessHost was initialized while a different
// instance of ContentBrowserClient was in action.
scoped_refptr<content::TestTextInputClientMessageFilter>
GetTextInputClientMessageFilterForProcess(
content::RenderProcessHost* process_host) const {
for (auto filter : filters_) {
if (filter->process() == process_host)
return filter;
}
return nullptr;
}

private:
content::ContentBrowserClient* old_client_;
std::vector<scoped_refptr<content::TestTextInputClientMessageFilter>>
filters_;

DISALLOW_COPY_AND_ASSIGN(BrowserClientForTextInputClientMac);
};

// This class observes the RenderWidgetHostViewCocoa corresponding to the outer
// most WebContents provided for newly added subviews. The added subview
// corresponds to a NSPopUpButtonCell which will be removed shortly after being
Expand Down Expand Up @@ -1518,22 +1474,15 @@ IN_PROC_BROWSER_TEST_F(WebViewInteractiveTest, TextSelection) {
}

// Verifies that asking for a word lookup from a guest will lead to a returned
// IPC from the renderer containing the right selected word.
// mojo callback from the renderer containing the right selected word.
IN_PROC_BROWSER_TEST_F(WebViewInteractiveTest, WordLookup) {
// BrowserClientForTextInputClientMac needs to replace the
// ChromeContentBrowserClient after most things are initialized but before the
// WebContents is created.
BrowserClientForTextInputClientMac browser_client;

SetupTest("web_view/text_selection",
"/extensions/platform_apps/web_view/text_selection/guest.html");
ASSERT_TRUE(guest_web_contents());
ASSERT_TRUE(ui_test_utils::ShowAndFocusNativeWindow(GetPlatformAppWindow()));

auto guest_message_filter =
browser_client.GetTextInputClientMessageFilterForProcess(
guest_web_contents()->GetMainFrame()->GetProcess());
ASSERT_TRUE(guest_message_filter);
content::TextInputTestLocalFrame text_input_local_frame;
text_input_local_frame.SetUp(guest_web_contents()->GetMainFrame());

// Lookup some string through context menu.
ContextMenuNotificationObserver menu_observer(IDC_CONTENT_CONTEXT_LOOK_UP);
Expand All @@ -1542,10 +1491,10 @@ IN_PROC_BROWSER_TEST_F(WebViewInteractiveTest, WordLookup) {
SimulateRWHMouseClick(guest_web_contents()->GetRenderViewHost()->GetWidget(),
blink::WebMouseEvent::Button::kRight, 20, 20);
// Wait for the response form the guest renderer.
guest_message_filter->WaitForStringFromRange();
text_input_local_frame.WaitForGetStringForRange();

// Sanity check.
ASSERT_EQ("AAAA", guest_message_filter->string_from_range().substr(0, 4));
ASSERT_EQ("AAAA", text_input_local_frame.GetStringFromRange().substr(0, 4));
}
#endif

Expand Down
121 changes: 20 additions & 101 deletions chrome/browser/site_isolation/site_per_process_text_input_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
#include "base/strings/stringprintf.h"
#include "base/strings/utf_string_conversions.h"
#include "build/build_config.h"
#include "chrome/browser/chrome_content_browser_client.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_window.h"
#include "chrome/browser/ui/location_bar/location_bar.h"
Expand Down Expand Up @@ -1354,89 +1353,12 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessTextInputManagerTest,
}
}

// The original TextInputClientMessageFilter is added during the initialization
// phase of RenderProcessHost. The only chance we have to add the test filter
// (so that it can receive the TextInputClientMac incoming IPC messages) is
// during the call to RenderProcessWillLaunch() on ContentBrowserClient. This
// class provides that for testing.
class TestBrowserClient : public ChromeContentBrowserClient {
public:
TestBrowserClient() {
old_client_ = content::SetBrowserClientForTesting(this);
}

~TestBrowserClient() override {
content::SetBrowserClientForTesting(old_client_);
}

// ContentBrowserClient overrides.
void RenderProcessWillLaunch(
content::RenderProcessHost* process_host) override {
ChromeContentBrowserClient::RenderProcessWillLaunch(process_host);
filters_.push_back(
new content::TestTextInputClientMessageFilter(process_host));
}

// Retrieves the registered filter for the given RenderProcessHost. It will
// return false if the RenderProcessHost was initialized while a different
// instance of ContentBrowserClient was in action.
scoped_refptr<content::TestTextInputClientMessageFilter>
GetTextInputClientMessageFilterForProcess(
content::RenderProcessHost* process_host) const {
for (auto filter : filters_) {
if (filter->process() == process_host)
return filter;
}
return nullptr;
}

private:
content::ContentBrowserClient* old_client_ = nullptr;
std::vector<scoped_refptr<content::TestTextInputClientMessageFilter>>
filters_;

DISALLOW_COPY_AND_ASSIGN(TestBrowserClient);
};

// Earlier injection of TestBrowserClient (a ContentBrowserClient) is needed to
// make sure it is active during creation of the first spare RenderProcessHost.
// Without this change, the tests would be surprised that they cannot find an
// injected message filter via GetTextInputClientMessageFilterForProcess.
class SitePerProcessCustomTextInputManagerFilteringTest
: public SitePerProcessTextInputManagerTest {
public:
SitePerProcessCustomTextInputManagerFilteringTest() {}
~SitePerProcessCustomTextInputManagerFilteringTest() override {}

void CreatedBrowserMainParts(content::BrowserMainParts* parts) override {
SitePerProcessTextInputManagerTest::CreatedBrowserMainParts(parts);
browser_client_ = std::make_unique<TestBrowserClient>();
}

void TearDown() override {
browser_client_.reset();
SitePerProcessTextInputManagerTest::TearDown();
}

scoped_refptr<content::TestTextInputClientMessageFilter>
GetTextInputClientMessageFilterForProcess(
content::RenderProcessHost* process_host) const {
return browser_client_->GetTextInputClientMessageFilterForProcess(
process_host);
}

private:
std::unique_ptr<TestBrowserClient> browser_client_;

DISALLOW_COPY_AND_ASSIGN(SitePerProcessCustomTextInputManagerFilteringTest);
};

// This test verifies that when a word lookup result comes from the renderer
// after the target RenderWidgetHost has been deleted, the browser will not
// crash. This test covers the case where the target RenderWidgetHost is that of
// an OOPIF.
IN_PROC_BROWSER_TEST_F(
SitePerProcessCustomTextInputManagerFilteringTest,
SitePerProcessTextInputManagerTest,
DoNotCrashBrowserInWordLookUpForDestroyedWidget_ChildFrame) {
std::unique_ptr<content::WebContents> new_contents =
content::WebContents::Create(content::WebContents::CreateParams(
Expand All @@ -1457,25 +1379,23 @@ IN_PROC_BROWSER_TEST_F(
"document.querySelector('input').focus();"
"document.querySelector('input').select();"));

content::RenderWidgetHostView* child_view = child_frame->GetView();
scoped_refptr<content::TestTextInputClientMessageFilter>
child_message_filter = GetTextInputClientMessageFilterForProcess(
child_view->GetRenderWidgetHost()->GetProcess());
DCHECK(child_message_filter);
content::TextInputTestLocalFrame text_input_local_frame;
text_input_local_frame.SetUp(child_frame);

// We need to wait for test scenario to complete before leaving this block.
base::RunLoop test_complete_waiter;

// Destroy the RenderWidgetHost from the browser side right after the
// dictionary IPC is received. The destruction is post tasked to UI thread.
int32_t child_process_id =
child_view->GetRenderWidgetHost()->GetProcess()->GetID();
// dictionary message is received. The destruction is post tasked to UI
// thread.
int32_t child_process_id = child_frame->GetProcess()->GetID();
int32_t child_frame_routing_id = child_frame->GetRoutingID();
child_message_filter->SetStringForRangeCallback(base::Bind(

text_input_local_frame.SetStringForRangeCallback(base::Bind(
[](int32_t process_id, int32_t routing_id,
const base::Closure& callback_on_io) {
// This runs before TextInputClientMac gets to handle the IPC. Then,
// by the time TextInputClientMac calls back into UI to show the
// This runs before TextInputClientMac gets to handle the mojo message.
// Then, by the time TextInputClientMac calls back into UI to show the
// dictionary, the target RWH is already destroyed which will be a
// close enough repro for the crash in https://crbug.com/737032.
ASSERT_TRUE(content::DestroyRenderWidgetHost(process_id, routing_id));
Expand Down Expand Up @@ -1506,7 +1426,7 @@ IN_PROC_BROWSER_TEST_F(
// crash. This test covers the case where the target RenderWidgetHost is that of
// the main frame (no OOPIFs on page).
IN_PROC_BROWSER_TEST_F(
SitePerProcessCustomTextInputManagerFilteringTest,
SitePerProcessTextInputManagerTest,
DoNotCrashBrowserInWordLookUpForDestroyedWidget_MainFrame) {
std::unique_ptr<content::WebContents> new_contents =
content::WebContents::Create(content::WebContents::CreateParams(
Expand All @@ -1526,25 +1446,24 @@ IN_PROC_BROWSER_TEST_F(
"document.querySelector('input').focus();"
"document.querySelector('input').select();"));

content::TextInputTestLocalFrame text_input_local_frame;
text_input_local_frame.SetUp(main_frame);

content::RenderWidgetHostView* page_rwhv = main_frame->GetView();
scoped_refptr<content::TestTextInputClientMessageFilter> message_filter =
GetTextInputClientMessageFilterForProcess(
page_rwhv->GetRenderWidgetHost()->GetProcess());
DCHECK(message_filter);

// We need to wait for test scenario to complete before leaving this block.
base::RunLoop test_complete_waiter;

// Destroy the RenderWidgetHost from the browser side right after the
// dictionary IPC is received. The destruction is post tasked to UI thread.
int32_t main_frame_process_id =
page_rwhv->GetRenderWidgetHost()->GetProcess()->GetID();
// dictionary message is received. The destruction is post tasked to UI
// thread.
int32_t main_frame_process_id = main_frame->GetProcess()->GetID();
int32_t main_frame_routing_id = main_frame->GetRoutingID();
message_filter->SetStringForRangeCallback(base::Bind(
text_input_local_frame.SetStringForRangeCallback(base::Bind(
[](int32_t process_id, int32_t routing_id,
const base::Closure& callback_on_io) {
// This runs before TextInputClientMac gets to handle the IPC. Then,
// by the time TextInputClientMac calls back into UI to show the
// This runs before TextInputClientMac gets to handle the mojo message.
// Then, by the time TextInputClientMac calls back into UI to show the
// dictionary, the target RWH is already destroyed which will be a
// close enough repro for the crash in https://crbug.com/737032.
ASSERT_TRUE(content::DestroyRenderWidgetHost(process_id, routing_id));
Expand Down
2 changes: 0 additions & 2 deletions content/browser/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1611,8 +1611,6 @@ jumbo_source_set("browser") {
"renderer_host/render_widget_targeter.h",
"renderer_host/text_input_client_mac.h",
"renderer_host/text_input_client_mac.mm",
"renderer_host/text_input_client_message_filter.h",
"renderer_host/text_input_client_message_filter.mm",
"renderer_host/text_input_host_impl.h",
"renderer_host/text_input_host_impl.mm",
"renderer_host/text_input_manager.cc",
Expand Down
1 change: 1 addition & 0 deletions content/browser/frame_host/render_frame_host_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1767,6 +1767,7 @@ class CONTENT_EXPORT RenderFrameHostImpl
friend class RenderFrameHostFeaturePolicyTest;
friend class TestRenderFrameHost;
friend class TestRenderViewHost;
friend class TextInputTestLocalFrame;
friend class WebContentsSplitCacheBrowserTest;

FRIEND_TEST_ALL_PREFIXES(NavigatorTest, TwoNavigationsRacingCommit);
Expand Down
4 changes: 0 additions & 4 deletions content/browser/renderer_host/render_process_host_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,6 @@

#if defined(OS_MACOSX)
#include "content/browser/child_process_task_port_provider_mac.h"
#include "content/browser/renderer_host/text_input_client_message_filter.h"
#include "content/browser/sandbox_support_mac_impl.h"
#include "content/common/sandbox_support_mac.mojom.h"
#endif
Expand Down Expand Up @@ -1988,9 +1987,6 @@ void RenderProcessHostImpl::CreateMessageFilters() {
#if BUILDFLAG(ENABLE_PLUGINS)
AddFilter(new PepperRendererConnection(GetID()));
#endif
#if defined(OS_MACOSX)
AddFilter(new TextInputClientMessageFilter());
#endif

p2p_socket_dispatcher_host_ =
std::make_unique<P2PSocketDispatcherHost>(GetID());
Expand Down
8 changes: 0 additions & 8 deletions content/browser/renderer_host/text_input_client_mac.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,11 +98,6 @@ class CONTENT_EXPORT TextInputClientMac {
const gfx::Range& range,
GetStringCallback callback);

// This is called on the IO thread when we get the renderer's reply for
// GetStringFromRange.
void GetStringFromRangeReply(ui::mojom::AttributedStringPtr string,
const gfx::Point& point);

private:
friend struct base::DefaultSingletonTraits<TextInputClientMac>;
TextInputClientMac();
Expand All @@ -123,9 +118,6 @@ class CONTENT_EXPORT TextInputClientMac {
base::Lock lock_;
base::ConditionVariable condition_;

// The callback when received IPC TextInputClientReplyMsg_GotStringForRange.
GetStringCallback replyForRangeHandler_;

DISALLOW_COPY_AND_ASSIGN(TextInputClientMac);
};

Expand Down
43 changes: 12 additions & 31 deletions content/browser/renderer_host/text_input_client_mac.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,31 +15,17 @@
#include "content/browser/frame_host/render_frame_host_impl.h"
#include "content/browser/renderer_host/render_widget_host_delegate.h"
#include "content/browser/renderer_host/render_widget_host_impl.h"
#include "content/common/text_input_client_messages.h"
#include "ui/base/mojom/attributed_string.mojom.h"

namespace content {

namespace {

// TODO(ekaramad): TextInputClientObserver, the renderer side of
// TextInputClientMac for each RenderWidgetHost, expects to have a
// WebFrameWidget to use for handling these IPCs. However, for fullscreen flash,
// we end up with a PepperWidget. For those scenarios, do not send the IPCs. We
// need to figure out what features are properly supported and perhaps send the
// IPC to the parent widget of the plugin (https://crbug.com/663384).
bool SendMessageToRenderWidget(RenderWidgetHostImpl* widget,
IPC::Message* message) {
if (!widget->delegate() ||
widget == widget->delegate()->GetFullscreenRenderWidgetHost()) {
delete message;
return false;
}

DCHECK_EQ(widget->GetRoutingID(), message->routing_id());
return widget->Send(message);
}

// TODO(ekaramad): TextInputClientMac expects to have a RenderWidgetHost to use
// for handling mojo calls. However, for fullscreen flash, we end up with a
// PepperWidget. For those scenarios, do not send the mojo calls. We need to
// figure out what features are properly supported and perhaps send the
// mojo call to the parent widget of the plugin (https://crbug.com/663384).
bool IsFullScreenRenderWidget(RenderWidgetHost* widget) {
RenderWidgetHostImpl* rwhi = RenderWidgetHostImpl::From(widget);
if (!rwhi->delegate() ||
Expand Down Expand Up @@ -86,19 +72,14 @@ bool IsFullScreenRenderWidget(RenderWidgetHost* widget) {
void TextInputClientMac::GetStringFromRange(RenderWidgetHost* rwh,
const gfx::Range& range,
GetStringCallback callback) {
DCHECK(!replyForRangeHandler_);
replyForRangeHandler_ = std::move(callback);
RenderWidgetHostImpl* rwhi = RenderWidgetHostImpl::From(rwh);
SendMessageToRenderWidget(
rwhi, new TextInputClientMsg_StringForRange(rwhi->GetRoutingID(), range));
}
RenderFrameHostImpl* rfhi = GetFocusedRenderFrameHostImpl(rwh);
// If it doesn't have a focused frame, it calls |callback| with
// an empty string and point.
if (!rfhi)
return std::move(callback).Run(nullptr, gfx::Point());

void TextInputClientMac::GetStringFromRangeReply(
ui::mojom::AttributedStringPtr string,
const gfx::Point& point) {
if (replyForRangeHandler_) {
std::move(replyForRangeHandler_).Run(std::move(string), point);
}
rfhi->GetAssociatedLocalFrame()->GetStringForRange(range,
std::move(callback));
}

uint32_t TextInputClientMac::GetCharacterIndexAtPoint(RenderWidgetHost* rwh,
Expand Down

0 comments on commit a4bc40d

Please sign in to comment.