WebUI: Fix lifecycle-bug when RenderFrameHost is swapped out

WebUI: Fix lifecycle-bug when RenderFrameHost is swapped out

Currently, once the RenderFrameHost is swapped out, WebUI C++ -> JS
calls will trigger a crash.

These C++ -> JS calls are usually triggered by an observer action that
occurs on navigation. (For instance, updating the list of cookies in
the settings UI.)

This patch calls WebUIMessageHandler::DisallowJavascript once Swap Out
begins. This is a quick fix to the issue.

The ideal fix would be to make WebUI have a pointer to its owning
RenderFrameHost. However, that will have to wait until the UberUI is
removed.

BUG=615274
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/ff556e56a916f1b24d35573641b3a8f6c9d49884
Cr-Commit-Position: refs/heads/master@{#398898}

[tommycli, 2016-06-06 19:29:08]

Description was changed from

==========
WebUI: Fix lifecycle-bug when RenderFrameHost is swapped out

Currently, once the RenderFrameHost is swapped out, WebUI C++ -> JS
calls will trigger a crash.

These C++ -> JS calls are usually triggered by an observer action that
occurs on navigation. (For instance, updating the list of cookies in
the settings UI.)

This patch calls WebUIMessageHandler::DisallowJavascript once Swap Out
begins. This is a quick fix to the issue.

The ideal fix would be to make WebUI have a pointer to its owning
RenderFrameHost. However, that will have to wait until the UberUI is
removed.

BUG=615274
==========

to

==========
WebUI: Fix lifecycle-bug when RenderFrameHost is swapped out

Currently, once the RenderFrameHost is swapped out, WebUI C++ -> JS
calls will trigger a crash.

These C++ -> JS calls are usually triggered by an observer action that
occurs on navigation. (For instance, updating the list of cookies in
the settings UI.)

This patch calls WebUIMessageHandler::DisallowJavascript once Swap Out
begins. This is a quick fix to the issue.

The ideal fix would be to make WebUI have a pointer to its owning
RenderFrameHost. However, that will have to wait until the UberUI is
removed.

BUG=615274
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[tommycli, 2016-06-06 19:30:23]

tommycli@chromium.org changed reviewers:
+ creis@chromium.org, dbeam@chromium.org

[tommycli, 2016-06-06 19:30:23]

creis, dbeam: PTAL, this is the quick-fix we discussed. Thanks!

[Charlie Reis, 2016-06-06 21:09:52]

Thanks.  Would it be possible to include a test for this?  Seems like something
we'll want to prevent from regressing if we refactor it (e.g., giving WebUI a
pointer to its owning RFH).

Also, I was looking for a way to use something like
WebContentsObserver::RenderFrameHostChanged here instead, but I'm not seeing a
convenient way to do it.  I suppose this will do for now, unless you have ideas
along those lines.

https://codereview.chromium.org/2041993002/diff/1/content/public/browser/web_...
File content/public/browser/web_ui_message_handler.h (right):

https://codereview.chromium.org/2041993002/diff/1/content/public/browser/web_...
content/public/browser/web_ui_message_handler.h:110: void
RenderFrameHostSwappingOut();
Kind of unfortunate to be including something about swap out in content/public,
even if it's in the private section.  If it's not easy to avoid, can you include
a TODO to remove this when WebUI gets a pointer to its owning RFH (with a bug
number)?

[Dan Beam, 2016-06-06 23:41:47]

https://codereview.chromium.org/2041993002/diff/1/content/browser/webui/web_u...
File content/browser/webui/web_ui_impl.cc (right):

https://codereview.chromium.org/2041993002/diff/1/content/browser/webui/web_u...
content/browser/webui/web_ui_impl.cc:105: handler->RenderFrameHostSwappingOut();
why wouldn't we just call DisallowJavascript() directly here and skip changing
the /public/ API?

https://codereview.chromium.org/2041993002/diff/1/content/public/browser/web_...
File content/public/browser/web_ui_message_handler.h (right):

https://codereview.chromium.org/2041993002/diff/1/content/public/browser/web_...
content/public/browser/web_ui_message_handler.h:110: void
RenderFrameHostSwappingOut();
On 2016/06/06 21:09:52, Charlie Reis wrote:
> Kind of unfortunate to be including something about swap out in
content/public,
> even if it's in the private section.  If it's not easy to avoid, can you
include
> a TODO to remove this when WebUI gets a pointer to its owning RFH (with a bug
> number)?

i think it's easy to avoid

[tommycli, 2016-06-07 17:46:07]

Hey, I updated it to no longer modify the public interface.

As for a test - I don't see an easy place to do a unit test per se, only an
integration test.

https://codereview.chromium.org/2041993002/diff/1/content/browser/webui/web_u...
File content/browser/webui/web_ui_impl.cc (right):

https://codereview.chromium.org/2041993002/diff/1/content/browser/webui/web_u...
content/browser/webui/web_ui_impl.cc:105: handler->RenderFrameHostSwappingOut();
On 2016/06/06 23:41:46, Dan Beam wrote:
> why wouldn't we just call DisallowJavascript() directly here and skip changing
> the /public/ API?

Done.

https://codereview.chromium.org/2041993002/diff/1/content/public/browser/web_...
File content/public/browser/web_ui_message_handler.h (right):

https://codereview.chromium.org/2041993002/diff/1/content/public/browser/web_...
content/public/browser/web_ui_message_handler.h:110: void
RenderFrameHostSwappingOut();
On 2016/06/06 23:41:46, Dan Beam wrote:
> On 2016/06/06 21:09:52, Charlie Reis wrote:
> > Kind of unfortunate to be including something about swap out in
> content/public,
> > even if it's in the private section.  If it's not easy to avoid, can you
> include
> > a TODO to remove this when WebUI gets a pointer to its owning RFH (with a
bug
> > number)?
> 
> i think it's easy to avoid

Done.

[Charlie Reis, 2016-06-07 18:12:01]

On 2016/06/07 17:46:07, tommycli wrote:
> Hey, I updated it to no longer modify the public interface.
> 
> As for a test - I don't see an easy place to do a unit test per se, only an
> integration test.

I'm not opposed to an integration test.  :)  I do think we should have a
regression test for this if possible.  If it helps, you can call
RFHI::DisableSwapOutTimerForTesting to have a RenderFrameHost in the swap out
state for as long as you need (e.g., if you create an unload handler that
loops).  You can see CreateRenderViewAfterProcessKillAndClosedProxy for an
example.

[Dan Beam, 2016-06-08 00:21:39]

lgtm

[tommycli, 2016-06-08 18:14:14]

creis: Thanks. Based on the example you pointed out, I was able to write a
integration browsertest for this.

It could use some extra scrutiny, as I haven't written a RFH browsertest before.
Thanks!

[Charlie Reis, 2016-06-08 20:57:11]

Thanks!  LGTM with one flakiness fix.

https://codereview.chromium.org/2041993002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/2041993002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1874: //
This test ensures that after an RVH is swapped out, the associated WebUI
nit: RFH

https://codereview.chromium.org/2041993002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1877: // the
current main frame, rather than the RVH that owns the WebUI.
nit: RFH

https://codereview.chromium.org/2041993002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1916:
ASSERT_TRUE(rfh->IsRenderFrameLive());
I think this part will be flaky unless we guarantee the unload handler doesn't
finish.  It's possible for the actual SwapOut ACK to occur during
WaitForCommit(), since the message loop will finish processing any queued tasks
after we tell it to quit at commit time.

Just add something like the slow unload handler from line 2437 below to the
WebUI RFH (before navigating away) and you should be set.

[tommycli, 2016-06-08 22:52:34]

creis: thanks!

https://codereview.chromium.org/2041993002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/2041993002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1874: //
This test ensures that after an RVH is swapped out, the associated WebUI
On 2016/06/08 20:57:11, Charlie Reis wrote:
> nit: RFH

Done.

https://codereview.chromium.org/2041993002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1877: // the
current main frame, rather than the RVH that owns the WebUI.
On 2016/06/08 20:57:11, Charlie Reis wrote:
> nit: RFH

Done.

https://codereview.chromium.org/2041993002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1916:
ASSERT_TRUE(rfh->IsRenderFrameLive());
On 2016/06/08 20:57:11, Charlie Reis wrote:
> I think this part will be flaky unless we guarantee the unload handler doesn't
> finish.  It's possible for the actual SwapOut ACK to occur during
> WaitForCommit(), since the message loop will finish processing any queued
tasks
> after we tell it to quit at commit time.
> 
> Just add something like the slow unload handler from line 2437 below to the
> WebUI RFH (before navigating away) and you should be set.

Done.

[tommycli, 2016-06-08 22:52:52]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-06-08 22:52:52]

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

[commit-bot: I haz the power, 2016-06-08 22:53:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041993002/60001

[commit-bot: I haz the power, 2016-06-09 00:50:17]

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

[commit-bot: I haz the power, 2016-06-09 00:50:19]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tommycli, 2016-06-09 15:20:13]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-06-09 15:20:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041993002/60001

[commit-bot: I haz the power, 2016-06-09 15:55:03]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-09 15:55:04]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-09 15:56:24]

Description was changed from

==========
WebUI: Fix lifecycle-bug when RenderFrameHost is swapped out

Currently, once the RenderFrameHost is swapped out, WebUI C++ -> JS
calls will trigger a crash.

These C++ -> JS calls are usually triggered by an observer action that
occurs on navigation. (For instance, updating the list of cookies in
the settings UI.)

This patch calls WebUIMessageHandler::DisallowJavascript once Swap Out
begins. This is a quick fix to the issue.

The ideal fix would be to make WebUI have a pointer to its owning
RenderFrameHost. However, that will have to wait until the UberUI is
removed.

BUG=615274
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
WebUI: Fix lifecycle-bug when RenderFrameHost is swapped out

Currently, once the RenderFrameHost is swapped out, WebUI C++ -> JS
calls will trigger a crash.

These C++ -> JS calls are usually triggered by an observer action that
occurs on navigation. (For instance, updating the list of cookies in
the settings UI.)

This patch calls WebUIMessageHandler::DisallowJavascript once Swap Out
begins. This is a quick fix to the issue.

The ideal fix would be to make WebUI have a pointer to its owning
RenderFrameHost. However, that will have to wait until the UberUI is
removed.

BUG=615274
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/ff556e56a916f1b24d35573641b3a8f6c9d49884
Cr-Commit-Position: refs/heads/master@{#398898}
==========

[commit-bot: I haz the power, 2016-06-09 15:56:25]

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