Adding tests for two recent regressions due to RenderViewImpl's Swapped Out State

Adding tests for two recent regressions due to RenderViewImpl's Swapped Out State

Due to RenderViewImpl::GetWebWidget() returning a WebViewImpl in a swapped out state, a few
regressions were caused where an incoming IPC (which should not have been sent by the browser
in the first place) where handled incorrectly by the renderer and lead to renderer crashes.

This CL will add a test to verify such IPCs do not lead to a crash. This test should be removed
later one when we make sure such IPCs will never be sent by the browser in the given state (perhaps
then the test should move to the browser side and verify we do not send those IPCs).

Some of the IPCs involved relate to IME and TextInputClientMac.

BUG=664890, 668106, 669219, 680438, 683098
Review-Url: https://codereview.chromium.org/2656433002
Cr-Commit-Position: refs/heads/master@{#448175}
Committed: https://chromium.googlesource.com/chromium/src/+/9f554a235d98d212b0612cc8188c13de2884fed9

[EhsanK, 2017-01-23 19:37:11]

Patchset #1 (id:1) has been deleted

[EhsanK, 2017-01-23 19:39:00]

Description was changed from

==========
Adding tests for two recent regressions due to RenderViewImpl's Swapped Out
State

Due to RenderViewImpl::GetWebWidget() returning a WebViewImpl in a swapped out
state, a few
regressions were caused where an incoming IPC (which should not have been sent
by the browser
in the first place) where handled incorrectly by the renderer and lead to
renderer crashes.

This CL will add a test to verify such IPCs do not lead to a crash. This test
should be removed
later one when we make sure such IPCs will never be sent by the browser in the
given state (perhaps
then the test should move to the browser side and verify we do not send those
IPCs).

Some of the IPCs involved relate to IME and TextInputClientMac.

BUG=680438,683098
==========

to

==========
Adding tests for two recent regressions due to RenderViewImpl's Swapped Out
State

Due to RenderViewImpl::GetWebWidget() returning a WebViewImpl in a swapped out
state, a few
regressions were caused where an incoming IPC (which should not have been sent
by the browser
in the first place) where handled incorrectly by the renderer and lead to
renderer crashes.

This CL will add a test to verify such IPCs do not lead to a crash. This test
should be removed
later one when we make sure such IPCs will never be sent by the browser in the
given state (perhaps
then the test should move to the browser side and verify we do not send those
IPCs).

Some of the IPCs involved relate to IME and TextInputClientMac.

BUG=680438,683098, 669219
==========

[EhsanK, 2017-01-23 19:42:31]

Patchset #1 (id:20001) has been deleted

[EhsanK, 2017-01-23 19:45:27]

ekaramad@chromium.org changed reviewers:
+ avi@chromium.org

[EhsanK, 2017-01-23 19:45:28]

Hello Avi,

Please take a look. Thanks!

FYI: My understanding is that we still send such IPCs in the swapped out state.
I had filed a bug on that but not sure if we have another bug for tracking the
issue or not (bug: https://crbug.com/669219). This test is supposed to be
temporary since if we decide not to send those IPCs in the swapped out state
(which seems reasonable), it will obviate the need for a test such as this one.

[EhsanK, 2017-01-23 19:45:48]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 19:46:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 19:57:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 19:57:07]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Avi (use Gerrit), 2017-02-03 21:39:47]

yay tests! LGTM

[EhsanK, 2017-02-05 01:07:00]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-05 01:07:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[EhsanK, 2017-02-05 01:08:39]

Description was changed from

==========
Adding tests for two recent regressions due to RenderViewImpl's Swapped Out
State

Due to RenderViewImpl::GetWebWidget() returning a WebViewImpl in a swapped out
state, a few
regressions were caused where an incoming IPC (which should not have been sent
by the browser
in the first place) where handled incorrectly by the renderer and lead to
renderer crashes.

This CL will add a test to verify such IPCs do not lead to a crash. This test
should be removed
later one when we make sure such IPCs will never be sent by the browser in the
given state (perhaps
then the test should move to the browser side and verify we do not send those
IPCs).

Some of the IPCs involved relate to IME and TextInputClientMac.

BUG=680438,683098, 669219
==========

to

==========
Adding tests for two recent regressions due to RenderViewImpl's Swapped Out
State

Due to RenderViewImpl::GetWebWidget() returning a WebViewImpl in a swapped out
state, a few
regressions were caused where an incoming IPC (which should not have been sent
by the browser
in the first place) where handled incorrectly by the renderer and lead to
renderer crashes.

This CL will add a test to verify such IPCs do not lead to a crash. This test
should be removed
later one when we make sure such IPCs will never be sent by the browser in the
given state (perhaps
then the test should move to the browser side and verify we do not send those
IPCs).

Some of the IPCs involved relate to IME and TextInputClientMac.

BUG=664890, 668106, 669219, 680438,683098
==========

[commit-bot: I haz the power, 2017-02-05 01:19:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-05 01:19:27]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2017-02-05 01:20:25]

The CQ bit was checked by ekaramad@chromium.org

[commit-bot: I haz the power, 2017-02-05 01:20:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-05 01:24:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-05 01:24:37]

Failed to apply patch for content/renderer/render_view_browsertest_mac.mm:
While running git apply --index -p1;
  error: patch failed: content/renderer/render_view_browsertest_mac.mm:148
  error: content/renderer/render_view_browsertest_mac.mm: patch does not apply

Patch:       content/renderer/render_view_browsertest_mac.mm
Index: content/renderer/render_view_browsertest_mac.mm
diff --git a/content/renderer/render_view_browsertest_mac.mm
b/content/renderer/render_view_browsertest_mac.mm
index
4e1b73208327378b38bdd20a33a608d698a295e1..3bc240fe78dffc81fdf09c07f1250ad489d60413
100644
--- a/content/renderer/render_view_browsertest_mac.mm
+++ b/content/renderer/render_view_browsertest_mac.mm
@@ -5,6 +5,10 @@
 #include "base/strings/string16.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "content/common/frame_messages.h"
+#include "content/common/frame_replication_state.h"
+#include "content/common/input_messages.h"
+#include "content/common/text_input_client_messages.h"
 #include "content/public/browser/native_web_keyboard_event.h"
 #include "content/public/common/web_preferences.h"
 #include "content/public/test/render_view_test.h"
@@ -18,6 +22,7 @@
 #include <Cocoa/Cocoa.h>
 
 using blink::WebFrameContentDumper;
+using blink::WebCompositionUnderline;
 
 namespace content {
 
@@ -148,4 +153,48 @@ TEST_F(RenderViewTest, MacTestCmdUp) {
   EXPECT_EQ(kArrowUpNoScroll, base::UTF16ToASCII(output));
 }
 
+// TODO(ekaramad): This test could be removed once we do not send irrelevant
+// IPCs from browser during the time RenderViewImpl is swapped out
+// (https://crbug.com/669219).
+// This test verfies that when RenderViewImpl is swapped out, handling IPCs
+// which need a WebFrameWidget will not lead to a crash.
+TEST_F(RenderViewTest, HandleIPCsInSwappedOutState) {
+  LoadHTML("<input/>");
+
+  // Normally, we have a WebFrameWidget.
+  EXPECT_TRUE(GetWebWidget()->isWebFrameWidget());
+
+  // Swap out the main frame so that the frame widget is destroyed.
+  auto* view = static_cast<RenderViewImpl*>(view_);
+  auto* main_frame = view->GetMainRenderFrame();
+  main_frame->OnMessageReceived(FrameMsg_SwapOut(
+      main_frame->GetRoutingID(), 123, true, FrameReplicationState()));
+
+  // We no longer have a frame widget.
+  EXPECT_FALSE(GetWebWidget()->isWebFrameWidget());
+
+  int routing_id = view->GetRoutingID();
+  // Now simulate some TextInputClientMac IPCs. These will be handled by
+  // RenderWidget which forwards them to the TextInputClientObserver
+  using Range = gfx::Range;
+  using Point = gfx::Point;
+  view->OnMessageReceived(
+      TextInputClientMsg_CharacterIndexForPoint(routing_id, Point()));
+  view->OnMessageReceived(
+      TextInputClientMsg_FirstRectForCharacterRange(routing_id, Range()));
+  view->OnMessageReceived(
+      TextInputClientMsg_StringForRange(routing_id, Range()));
+  view->OnMessageReceived(
+      TextInputClientMsg_CharacterIndexForPoint(routing_id, Point()));
+
+  // Simulate some IME related IPCs.
+  using Text = base::string16;
+  using Underlines = std::vector<blink::WebCompositionUnderline>;
+  view->OnMessageReceived(InputMsg_ImeSetComposition(
+      routing_id, Text(), Underlines(), Range(), 0, 0));
+  view->OnMessageReceived(
+      InputMsg_ImeCommitText(routing_id, Text(), Underlines(), Range(), 0));
+  view->OnMessageReceived(InputMsg_ImeFinishComposingText(routing_id, false));
+}
+
 }  // namespace content

[EhsanK, 2017-02-05 02:40:27]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-05 02:40:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-05 02:55:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-05 02:55:10]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2017-02-05 04:15:42]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2017-02-05 04:15:42]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2656433002/#ps60001
(title: "Rebased")

[commit-bot: I haz the power, 2017-02-05 04:15:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-05 04:34:01]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486268142122950,
"parent_rev": "23da976a5a8354e876e523c942aa0350f404bd74", "commit_rev":
"9f554a235d98d212b0612cc8188c13de2884fed9"}

[commit-bot: I haz the power, 2017-02-05 04:34:59]

Description was changed from

==========
Adding tests for two recent regressions due to RenderViewImpl's Swapped Out
State

Due to RenderViewImpl::GetWebWidget() returning a WebViewImpl in a swapped out
state, a few
regressions were caused where an incoming IPC (which should not have been sent
by the browser
in the first place) where handled incorrectly by the renderer and lead to
renderer crashes.

This CL will add a test to verify such IPCs do not lead to a crash. This test
should be removed
later one when we make sure such IPCs will never be sent by the browser in the
given state (perhaps
then the test should move to the browser side and verify we do not send those
IPCs).

Some of the IPCs involved relate to IME and TextInputClientMac.

BUG=664890, 668106, 669219, 680438,683098
==========

to

==========
Adding tests for two recent regressions due to RenderViewImpl's Swapped Out
State

Due to RenderViewImpl::GetWebWidget() returning a WebViewImpl in a swapped out
state, a few
regressions were caused where an incoming IPC (which should not have been sent
by the browser
in the first place) where handled incorrectly by the renderer and lead to
renderer crashes.

This CL will add a test to verify such IPCs do not lead to a crash. This test
should be removed
later one when we make sure such IPCs will never be sent by the browser in the
given state (perhaps
then the test should move to the browser side and verify we do not send those
IPCs).

Some of the IPCs involved relate to IME and TextInputClientMac.

BUG=664890, 668106, 669219, 680438,683098

Review-Url: https://codereview.chromium.org/2656433002
Cr-Commit-Position: refs/heads/master@{#448175}
Committed:
https://chromium.googlesource.com/chromium/src/+/9f554a235d98d212b0612cc8188c...
==========

[commit-bot: I haz the power, 2017-02-05 04:35:00]

Committed patchset #2 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/9f554a235d98d212b0612cc8188c...