Discard compositor frames from unloaded web content

Discard compositor frames from unloaded web content

After a navigation commits, it is still possible for the compositor to
resend a frame that draws the previous (now unloaded) page. In some
situations this works against the browser process' new content
rendering timer that attempts to clear a page after 4 seconds if a
navigation has committed but not yet painted in that time.

This CL adds a counter called content_source_id to that increments on
every non-same-page navigation commit under a given top-level
RenderWidget. It tags the CompositorFrameMetadata with this identifier,
and also sends it to the browser process with the
DidCommitProvisionaLoad IPC message. This enables the browser to
recognize compositor frames with stale IDs, corresponding to unloaded
pages.

BUG=672847
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2707243005
Cr-Commit-Position: refs/heads/master@{#454578}
Committed: https://chromium.googlesource.com/chromium/src/+/efd1189a0b3febada2bceb4c037fff1718306a3a

[kenrb, 2017-02-23 17:00:41]

Description was changed from

==========
Discard compositor frames from unloaded web content

After a navigation commits, it is still possible for the compositor to
resend a frame that draws the previous (now unloaded) page. In some
situations this works against the browser process' new content
rendering timer that attempts to clear a page after 4 seconds if a
navigation has committed but not yet painted in that time.

This CL adds a counter called content_source_id to that increments on
every non-same-page navigation commit under a given top-level
RenderWidget. It tags the CompositorFrameMetadata with this identifier,
and also sends it to the browser process with the
DidCommitProvisionaLoad IPC message. This enables the browser to
recognize compositor frames with stale IDs, corresponding to unloaded
pages.

BUG=672847
==========

to

==========
Discard compositor frames from unloaded web content

After a navigation commits, it is still possible for the compositor to
resend a frame that draws the previous (now unloaded) page. In some
situations this works against the browser process' new content
rendering timer that attempts to clear a page after 4 seconds if a
navigation has committed but not yet painted in that time.

This CL adds a counter called content_source_id to that increments on
every non-same-page navigation commit under a given top-level
RenderWidget. It tags the CompositorFrameMetadata with this identifier,
and also sends it to the browser process with the
DidCommitProvisionaLoad IPC message. This enables the browser to
recognize compositor frames with stale IDs, corresponding to unloaded
pages.

BUG=672847
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[kenrb, 2017-02-23 17:00:42]

Description was changed from

==========
Discard compositor frames from unloaded web content

After a navigation commits, it is still possible for the compositor to
resend a frame that draws the previous (now unloaded) page. In some
situations this works against the browser process' new content
rendering timer that attempts to clear a page after 4 seconds if a
navigation has committed but not yet painted in that time.

This CL adds a counter called content_source_id to that increments on
every non-same-page navigation commit under a given top-level
RenderWidget. It tags the CompositorFrameMetadata with this identifier,
and also sends it to the browser process with the
DidCommitProvisionaLoad IPC message. This enables the browser to
recognize compositor frames with stale IDs, corresponding to unloaded
pages.

BUG=672847
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Discard compositor frames from unloaded web content

After a navigation commits, it is still possible for the compositor to
resend a frame that draws the previous (now unloaded) page. In some
situations this works against the browser process' new content
rendering timer that attempts to clear a page after 4 seconds if a
navigation has committed but not yet painted in that time.

This CL adds a counter called content_source_id to that increments on
every non-same-page navigation commit under a given top-level
RenderWidget. It tags the CompositorFrameMetadata with this identifier,
and also sends it to the browser process with the
DidCommitProvisionaLoad IPC message. This enables the browser to
recognize compositor frames with stale IDs, corresponding to unloaded
pages.

BUG=672847
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[kenrb, 2017-02-23 17:00:50]

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

[commit-bot: I haz the power, 2017-02-23 17:01:20]

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-23 17:14:04]

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

[commit-bot: I haz the power, 2017-02-23 17:14:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kenrb, 2017-02-23 18:44:51]

Description was changed from

==========
Discard compositor frames from unloaded web content

After a navigation commits, it is still possible for the compositor to
resend a frame that draws the previous (now unloaded) page. In some
situations this works against the browser process' new content
rendering timer that attempts to clear a page after 4 seconds if a
navigation has committed but not yet painted in that time.

This CL adds a counter called content_source_id to that increments on
every non-same-page navigation commit under a given top-level
RenderWidget. It tags the CompositorFrameMetadata with this identifier,
and also sends it to the browser process with the
DidCommitProvisionaLoad IPC message. This enables the browser to
recognize compositor frames with stale IDs, corresponding to unloaded
pages.

BUG=672847
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Discard compositor frames from unloaded web content

After a navigation commits, it is still possible for the compositor to
resend a frame that draws the previous (now unloaded) page. In some
situations this works against the browser process' new content
rendering timer that attempts to clear a page after 4 seconds if a
navigation has committed but not yet painted in that time.

This CL adds a counter called content_source_id to that increments on
every non-same-page navigation commit under a given top-level
RenderWidget. It tags the CompositorFrameMetadata with this identifier,
and also sends it to the browser process with the
DidCommitProvisionaLoad IPC message. This enables the browser to
recognize compositor frames with stale IDs, corresponding to unloaded
pages.

BUG=672847
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

[kenrb, 2017-02-23 18:44:55]

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

[commit-bot: I haz the power, 2017-02-23 18:45:17]

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-23 18:59:05]

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

[commit-bot: I haz the power, 2017-02-23 18:59:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kenrb, 2017-02-23 19:23:48]

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

[commit-bot: I haz the power, 2017-02-23 19:24:06]

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-23 19:36:09]

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

[commit-bot: I haz the power, 2017-02-23 19:36:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kenrb, 2017-02-23 19:38:36]

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

[commit-bot: I haz the power, 2017-02-23 19:39:06]

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

[kenrb, 2017-02-23 20:19:40]

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

[kenrb, 2017-02-23 20:19:41]

danakj: Can you please give this a first review, as a cc/ OWNER? This is more or
less as we discussed over chat a few days ago.

I've been trying to figure out a way to test this, but I'm not sure how to do
that other than maybe add superficial unit tests. The condition where it is
actually discarding compositor frames is difficult to simulate from the browser
process -- I don't really want to rely on any existing behavior that creates the
'page committed but not painting' state, since if that behavior goes away then
it would nullify the test.

[commit-bot: I haz the power, 2017-02-23 20:44:21]

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

[commit-bot: I haz the power, 2017-02-23 20:44:22]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kenrb, 2017-02-28 16:29:56]

kenrb@chromium.org changed reviewers:
+ enne@chromium.org
- danakj@chromium.org

[kenrb, 2017-02-28 16:29:57]

enne can you do this review instead? Let me know if you need any more context
than is in the description and the bug.

[enne (OOO), 2017-02-28 18:27:09]

On 2017/02/28 at 16:29:57, kenrb wrote:
> enne can you do this review instead? Let me know if you need any more context
than is in the description and the bug.

Sure, can do.  I can't see the bug, for what it's worth.

[enne (OOO), 2017-02-28 19:02:23]

This all makes a lot of sense.  I have a few small questions, but seems
reasonable overall.

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

https://codereview.chromium.org/2707243005/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1016: //
current_content_source_id_ should be non-decreasing, but it is hard to
Ignoring unsigned overflow here, how can raciness cause this to happen? It seems
like even with fast navigations, all of those did commit provisional loads will
happen in order.  Why not just always set current_source_id_ to next_source_id_
here?

https://codereview.chromium.org/2707243005/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1851: // Ignore this
frame if its content has already been unloaded.
I wonder if this early out needs to go after some of the things below.  In
particular, do the latency info, latency tracker, and reclaimed resources all
need to still happen?

Maybe this should be treated as if there's no view_ but otherwise similarly? 
I'm not 100% sure, but am just being skeptical about the placement of this
return.

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

https://codereview.chromium.org/2707243005/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:905: // TODO(kenrb,
fsamuel): We should use SurfaceIDs for this purpose when they
Yeah, having renderers know the surface id they're producing for makes a lot
more sense here.

[kenrb, 2017-02-28 22:18:44]

Thanks for the review!

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

https://codereview.chromium.org/2707243005/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1016: //
current_content_source_id_ should be non-decreasing, but it is hard to
On 2017/02/28 19:02:23, enne wrote:
> Ignoring unsigned overflow here, how can raciness cause this to happen? It
seems
> like even with fast navigations, all of those did commit provisional loads
will
> happen in order.  Why not just always set current_source_id_ to
next_source_id_
> here?

Done. You're right, I think I was being overly conservative, and it doesn't seem
like there is a way that these should get received out of order.

https://codereview.chromium.org/2707243005/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1851: // Ignore this
frame if its content has already been unloaded.
On 2017/02/28 19:02:23, enne wrote:
> I wonder if this early out needs to go after some of the things below.  In
> particular, do the latency info, latency tracker, and reclaimed resources all
> need to still happen?
> 
> Maybe this should be treated as if there's no view_ but otherwise similarly? 
> I'm not 100% sure, but am just being skeptical about the placement of this
> return.

I've moved it after the latency tracking, but for the rest I feel like it is
better to early abort. The renderer is still alive and ostensibly trying to
produce compositor frames for a new page, and this frame shouldn't have any
relevance. In the specific example I am trying to prevent, this compositor frame
has been sent once before, but is being sent again because the tab was hidden
and then re-shown. In that case we are waiting for a new frame, but this isn't
the one we want.

Of course, I can change it if there is a specific reason to continue to further
down the method.

[enne (OOO), 2017-02-28 22:23:21]

https://codereview.chromium.org/2707243005/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2707243005/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1855: // Ignore this
frame if its content has already been unloaded.
I agree that maybe the mobile optimized and double tap support for page
shouldn't be updated if the frame is going to be dropped.  However, I do think
that the resources from the frame should be returned so that the renderer can
reuse them.

[kenrb, 2017-03-01 15:32:43]

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

[kenrb, 2017-03-01 15:32:48]

https://codereview.chromium.org/2707243005/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2707243005/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1855: // Ignore this
frame if its content has already been unloaded.
On 2017/02/28 22:23:21, enne wrote:
> I agree that maybe the mobile optimized and double tap support for page
> shouldn't be updated if the frame is going to be dropped.  However, I do think
> that the resources from the frame should be returned so that the renderer can
> reuse them.

Done.

[commit-bot: I haz the power, 2017-03-01 15:32:56]

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-03-01 17:11:28]

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

[commit-bot: I haz the power, 2017-03-01 17:11:29]

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2017-03-01 17:49:00]

lgtm, thanks

[kenrb, 2017-03-01 18:02:02]

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

[kenrb, 2017-03-01 18:02:03]

creis@: PTAL content/?

[danakj, 2017-03-01 18:14:27]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2017-03-01 18:14:28]

LG to me also, but noticing a conspicuous lack of unit tests.

https://codereview.chromium.org/2707243005/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/2707243005/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.h:902: // drawn after a
navigation to a new page has already committed. This is
"This is be a"

[Charlie Reis, 2017-03-01 20:13:29]

+1 to the test request, but otherwise this looks generally good.  Can you
confirm my sanity checks below?  Thanks!

https://codereview.chromium.org/2707243005/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2707243005/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1861: if (view_ &&
frame.metadata.content_source_id >= current_content_source_id_) {
What if this came from an OOPIF?  Is it safe because both are guaranteed to be
0?  Let's document that.

https://codereview.chromium.org/2707243005/diff/100001/content/renderer/rende...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/2707243005/diff/100001/content/renderer/rende...
content/renderer/render_widget.h:854: // navigation commit by RenderFrameImpl.
At that time it is sent to the
Please clarify that this is only incremented and checked for main frames.

(I was concerned about how a possibly-invisible subframe commit within the same
widget might get us out of sync, but it doesn't look like subframes affect the
source ID at all.)

[kenrb, 2017-03-02 17:06:09]

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

[commit-bot: I haz the power, 2017-03-02 17:06:34]

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

[kenrb, 2017-03-02 17:08:18]

Thanks for the reviews. I have updated comments as requested and added two unit
tests: one in cc/ that checks the new field is propagated, and another in
content/browser/ that verifies old compositor frames are discarded. If you can
think of any other places where tests would be appropriate I would happily add
more (content/ is pretty sparse for unit tests in general, and this isn't really
suited for a browser test).

https://codereview.chromium.org/2707243005/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2707243005/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1861: if (view_ &&
frame.metadata.content_source_id >= current_content_source_id_) {
On 2017/03/01 20:13:29, Charlie Reis (slow) wrote:
> What if this came from an OOPIF?  Is it safe because both are guaranteed to be
> 0?  Let's document that.

Yes, and done.

https://codereview.chromium.org/2707243005/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/2707243005/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.h:902: // drawn after a
navigation to a new page has already committed. This is
On 2017/03/01 18:14:28, danakj wrote:
> "This is be a"

Done.

https://codereview.chromium.org/2707243005/diff/100001/content/renderer/rende...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/2707243005/diff/100001/content/renderer/rende...
content/renderer/render_widget.h:854: // navigation commit by RenderFrameImpl.
At that time it is sent to the
On 2017/03/01 20:13:29, Charlie Reis (slow) wrote:
> Please clarify that this is only incremented and checked for main frames.
> 
> (I was concerned about how a possibly-invisible subframe commit within the
same
> widget might get us out of sync, but it doesn't look like subframes affect the
> source ID at all.)

Correct, both in the renderer and browser we ignore this field for subframes. My
comment in render_widget_host_impl.h mentioned that, but I've added
clarification here also.

[danakj, 2017-03-02 18:12:50]

https://codereview.chromium.org/2707243005/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/2707243005/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_unittest.cc:1300:
frame.metadata.content_source_id = 100;
The code also accepts source id > 100. I've been wondering about it, and notice
that you don't test it here. Is that > actually important, if so it should be
tested. If not, maybe removed?

[Charlie Reis, 2017-03-02 18:32:13]

Thanks!  LGTM with danakj's suggestion in the test.

[commit-bot: I haz the power, 2017-03-02 18:36:41]

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

[commit-bot: I haz the power, 2017-03-02 18:36:43]

Dry run: This issue passed the CQ dry run.

[kenrb, 2017-03-02 19:35:57]

https://codereview.chromium.org/2707243005/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/2707243005/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_unittest.cc:1300:
frame.metadata.content_source_id = 100;
On 2017/03/02 18:12:50, danakj wrote:
> The code also accepts source id > 100. I've been wondering about it, and
notice
> that you don't test it here. Is that > actually important, if so it should be
> tested. If not, maybe removed?

That's a point, and I've added it to the test. The > is important because at
times the first compositor frame for a newly-loaded page can arrive at the
browser before the navigation commit message. It is racy because they are sent
by different threads. The comparison accounts for this case.

[danakj, 2017-03-02 19:56:34]

LGTM

https://codereview.chromium.org/2707243005/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/2707243005/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_unittest.cc:1300:
frame.metadata.content_source_id = 100;
On 2017/03/02 19:35:57, kenrb wrote:
> On 2017/03/02 18:12:50, danakj wrote:
> > The code also accepts source id > 100. I've been wondering about it, and
> notice
> > that you don't test it here. Is that > actually important, if so it should
be
> > tested. If not, maybe removed?
> 
> That's a point, and I've added it to the test. The > is important because at
> times the first compositor frame for a newly-loaded page can arrive at the
> browser before the navigation commit message. It is racy because they are sent
> by different threads. The comparison accounts for this case.

OK Thanks. A comment on the >= would be appreciated so this isn't lost in
codereview.

[danakj, 2017-03-02 19:57:20]

On 2017/03/02 19:56:34, danakj wrote:
> LGTM
> 
>
https://codereview.chromium.org/2707243005/diff/140001/content/browser/render...
> File content/browser/renderer_host/render_widget_host_unittest.cc (right):
> 
>
https://codereview.chromium.org/2707243005/diff/140001/content/browser/render...
> content/browser/renderer_host/render_widget_host_unittest.cc:1300:
> frame.metadata.content_source_id = 100;
> On 2017/03/02 19:35:57, kenrb wrote:
> > On 2017/03/02 18:12:50, danakj wrote:
> > > The code also accepts source id > 100. I've been wondering about it, and
> > notice
> > > that you don't test it here. Is that > actually important, if so it should
> be
> > > tested. If not, maybe removed?
> > 
> > That's a point, and I've added it to the test. The > is important because at
> > times the first compositor frame for a newly-loaded page can arrive at the
> > browser before the navigation commit message. It is racy because they are
sent
> > by different threads. The comparison accounts for this case.
> 
> OK Thanks. A comment on the >= would be appreciated so this isn't lost in
> codereview.

(I see u added one in the test which is very nice, but would like something in
the prod code too if u dont mind)

[kenrb, 2017-03-02 21:18:53]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2017-03-02 21:18:55]

The CQ bit was unchecked by kenrb@chromium.org

[kenrb, 2017-03-02 21:18:57]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
creis@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2707243005/#ps180001
(title: "Another comment tweak")

[commit-bot: I haz the power, 2017-03-02 21:19:24]

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

[kenrb, 2017-03-02 21:19:51]

Thanks for the reviews!

https://codereview.chromium.org/2707243005/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/2707243005/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_unittest.cc:1300:
frame.metadata.content_source_id = 100;
On 2017/03/02 19:56:34, danakj wrote:
> On 2017/03/02 19:35:57, kenrb wrote:
> > On 2017/03/02 18:12:50, danakj wrote:
> > > The code also accepts source id > 100. I've been wondering about it, and
> > notice
> > > that you don't test it here. Is that > actually important, if so it should
> be
> > > tested. If not, maybe removed?
> > 
> > That's a point, and I've added it to the test. The > is important because at
> > times the first compositor frame for a newly-loaded page can arrive at the
> > browser before the navigation commit message. It is racy because they are
sent
> > by different threads. The comparison accounts for this case.
> 
> OK Thanks. A comment on the >= would be appreciated so this isn't lost in
> codereview.

Done.

[commit-bot: I haz the power, 2017-03-02 23:33:13]

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

[commit-bot: I haz the power, 2017-03-02 23:33:15]

Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[Charlie Reis, 2017-03-02 23:37:09]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2017-03-02 23:37:40]

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-03-03 02:11:11]

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

[commit-bot: I haz the power, 2017-03-03 02:11:14]

Failed to apply patch for cc/ipc/cc_param_traits_macros.h:
While running git apply --index -p1;
  error: patch failed: cc/ipc/cc_param_traits_macros.h:196
  error: cc/ipc/cc_param_traits_macros.h: patch does not apply

Patch:       cc/ipc/cc_param_traits_macros.h
Index: cc/ipc/cc_param_traits_macros.h
diff --git a/cc/ipc/cc_param_traits_macros.h b/cc/ipc/cc_param_traits_macros.h
index
7df6c30758e096595eea91137a05f570f88ed00f..5c982b42202824667ffb85d1c80e08f39f889888
100644
--- a/cc/ipc/cc_param_traits_macros.h
+++ b/cc/ipc/cc_param_traits_macros.h
@@ -196,6 +196,7 @@ IPC_STRUCT_TRAITS_BEGIN(cc::CompositorFrameMetadata)
   IPC_STRUCT_TRAITS_MEMBER(selection)
   IPC_STRUCT_TRAITS_MEMBER(latency_info)
   IPC_STRUCT_TRAITS_MEMBER(referenced_surfaces)
+  IPC_STRUCT_TRAITS_MEMBER(content_source_id)
 IPC_STRUCT_TRAITS_END()
 
 IPC_STRUCT_TRAITS_BEGIN(cc::SurfaceInfo)

[kenrb, 2017-03-03 13:53:36]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2017-03-03 13:53:36]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
creis@chromium.org, enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2707243005/#ps200001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-03 13:53:47]

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-03-03 14:44:17]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1488549216439930,
"parent_rev": "45bc57b54c6a1e20ed41254327a06de44ea7bb1d", "commit_rev":
"efd1189a0b3febada2bceb4c037fff1718306a3a"}

[commit-bot: I haz the power, 2017-03-03 14:44:56]

Description was changed from

==========
Discard compositor frames from unloaded web content

After a navigation commits, it is still possible for the compositor to
resend a frame that draws the previous (now unloaded) page. In some
situations this works against the browser process' new content
rendering timer that attempts to clear a page after 4 seconds if a
navigation has committed but not yet painted in that time.

This CL adds a counter called content_source_id to that increments on
every non-same-page navigation commit under a given top-level
RenderWidget. It tags the CompositorFrameMetadata with this identifier,
and also sends it to the browser process with the
DidCommitProvisionaLoad IPC message. This enables the browser to
recognize compositor frames with stale IDs, corresponding to unloaded
pages.

BUG=672847
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Discard compositor frames from unloaded web content

After a navigation commits, it is still possible for the compositor to
resend a frame that draws the previous (now unloaded) page. In some
situations this works against the browser process' new content
rendering timer that attempts to clear a page after 4 seconds if a
navigation has committed but not yet painted in that time.

This CL adds a counter called content_source_id to that increments on
every non-same-page navigation commit under a given top-level
RenderWidget. It tags the CompositorFrameMetadata with this identifier,
and also sends it to the browser process with the
DidCommitProvisionaLoad IPC message. This enables the browser to
recognize compositor frames with stale IDs, corresponding to unloaded
pages.

BUG=672847
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2707243005
Cr-Commit-Position: refs/heads/master@{#454578}
Committed:
https://chromium.googlesource.com/chromium/src/+/efd1189a0b3febada2bceb4c037f...
==========

[commit-bot: I haz the power, 2017-03-03 14:44:58]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/efd1189a0b3febada2bceb4c037f...

[sclittle, 2017-03-04 00:54:48]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/2730203002/ by sclittle@chromium.org.

The reason for reverting is: It looks like this broke the MSAN bot:

https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu...

https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....

[kenrb, 2017-03-06 16:08:20]

Description was changed from

==========
Discard compositor frames from unloaded web content

After a navigation commits, it is still possible for the compositor to
resend a frame that draws the previous (now unloaded) page. In some
situations this works against the browser process' new content
rendering timer that attempts to clear a page after 4 seconds if a
navigation has committed but not yet painted in that time.

This CL adds a counter called content_source_id to that increments on
every non-same-page navigation commit under a given top-level
RenderWidget. It tags the CompositorFrameMetadata with this identifier,
and also sends it to the browser process with the
DidCommitProvisionaLoad IPC message. This enables the browser to
recognize compositor frames with stale IDs, corresponding to unloaded
pages.

BUG=672847
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2707243005
Cr-Commit-Position: refs/heads/master@{#454578}
Committed:
https://chromium.googlesource.com/chromium/src/+/efd1189a0b3febada2bceb4c037f...
==========

to

==========
Discard compositor frames from unloaded web content

After a navigation commits, it is still possible for the compositor to
resend a frame that draws the previous (now unloaded) page. In some
situations this works against the browser process' new content
rendering timer that attempts to clear a page after 4 seconds if a
navigation has committed but not yet painted in that time.

This CL adds a counter called content_source_id to that increments on
every non-same-page navigation commit under a given top-level
RenderWidget. It tags the CompositorFrameMetadata with this identifier,
and also sends it to the browser process with the
DidCommitProvisionaLoad IPC message. This enables the browser to
recognize compositor frames with stale IDs, corresponding to unloaded
pages.

BUG=672847
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2707243005
Cr-Commit-Position: refs/heads/master@{#454578}
Committed:
https://chromium.googlesource.com/chromium/src/+/efd1189a0b3febada2bceb4c037f...
==========