Clear page display after navigation if no rendered output arrives

Clear page display after navigation if no rendered output arrives

A bug reporter found a way to navigate a page and cause the omnibar to
update with the destination's URL, but block the renderer from producing
any compositor frames using script. This has some implications for
phishing, even though the phisher's actual content has been unloaded
from the renderer process.

This CL creates a timeout which clears the last displayed compositor
frame if no update is received within 2 seconds after the navigation
being committed.

BUG=497588
Committed: https://crrev.com/3ddcd84850a1bfe9f4b3974f355abce66f4e35ab
Cr-Commit-Position: refs/heads/master@{#348902}

[kenrb, 2015-09-14 20:02:20]

kenrb@chromium.org changed reviewers:
+ creis@chromium.org

[kenrb, 2015-09-14 20:02:41]

Charlie: Do you have time to look at this?

[Charlie Reis, 2015-09-14 20:54:56]

Looks pretty good-- thanks for following up on this.  A few minor questions
below and we should be set.

https://codereview.chromium.org/1339803002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1339803002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:928:
frame_tree_node()->navigator()->DidNavigate(this, validated_params);
This is the line that changes the address bar.  There's some early returns
before this line for cases that we don't update the address bar, so we might
want to move your new code after this.

https://codereview.chromium.org/1339803002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/1339803002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:119: void
set_new_content_rendering_delay(const base::TimeDelta& delay) {
nit: "_for_testing"?  Or is it expected to be used by real code as well?

https://codereview.chromium.org/1339803002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:509: virtual void
NotifyNewContentRenderingTimeout() {}
nit: End name with "ForTesting" so that other code doesn't start using this.

https://codereview.chromium.org/1339803002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/1339803002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_unittest.cc:1127:
EXPECT_TRUE(host_->new_content_rendering_timeout_fired());
This is a nice test of the new timer API, though it doesn't cover how it's used
to fix the bug.  I'm guessing it's difficult to test that, though, unless
there's a way to check if the compositor frame has been cleared in a browser
test.  I'm probably ok with this as is unless you see a good way to add such a
test.

https://codereview.chromium.org/1339803002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/1339803002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:167: virtual void
ClearCompositorFrame() {}
Should this ever be left unimplemented, or should we make it pure virtual? 
Seems like it's a potential security issue to have some RHWVs that leave it as a
no-op.

[kenrb, 2015-09-14 21:29:36]

https://codereview.chromium.org/1339803002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1339803002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:928:
frame_tree_node()->navigator()->DidNavigate(this, validated_params);
On 2015/09/14 20:54:55, Charlie Reis (slow till 9-14) wrote:
> This is the line that changes the address bar.  There's some early returns
> before this line for cases that we don't update the address bar, so we might
> want to move your new code after this.

Done.

https://codereview.chromium.org/1339803002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/1339803002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:119: void
set_new_content_rendering_delay(const base::TimeDelta& delay) {
On 2015/09/14 20:54:55, Charlie Reis (slow till 9-14) wrote:
> nit: "_for_testing"?  Or is it expected to be used by real code as well?

Done.

https://codereview.chromium.org/1339803002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:509: virtual void
NotifyNewContentRenderingTimeout() {}
On 2015/09/14 20:54:55, Charlie Reis (slow till 9-14) wrote:
> nit: End name with "ForTesting" so that other code doesn't start using this.

Done.

https://codereview.chromium.org/1339803002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/1339803002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_unittest.cc:1127:
EXPECT_TRUE(host_->new_content_rendering_timeout_fired());
On 2015/09/14 20:54:55, Charlie Reis (slow till 9-14) wrote:
> This is a nice test of the new timer API, though it doesn't cover how it's
used
> to fix the bug.  I'm guessing it's difficult to test that, though, unless
> there's a way to check if the compositor frame has been cleared in a browser
> test.  I'm probably ok with this as is unless you see a good way to add such a
> test.

I was looking at this earlier, and I couldn't see a good way to do it. The
details of the frame being cleared are platform specific and not really
accessible to the test code.

https://codereview.chromium.org/1339803002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/1339803002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:167: virtual void
ClearCompositorFrame() {}
On 2015/09/14 20:54:55, Charlie Reis (slow till 9-14) wrote:
> Should this ever be left unimplemented, or should we make it pure virtual? 
> Seems like it's a potential security issue to have some RHWVs that leave it as
a
> no-op.

RenderWidgetHostViewChildFrame and RenderWidgetHostViewGuest do not implement
it, because the URL of their content is not displayed in the omnibar.

[Charlie Reis, 2015-09-14 21:40:18]

Thanks.  LGTM with one note.

https://codereview.chromium.org/1339803002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/1339803002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_unittest.cc:1127:
EXPECT_TRUE(host_->new_content_rendering_timeout_fired());
On 2015/09/14 21:29:36, kenrb wrote:
> On 2015/09/14 20:54:55, Charlie Reis (slow till 9-14) wrote:
> > This is a nice test of the new timer API, though it doesn't cover how it's
> used
> > to fix the bug.  I'm guessing it's difficult to test that, though, unless
> > there's a way to check if the compositor frame has been cleared in a browser
> > test.  I'm probably ok with this as is unless you see a good way to add such
a
> > test.
> 
> I was looking at this earlier, and I couldn't see a good way to do it. The
> details of the frame being cleared are platform specific and not really
> accessible to the test code.

Acknowledged.

https://codereview.chromium.org/1339803002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/1339803002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:167: virtual void
ClearCompositorFrame() {}
On 2015/09/14 21:29:36, kenrb wrote:
> On 2015/09/14 20:54:55, Charlie Reis (slow till 9-14) wrote:
> > Should this ever be left unimplemented, or should we make it pure virtual? 
> > Seems like it's a potential security issue to have some RHWVs that leave it
as
> a
> > no-op.
> 
> RenderWidgetHostViewChildFrame and RenderWidgetHostViewGuest do not implement
> it, because the URL of their content is not displayed in the omnibar.

One option would be to put an empty implementation in
RenderWidgetHostViewChildFrame (inherited by RWHVG) but still leave it pure
virtual here, so that future platform subclasses have to implement it.  In the
future, it seems more likely that we'll add more platform subclasses than more
"subframe" subclasses.

I think that might be worthwhile, but I'll defer to you if there's a reason not
to.

[kenrb, 2015-09-15 15:25:41]

https://codereview.chromium.org/1339803002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/1339803002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:167: virtual void
ClearCompositorFrame() {}
On 2015/09/14 21:40:18, Charlie Reis (slow till 9-14) wrote:
> On 2015/09/14 21:29:36, kenrb wrote:
> > On 2015/09/14 20:54:55, Charlie Reis (slow till 9-14) wrote:
> > > Should this ever be left unimplemented, or should we make it pure virtual?

> > > Seems like it's a potential security issue to have some RHWVs that leave
it
> as
> > a
> > > no-op.
> > 
> > RenderWidgetHostViewChildFrame and RenderWidgetHostViewGuest do not
implement
> > it, because the URL of their content is not displayed in the omnibar.
> 
> One option would be to put an empty implementation in
> RenderWidgetHostViewChildFrame (inherited by RWHVG) but still leave it pure
> virtual here, so that future platform subclasses have to implement it.  In the
> future, it seems more likely that we'll add more platform subclasses than more
> "subframe" subclasses.
> 
> I think that might be worthwhile, but I'll defer to you if there's a reason
not
> to.

This makes sense. I've made the change.

[kenrb, 2015-09-15 15:52:31]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2015-09-15 15:52:32]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1339803002/#ps100001
(title: "Made ClearCompositorFrame pure virtual")

[commit-bot: I haz the power, 2015-09-15 15:53:33]

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

[commit-bot: I haz the power, 2015-09-15 16:13:07]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-15 16:13:44]

Patchset 6 (id:??) landed as
https://crrev.com/3ddcd84850a1bfe9f4b3974f355abce66f4e35ab
Cr-Commit-Position: refs/heads/master@{#348902}

[commit-bot: I haz the power, 2015-09-23 12:45:40]

Patchset 6 (id:??) landed as
https://crrev.com/3ddcd84850a1bfe9f4b3974f355abce66f4e35ab
Cr-Commit-Position: refs/heads/master@{#348902}