Fix race conditions in window snapshot code.

Fix race conditions in window snapshot code.

Use LatencyInfo to track when a rendered frame reaches the screen.
Only tested so far on Linux with single-threaded compositor. Must be
tested with Telemetry's GpuTabTest.testScreenshot test on all of
Windows, Mac and Linux with the compositor disabled, enabled and
single-threaded, and enabled and multi-threaded.

BUG=256848
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228652

[Ken Russell (switch to Gerrit), 2013-09-10 02:55:00]

Brandon/Mo, hoping you may be able to take over this work while I'm traveling
this week.

Enne, Nat, John, will appreciate your feedback on the direction taken so far.

[Zhenyao Mo, 2013-09-11 17:08:05]

Enne, Nat, John, please take a look and provide some feedback before I move
forward with ken's CL.

[enne (OOO), 2013-09-11 17:42:42]

https://codereview.chromium.org/23694031/diff/1/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/23694031/diff/1/cc/trees/thread_proxy.cc#newc...
cc/trees/thread_proxy.cc:517: void ThreadProxy::SetNextCommitForcesRedraw() {
This isn't going to work.  This function is "SetNextDrawForcesRedraw", but it
could force the previous source frame (or the one previous to that!) to draw and
not the source frame that comes with the next commit.

The right way to do this is to set a boolean on LayerTreeHost in
LayerTreeHost::SetNextCommitForcesRedraw and then do some work in
LayerTreeHost::FinishCommitOnImplThread.

For the non-impl-side painting path, you can just set full layer damage in that
function.

For the impl-side painting path, you'll need to set some new boolean on
LayerTreeImpl.  Then, in LayerTreeImpl::PushPropertiesTo (aka activation) if
that boolean is true, you can set full root layer damage there on
LayerTreeHostImpl.

That will properly synchronize the damage with the commit that you are forcing.

[jbauman, 2013-09-13 22:33:24]

The latency info stuff mostly looks good, but just a couple of comments.

https://codereview.chromium.org/23694031/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/23694031/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_impl.cc:2521: const
CommandLine& command_line = *CommandLine::ForCurrentProcess();
Can multiple snapshots be outstanding at once? If so, they could be coalesced
together into one swap, which means the snapshot with the lower number will be
ignored.

https://codereview.chromium.org/23694031/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_impl.cc:2559:
RenderWidgetHostImpl::From(rwh)->FrameSwapped(latency_info);
You should probably make a set of RWHI to send the message to, or otherwise an
RWHI could get the same message twice, which could cause problems.

[bajones, 2013-09-19 23:45:44]

@enne, @jbauman: I've picked this code up from Ken and addressed your feedback.
Please take another look and make sure I understood you correctly. :)

[bajones, 2013-09-20 23:34:45]

@enne, @jbauman: Review ping?

[Ken Russell (switch to Gerrit), 2013-09-21 02:15:38]

This LGTM, but what kind of testing has been done? It needs to be tested on all
of Windows, Mac and Linux with the legacy software path, single-threaded
compositing, and threaded compositing. Or, at least, the legacy software path
plus whatever the default compositor path is on each platform.

It would be ideal if you would add at least one more stress test of the window
snapshot code, like the one you mentioned of generating random colors and making
sure they come through. That test could either fill a 2D canvas or change the
background color of a simple div.

Also please update the CL with a revised description.

[jbauman, 2013-09-21 02:18:13]

On 2013/09/20 23:34:45, bajones wrote:
> @enne, @jbauman: Review ping?

I can't see any response about the coalescing of multiple snapshot_ids into one
swap, so the lower id snapshot won't happen. Was that addressed anywhere?

[enne (OOO), 2013-09-24 17:22:25]

cc/ ⓛⓖⓣⓜ as soon as you add a layer_tree_host_unittest for this new conditional

https://codereview.chromium.org/23694031/diff/8001/content/renderer/gpu/rende...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/23694031/diff/8001/content/renderer/gpu/rende...
content/renderer/gpu/render_widget_compositor.cc:382: if (threaded_) {
Maybe just call RenderWidgetCompositor::setNeedsRedraw rather than replicating
this conditional?

[enne (OOO), 2013-09-24 17:23:26]

Er, s/new conditional/new flag/

[bajones, 2013-09-26 22:27:22]

Apologies for the delay getting this updated!

https://codereview.chromium.org/23694031/diff/21001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/23694031/diff/21001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest.cc:639: class
LayerTreeHostTestSetNextCommitForcesRedraw : public LayerTreeHostTest {
@enne: Not being overly familiar with these tests please let me know if you feel
this could be improved or if you don't feel it provides the right coverage.

https://codereview.chromium.org/23694031/diff/21001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/23694031/diff/21001/content/renderer/render_v...
content/renderer/render_view_impl.cc:2256: 
@jbauman: Sorry I missed this on my last pass. This code scans through all
pending snapshot requests and fires of the completed callback for any with an id
<= the completed snapshot. This should prevent any requests from becoming
orphaned by sharing a swap. Please let me know if you have concerns about this
approach!

https://codereview.chromium.org/23694031/diff/21001/content/test/data/screens...
File content/test/data/screenshot_sync.html (right):

https://codereview.chromium.org/23694031/diff/21001/content/test/data/screens...
content/test/data/screenshot_sync.html:1: <!DOCTYPE html>
This test spins through random color clears on a canvas, performing screenshots
at arbitrary intervals and comparing the results it receives with the last
cleared color.

The test works well standalone, but I still need to do a little bit of work to
launch it from telemetry and capture the results.

[jbauman, 2013-09-27 00:39:21]

lgtm

[enne (OOO), 2013-09-27 19:59:07]

https://codereview.chromium.org/23694031/diff/21001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/23694031/diff/21001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest.cc:670: virtual void
DrawLayersOnThread(LayerTreeHostImpl* host_impl) OVERRIDE {
This test is good, but I'd like to make more sure it's testing the right thing.

Right now you have:

commit -> activate -> draw -> PostSetNextCommitForcesRedraw ->
PostSetNeedsCommit -> commit -> activate -> draw with full damage -> end test

commit -> activate -> draw -> PostSetNextCommitForcesRedraw -> *set needs
redraw* -> *draw with no damage* -> PostSetNeedsCommitToMainThread -> commit ->
*set needs redraw* -> *draw with no damage* -> activate -> draw with full damage
-> end test

Can you insert two draws into the pipeline to make sure that the forced needs
redraw is associated with the right frame? Otherwise, it could be the case that
the "next commit forces full frame damage" isn't actually applied to the right
frame and just applies to the next draw, which only coincidentally is the right
frame.

[bajones, 2013-09-27 20:50:10]

https://codereview.chromium.org/23694031/diff/29001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/23694031/diff/29001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest.cc:640: public:
@enne: Is this what you had in mind?

https://codereview.chromium.org/23694031/diff/29001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/tab_unittest.py (right):

https://codereview.chromium.org/23694031/diff/29001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/tab_unittest.py:77: def testScreenshotSync(self):
Screenshot sync test has been added to telemetry's unittests. I was going to add
it to content/test/gpu_tests but then it's a separate telemetry test type that
will need to be added to the recipes. This felt more natural in the end. (cc/
kbr@)

[Ken Russell (switch to Gerrit), 2013-09-27 22:42:38]

LGTM

https://codereview.chromium.org/23694031/diff/29001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/tab_unittest.py (right):

https://codereview.chromium.org/23694031/diff/29001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/tab_unittest.py:77: def testScreenshotSync(self):
On 2013/09/27 20:50:11, bajones wrote:
> Screenshot sync test has been added to telemetry's unittests. I was going to
add
> it to content/test/gpu_tests but then it's a separate telemetry test type that
> will need to be added to the recipes. This felt more natural in the end. (cc/
> kbr@)

In order to provide complete coverage, the test will have to be run (for the
time being) with both the legacy software path and the compositor. It's fine in
my opinion to have it as part of telemetry's unit tests, but this will only be
testing one of these two configurations. Eventually we should invoke it as part
of the GPU bots' tests in both configurations.

[bajones, 2013-10-01 18:43:00]

enne@: ping?

cevans@: Please review changes in content/common

jamesr@: Please review changes in content/renderer

ben@: Please review changes in ui/events

[enne (OOO), 2013-10-01 19:15:29]

https://codereview.chromium.org/23694031/diff/29001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/23694031/diff/29001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest.cc:677: // Cycle through a couple of empty
commits to ensure we're observing the
Hrm.  I was hoping you could add a commit -> draw -> activate -> draw test to
verify that the damage was attached to the right frame and didn't just happen on
the first draw after a commit, but this change here just repeats the commit ->
activate -> draw flow.

You can insert such a draw by something like:

On num_draws_ == 2 in DrawLayers, keep posting what's already there and if
you're in impl-side painting mode BlockNotifyReadyToActivateForTesting(true). 
If you're not in impl-side painting mode, then just increment num_draws_ again
to skip this next case.

Override CommitCompleteOnThread and only when num_draws_ == 2, set some fake
damage rect that's non-full and set needs redraw.

On num_draws_ == 3 in DrawLayers, verify it's the fake damage rect and
BlockNotifyReadyToActivateForTesting(false).

This will let the tree activate and you'll get num_draws_ == 4 with a full
damage rect.

[Ben Goodger (Google), 2013-10-01 23:15:23]

ui/events lgtm

[jamesr, 2013-10-01 23:42:13]

It looks like something's very amiss with the data flow required to use
ui::LatencyInfo.  The IPC channel that a RenderProcess / RenderProcessHost use
to communicate with each other allows them to unique identify each other.  It
looks like you're trying to layer a redundant identification system on top of
this IPC channel and I can't figure out why.

https://codereview.chromium.org/23694031/diff/29001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/23694031/diff/29001/content/renderer/render_v...
content/renderer/render_view_impl.cc:2744: // Note: this must match the logic in
RenderWidgetHostImpl.
so if I understand correctly, what's going on here is:

1.) The browser process sends its ID for the render process over IPC to the
renderer
2.) The render process then stuffs this number into a ui::LatencyInfo event and
sends it back to the browser
3.) The browser process, in RenderWidgetHostImpl (which obviously knows which
render process it is talking to) then extracts information back out of the
ui::LatencyInfo struct by using the PID it already sent down

This seems a bit silly.  In RenderWidgetHostImpl when you are processing this
event, you know which render process you're talking to so why do you need this
extra plumbing?

[jbauman, 2013-10-01 23:49:27]

On 2013/10/01 23:42:13, jamesr wrote:
> It looks like something's very amiss with the data flow required to use
> ui::LatencyInfo.  The IPC channel that a RenderProcess / RenderProcessHost use
> to communicate with each other allows them to unique identify each other.  It
> looks like you're trying to layer a redundant identification system on top of
> this IPC channel and I can't figure out why.
> 
>
https://codereview.chromium.org/23694031/diff/29001/content/renderer/render_v...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/23694031/diff/29001/content/renderer/render_v...
> content/renderer/render_view_impl.cc:2744: // Note: this must match the logic
in
> RenderWidgetHostImpl.
> so if I understand correctly, what's going on here is:
> 
> 1.) The browser process sends its ID for the render process over IPC to the
> renderer
> 2.) The render process then stuffs this number into a ui::LatencyInfo event
and
> sends it back to the browser
> 3.) The browser process, in RenderWidgetHostImpl (which obviously knows which
> render process it is talking to) then extracts information back out of the
> ui::LatencyInfo struct by using the PID it already sent down
> 
> This seems a bit silly.  In RenderWidgetHostImpl when you are processing this
> event, you know which render process you're talking to so why do you need this
> extra plumbing?

The ui::LatencyInfo normally goes through the compositor, possibly through the
GPU process, through the browser compositor, through the GPU process again, and
into the browser, so it's not quite as straightforward as that.

Although I think you're right that we could patch the number in as soon as it
comes up from the renderer, in the various places where it does that (RWHVAura,
RWHI, GpuProcessHost, etc.). But I'm not sure that's more straightforward or
maintainable.

[jamesr, 2013-10-01 23:54:51]

Somehow this message is getting from the RenderWidget to the
RenderWidgetHostImpl, so we're definitely routing it correctly without this
additional information.

[jbauman, 2013-10-01 23:57:56]

On 2013/10/01 23:54:51, jamesr wrote:
> Somehow this message is getting from the RenderWidget to the
> RenderWidgetHostImpl, so we're definitely routing it correctly without this
> additional information.

We're routing it based on the information that was just added. See
RenderWidgetHostImpl::CompositorFrameDrawn.

[jamesr, 2013-10-02 00:06:40]

https://codereview.chromium.org/23694031/diff/29001/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.h (right):

https://codereview.chromium.org/23694031/diff/29001/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.h:58: // any early-outs.
is this the same as SetNeedsRedrawRect(...)? i.e., force window damage?  If it's
not, could you explain in the comment how it's different?  This function should
be next to that in the header since the two are closely related

https://codereview.chromium.org/23694031/diff/29001/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/23694031/diff/29001/content/renderer/render_w...
content/renderer/render_widget.cc:378: if (compositor_ && force_redraw) {
why don't you move this code into ScheduleCompositeWithForcedRedraw()?  Having
it in this function but only apply to one callsite is just confusing

[bajones, 2013-10-03 22:10:33]

Updated. Thanks @enne for the help with the unit tests!

[enne (OOO), 2013-10-03 23:18:38]

cc/ lgtm.  Thanks for making the unit test be more targeted.

[bajones, 2013-10-04 21:43:49]

nduca@ or dtu@: Quick review of the Telemetry changes, please?

cevans@, cdn@, jn@: Need a security review for common/view_messages.h. Pretty
minor change.

[dtu, 2013-10-04 23:57:41]

telemetry lgtm

[jln (very slow on Chromium), 2013-10-07 18:25:12]

content/common/view_messages.h lgtm with mandatory nit.

[jln (very slow on Chromium), 2013-10-07 18:25:41]

Err, forgot to send the nit in question

https://codereview.chromium.org/23694031/diff/57001/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/23694031/diff/57001/content/common/view_messa...
content/common/view_messages.h:812: int64 /* process_id */)
Please, use base::ProcessId

[commit-bot: I haz the power, 2013-10-08 23:24:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/23694031/67001

[commit-bot: I haz the power, 2013-10-09 02:06:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/23694031/67001

[commit-bot: I haz the power, 2013-10-09 05:34:50]

Retried try job too often on win_rel for step(s) telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-10-10 22:39:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/23694031/95001

[commit-bot: I haz the power, 2013-10-11 01:47:11]

Retried try job too often on win_rel for step(s) telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[bajones, 2013-10-14 23:42:33]

@piman, @danakj: Added some changes to SoftwareBrowserCompositorOutputSurface in
the latest revision. I'd appreciate your input.

We found that the software composting path was calling CompositorFrameDrawn,
which in turn takes the snapshot, synchronously while still in the
Compositor::Draw callstack. This was DCHECKing on Compositor::ReadPixels when a
new PendingSwap was created before the one in Compositor::Draw went out of
scope.

This change, which just moves the CompositorFrameDrawn call into a PostTask,
prevents that conflict and also brings the software compositor path a bit more
inline with the GPU path, which already calls this asynchronously from the GPU
process.

If you have any concerns about this approach please let us know!

[piman, 2013-10-15 00:12:13]

Seems reasonable, given that if this was on the compositor thread, we'd have to
post a task to the main thread, but I'll let jbauman double-check since he wrote
this code.

(at a high level, I'm not fond of abusing LatencyInfo as a replacement for IPCs.
The routing is based on a hack, and it's lacking fundamental safeties, e.g. the
renderer could fake its ID. Also adding active semantic to latency components
seems really wrong.)

https://codereview.chromium.org/23694031/diff/124001/content/browser/aura/sof...
File content/browser/aura/software_browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/23694031/diff/124001/content/browser/aura/sof...
content/browser/aura/software_browser_compositor_output_surface.cc:30:
latency_info));
nit: base::Bind(&RenderWidgetHostImpl::CompositorFrameDrawn, latency_info)

Simpler.

[jbauman, 2013-10-15 00:19:28]

On 2013/10/15 00:12:13, piman wrote:
> Seems reasonable, given that if this was on the compositor thread, we'd have
to
> post a task to the main thread, but I'll let jbauman double-check since he
wrote
> this code.

lgtm, sure.

> 
> (at a high level, I'm not fond of abusing LatencyInfo as a replacement for
IPCs.
> The routing is based on a hack, and it's lacking fundamental safeties, e.g.
the
> renderer could fake its ID. Also adding active semantic to latency components
> seems really wrong.)
> 
>
https://codereview.chromium.org/23694031/diff/124001/content/browser/aura/sof...
> File content/browser/aura/software_browser_compositor_output_surface.cc
(right):
> 
>
https://codereview.chromium.org/23694031/diff/124001/content/browser/aura/sof...
> content/browser/aura/software_browser_compositor_output_surface.cc:30:
> latency_info));
> nit: base::Bind(&RenderWidgetHostImpl::CompositorFrameDrawn, latency_info)
> 
> Simpler.

[commit-bot: I haz the power, 2013-10-15 06:01:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/23694031/144001

[commit-bot: I haz the power, 2013-10-15 08:24:58]

Change committed as 228652

[jam, 2014-01-09 23:42:09]

I just saw this change.

There's a big issue here: we have by design never sent the renderer its process
id (not the pid, but the child_process_id). This change was never reviewed by a
content/renderer owner. Ben is a top level owner, but he was asked only to
review ui/events.

We need to remove this asap from the renderer because I don't want any other
code using it (or this code at all).

Can we revert this change until this is fixed?

[bajones, 2014-01-10 00:11:37]

On 2014/01/09 23:42:09, jam wrote:
> I just saw this change.
> 
> There's a big issue here: we have by design never sent the renderer its
process
> id (not the pid, but the child_process_id). This change was never reviewed by
a
> content/renderer owner. Ben is a top level owner, but he was asked only to
> review ui/events.
> 
> We need to remove this asap from the renderer because I don't want any other
> code using it (or this code at all).
> 
> Can we revert this change until this is fixed?

Seeing as how the code has been in for > 2 months now I suspect reverting won't
be simple or clean, but I understand the concern.

enne@, @danakj: You've been doing some work in this space recently, is their any
chance that your changes have removed the need for the process id?

[enne (OOO), 2014-01-10 00:14:40]

On 2014/01/10 00:11:37, bajones wrote:

> enne@, @danakj: You've been doing some work in this space recently, is their
any
> chance that your changes have removed the need for the process id?

Nothing that I've been doing has affected any of the IPCs.  I just have been
touching the compositor readback code, sorry.

[bajones, 2014-01-10 00:16:02]

On 2014/01/10 00:14:40, enne wrote:
> On 2014/01/10 00:11:37, bajones wrote:
> 
> > enne@, @danakj: You've been doing some work in this space recently, is their
> any
> > chance that your changes have removed the need for the process id?
> 
> Nothing that I've been doing has affected any of the IPCs.  I just have been
> touching the compositor readback code, sorry.

No worries, just figured there might be a chance that the universe had corrected
itself naturally :)

Let me work with kbr@ to figure out the best path forward.

[Ken Russell (switch to Gerrit), 2014-01-10 00:20:16]

On 2014/01/09 23:42:09, jam wrote:
> I just saw this change.
> 
> There's a big issue here: we have by design never sent the renderer its
process
> id (not the pid, but the child_process_id). This change was never reviewed by
a
> content/renderer owner. Ben is a top level owner, but he was asked only to
> review ui/events.
> 
> We need to remove this asap from the renderer because I don't want any other
> code using it (or this code at all).
> 
> Can we revert this change until this is fixed?

This change was cleared with the security team at the time.

No, I am sorry, but this change can not simply be reverted. There are key
correctness tests running on the GPU bots which use this code.

Let's talk offline to discuss a path forward.

[jam, 2014-01-10 00:24:27]

On 2014/01/10 00:20:16, Ken Russell wrote:
> On 2014/01/09 23:42:09, jam wrote:
> > I just saw this change.
> > 
> > There's a big issue here: we have by design never sent the renderer its
> process
> > id (not the pid, but the child_process_id). This change was never reviewed
by
> a
> > content/renderer owner. Ben is a top level owner, but he was asked only to
> > review ui/events.
> > 
> > We need to remove this asap from the renderer because I don't want any other
> > code using it (or this code at all).
> > 
> > Can we revert this change until this is fixed?
> 
> This change was cleared with the security team at the time.

I see. This code wasn't reviewed by a content/renderer owner though, who could
have raised this issue.

We have explicitly avoided sending this id to the renderer so that it does not
send it back to the browser, as any checks based on that would be untrusted.
It's a major decision to revisit this, and that's what an owner of that code
would have said.

> 
> No, I am sorry, but this change can not simply be reverted. There are key
> correctness tests running on the GPU bots which use this code.
> 
> Let's talk offline to discuss a path forward.

I'll be in the office tomorrow, let's chat then. We really need this code to
stop using this ID on the renderer side. I'm not familiar at all with this code,
so can you look into ways of avoiding this in the meantime.

[jamesr, 2014-01-10 00:37:59]

This is all vague stuff, I don't have a good concrete suggestion for this patch.

My recollection of the issue here is that the code wants a way to receive a
notification in the browser process when a specific renderer's frame is
presented.  The frame is sent to the GPU process, where it's associated with a
surface, and then a notification is sent to the browser process.  In the browser
process, depending on which graphics configuration we're in, there may or may
not be a clear mapping from data on the notification (like the surface id) back
to the original renderer that sent the frame.

To get around this, this patch sticks the renderer's PID into a ui::LatencyInfo
object - which is effectively a hashmap of strings to ints - which is carried
along the frame data back to the browser process.  Then in the browser process a
static function parses out the PID and maps that back to the render host.

IMO, while there are many channels and indirections involved here between
renderer->gpu->browser process we are already plumbing enough data through to
correctly identify which renderer a given frame is for - since we obviously
present it in the correct place - and should be able to correlate this
notification as well with the existing data.  I think it's worrying when we
decide to tack additional data that's technically redundant with existing
mechanisms.  This leads really easily to an explosion where every subsystem
throws extra data in that's only useful for that subsystem because the things
the other subsystems do is too complicated to reuse.