Ignore InputMsg IPCs if RenderView is in swapped out state.

Ignore InputMsg IPCs if RenderView is in swapped out state.

InputMsg IPCs are dispatched to the compositor thread when they arrive
in the renderer process. If not handled there, they are sent back to the
main thread. This behavior allows for IPCs that are processed on the
main thread to come in order different than the browser process has sent
them in.

The crash in 541578 occurs when InputMsg_SetFocus is sent right before
FrameMsg_SwapOut. On the renderer side, the InputMsg_SetFocus message is
sent to the compositor thread and in the meantime the FrameMsg_SwapOut
message is processed on the main thread. After swapping out is complete
the top-level frame for the swapped out RenderView becomes a RemoteFrame
when swapped out RenderFrameHost usage is disabled. This causes the
FocusController to return RemoteFrame and that violates assumptions
elsewhere in code that all focused frames are LocalFrame.

Overall, swapped out RenderView should not be processing input messages
at all, so this CL implements this restriction.

BUG=357747, 541578
Committed: https://crrev.com/c6edf7e60d1d5dd6202fd778dbac8605fe71036d
Cr-Commit-Position: refs/heads/master@{#354426}

[nasko, 2015-10-15 22:08:37]

nasko@chromium.org changed reviewers:
+ alexmos@chromium.org

[nasko, 2015-10-15 22:08:37]

Hey Alex,
Can you review this CL for me? It implements the restriction that swapped out
RenderView should not be processing input IPCs.
Thanks in advance!
Nasko

[alexmos, 2015-10-15 22:52:38]

Great, I'm very glad to see this fixed!

https://codereview.chromium.org/1408873002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/1408873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:2138:
static_cast<WebContentsImpl*>(new_shell->web_contents());
nit: Is the cast to *Impl necessary?  Seems the rvh only exercises public APIs.

https://codereview.chromium.org/1408873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:2145:
new_shell->web_contents()->GetSiteInstance());
nit: is it also worth checking that rvh now corresponds to what
GetSwappedOutRenderViewHost(A) returns?

https://codereview.chromium.org/1408873002/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1408873002/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:1281: if
(IPC_MESSAGE_ID_CLASS(message.type()) == InputMsgStart)
Shouldn't there be an actual check here that the view is swapped out?

https://codereview.chromium.org/1408873002/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3076: CHECK(!enable ||
webview()->mainFrame()->isWebLocalFrame());
I wonder if "!enable" is needed.  If there's no currently focused frame,
FocusController::setFocused will set the main frame as focused regardless of the
flag value, so should we just always assert that this is called when the main
frame is local?

[nasko, 2015-10-15 23:18:26]

https://codereview.chromium.org/1408873002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/1408873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:2126:
embedded_test_server()->GetURL("a.com", "/title1.html"));
I missed to add EXPECT_TRUE on all calls to NavigateToURL, so added them in the
new patch.

https://codereview.chromium.org/1408873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:2138:
static_cast<WebContentsImpl*>(new_shell->web_contents());
On 2015/10/15 22:52:38, alexmos wrote:
> nit: Is the cast to *Impl necessary?  Seems the rvh only exercises public
APIs.

I either have to cast here or cast the call to GetRenderViewHost(), otherwise I
will get back a RenderViewHost. The Send method is not public on RVH.

https://codereview.chromium.org/1408873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:2145:
new_shell->web_contents()->GetSiteInstance());
On 2015/10/15 22:52:38, alexmos wrote:
> nit: is it also worth checking that rvh now corresponds to what
> GetSwappedOutRenderViewHost(A) returns?

Why not! Now I really need to cast the WebContents to the impl :).

https://codereview.chromium.org/1408873002/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1408873002/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:1281: if
(IPC_MESSAGE_ID_CLASS(message.type()) == InputMsgStart)
On 2015/10/15 22:52:38, alexmos wrote:
> Shouldn't there be an actual check here that the view is swapped out? 

d'oh!

https://codereview.chromium.org/1408873002/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3076: CHECK(!enable ||
webview()->mainFrame()->isWebLocalFrame());
On 2015/10/15 22:52:38, alexmos wrote:
> I wonder if "!enable" is needed.  If there's no currently focused frame,
> FocusController::setFocused will set the main frame as focused regardless of
the
> flag value, so should we just always assert that this is called when the main
> frame is local?

Done.

[nasko, 2015-10-15 23:18:39]

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

[commit-bot: I haz the power, 2015-10-15 23:19:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408873002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408873002/20001

[commit-bot: I haz the power, 2015-10-16 00:14:51]

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

[commit-bot: I haz the power, 2015-10-16 00:14:52]

Dry run: This issue passed the CQ dry run.

[alexmos, 2015-10-16 00:49:15]

LGTM

https://codereview.chromium.org/1408873002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/1408873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:2126:
embedded_test_server()->GetURL("a.com", "/title1.html"));
On 2015/10/15 23:18:26, nasko (slow to review) wrote:
> I missed to add EXPECT_TRUE on all calls to NavigateToURL, so added them in
the
> new patch.

Thanks!  Note to self: I should get back to http://crbug.com/425335 some day and
get this to have WARN_UNUSED_RESULT.

https://codereview.chromium.org/1408873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:2138:
static_cast<WebContentsImpl*>(new_shell->web_contents());
On 2015/10/15 23:18:26, nasko (slow to review) wrote:
> On 2015/10/15 22:52:38, alexmos wrote:
> > nit: Is the cast to *Impl necessary?  Seems the rvh only exercises public
> APIs.
> 
> I either have to cast here or cast the call to GetRenderViewHost(), otherwise
I
> will get back a RenderViewHost. The Send method is not public on RVH.

I thought Send() was public since RenderViewHost implements IPC::Sender, where
it is public.  But it doesn't matter now that you have the
GetSwappedOutRenderViewHost check.

[nasko, 2015-10-16 01:44:08]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2015-10-16 01:44:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408873002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408873002/20001

[commit-bot: I haz the power, 2015-10-16 01:48:53]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-16 01:49:53]

Patchset 2 (id:??) landed as
https://crrev.com/c6edf7e60d1d5dd6202fd778dbac8605fe71036d
Cr-Commit-Position: refs/heads/master@{#354426}