Check for BeginFrame time/seqnum continuity in ExternalBFS::OnBF.

cc/scheduler/begin_frame_source.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/scheduler/begin_frame_source.h"
 
 #include <stddef.h>
 
 #include "base/atomic_sequence_num.h"
 #include "base/auto_reset.h"
@@ skipping 280 matching lines @@
 
 void ExternalBeginFrameSource::OnSetBeginFrameSourcePaused(bool paused) {
   if (paused_ == paused)
     return;
   paused_ = paused;
   std::unordered_set<BeginFrameObserver*> observers(observers_);
   for (auto* obs : observers)
     obs->OnBeginFrameSourcePausedChanged(paused_);
 }
 
 void ExternalBeginFrameSource::OnBeginFrame(const BeginFrameArgs& args) {

[brianderson, 2017/02/17 20:29:09]

I didn't catch this in the source_id patch you landed earlier, but I wonder if
the ExternalBeginFrameSource should override the source_id / sequence number of
the args received here with it's own source_id / sequence number.

[Eric Seckler, 2017/02/17 22:38:39]

I don't think that makes sense. BeginFrame creators, not forwarders, assign
BeginFrame identifiers (source_id+seqnum). ExternalBFS is normally used as a
"middle piece" that forwards BeginFrames across process boundaries, e.g. browser
to renderer. I think that we want to be able to track the same BeginFrame
identifier in the renderer (behind the ExternalBFS) so that we can "report back"
to the browser (original BFS) when everyone in the renderer has finished a
particular BeginFrame.

[brianderson, 2017/02/17 22:47:19]

My thinking was that it's a little weird for this class to have a source_id that
isn't used. Also, mapping the browser's source_id to this classes source_id
would solve the source_id cross-process aliasing (non-)issue. In any case,
there's no benefit to do the mapping now and it would only add complexity, so
I'll drop the idea.

   missed_begin_frame_args_ = args;
   missed_begin_frame_args_.type = BeginFrameArgs::MISSED;
   std::unordered_set<BeginFrameObserver*> observers(observers_);
-  for (auto* obs : observers)
-    obs->OnBeginFrame(args);
+  for (auto* obs : observers) {
+    // It is possible that the source in which |args| originate changes, or that
+    // our hookup to this source changes, so we have to check for continuity.
+    // See also https://crbug.com/690127 for what may happen without this check.
+    BeginFrameArgs last_args = obs->LastUsedBeginFrameArgs();

[brianderson, 2017/02/17 20:29:09]

const ref

[Eric Seckler, 2017/02/17 22:38:39]

done, also above.

+    if (!last_args.IsValid() || (args.frame_time > last_args.frame_time)) {
+      DCHECK((args.source_id != last_args.source_id) ||
+             (args.sequence_number > last_args.sequence_number))
+          << "current " << args.AsValue()->ToString() << ", last "
+          << last_args.AsValue()->ToString();
+      obs->OnBeginFrame(args);
+    }
+  }
 }
 
 }  // namespace cc