sync: Buffer Protocol Events for about:sync page

chrome/browser/resources/sync_internals/about.js

 // Copyright (c) 2012 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.
 
 cr.define('chrome.sync.about_tab', function() {
   // Contains the latest snapshot of sync about info.
   chrome.sync.aboutInfo = {};
 
   function refreshAboutInfo(aboutInfo) {
     chrome.sync.aboutInfo = aboutInfo;
     var aboutInfoDiv = $('about-info');
     jstProcess(new JsEvalContext(aboutInfo), aboutInfoDiv);
   }
 
   function onAboutInfoUpdatedEvent(e) {
     refreshAboutInfo(e.details);
   }
 
   /** Container for accumulated sync protocol events. */
   var protocolEvents = [];
 
+  /** We may receive re-delivered events.  Keep a record of ones we've seen. */

[James Hawkins, 2014/03/27 21:01:25]

Can we not fix the issue of re-delivered events?

[rlarocque, 2014/03/27 21:19:54]

It gets ugly no matter how we handle it.

Option #1: Ignore it.  We send any buffered events to all listeners whenever a
new listener registers.  This will be a problem only when someone opens
about:sync twice.

Option #2: Send the buffered only events only to the newly registered listener. 
The problem is that the observation crosses a thread boundary.  The registration
path is

SyncInternalsMessageHandler
  -> ProfileSyncService (observer list is managed here)
  -> SyncBackendHost
   ... across a thread boundary ...
  -> SyncBackendHostCore
  -> SyncManagerImpl (which owns the buffer)

By the time the SyncBackendHostCore returns its response,
 1. the user could have closed the page, thus deleting the observer.
 2. (in theory) the observer could have unregistered itself.

We could handle (1) with a WeakPtr and a Closure.  I think we could handle (2)
if we had some sort of WeakPtr observer interface that got invalidated when the
ProfileSyncService was asked to remove the observer from its list.

This is starting to sound like some kind of generic cross-thread observer list
interface.  I'm pretty sure this is the right solution, but I'm not sure it's
worth building and maintaining just for this use case.

Option #3: The hack you see here.  Make the listeners figure it out.

That's the thinking that led me to this CL.  If you can think of a better
approach, I'd be happy to implement it.

+  var knownEventTimestamps = {};
+
   /**
    * Callback for incoming protocol events.
    * @param {Event} e The protocol event.
    */
   function onReceivedProtocolEvent(e) {
     var details = e.details;
+
+    // Return early if we've seen this event before.  Assumes that timestamps
+    // are sufficiently high resolution to uniquely identify an event.
+    if (knownEventTimestamps.hasOwnProperty(details.time)) {
+      return;
+    }
+
+    knownEventTimestamps[details.time] = true;
     protocolEvents.push(details);
 
     var context = new JsEvalContext({ events: protocolEvents });
     jstProcess(context, $('traffic-event-container'));
   }
 
   /**
    * Initializes state and callbacks for the protocol event log UI.
    */
   function initProtocolEventLog() {
     chrome.sync.events.addEventListener(
         'onProtocolEvent', onReceivedProtocolEvent);
 
     // Make the prototype jscontent element disappear.
     jstProcess({}, $('traffic-event-container'));
   }
 
   /**
-   * Toggles the given traffic event entry div's "expanded" state.
-   * @param {HTMLElement} element the element to toggle.
+   * Initializes listeners for status dump and import UI.
    */
-  function expandListener(element) {
-    element.target.classList.toggle('traffic-event-entry-expanded');
-  }
-
-  /**
-   * Attaches a listener to the given traffic event entry div.
-   * @param {HTMLElement} element the element to attach the listener to.
-   */
-  function addExpandListener(element) {
-    element.addEventListener('click', expandListener, false);
-  }
-
-  function onLoad() {
+  function initStatusDumpButton() {
     $('status-data').hidden = true;
 
-    chrome.sync.events.addEventListener(
-        'onAboutInfoUpdated',
-        onAboutInfoUpdatedEvent);
-    chrome.sync.requestUpdatedAboutInfo();
-
     var dumpStatusButton = $('dump-status');
     dumpStatusButton.addEventListener('click', function(event) {
       var aboutInfo = chrome.sync.aboutInfo;
       if (!$('include-ids').checked) {
         aboutInfo.details = chrome.sync.aboutInfo.details.filter(function(el) {
           return !el.is_sensitive;
         });
       }
       var data = '';
       data += new Date().toString() + '\n';
@@ skipping 25 matching lines @@
       data = data.substr(firstBrace);
 
       // Remove listeners to prevent sync events from overwriting imported data.
       chrome.sync.events.removeEventListener(
           'onAboutInfoUpdated',
           onAboutInfoUpdatedEvent);
 
       var aboutInfo = JSON.parse(data);
       refreshAboutInfo(aboutInfo);
     });
+  }
 
+  /**
+   * Toggles the given traffic event entry div's "expanded" state.
+   * @param {HTMLElement} element the element to toggle.
+   */
+  function expandListener(element) {
+    element.target.classList.toggle('traffic-event-entry-expanded');
+  }
+
+  /**
+   * Attaches a listener to the given traffic event entry div.
+   * @param {HTMLElement} element the element to attach the listener to.
+   */
+  function addExpandListener(element) {

[James Hawkins, 2014/03/27 21:01:25]

Who calls this method?

[rlarocque, 2014/03/27 21:19:54]

It's used to attach listeners to the elements created by jstemplate.  See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res....

+    element.addEventListener('click', expandListener, false);
+  }
+
+  function onLoad() {
+    initStatusDumpButton();
     initProtocolEventLog();
+
+    chrome.sync.events.addEventListener(
+        'onAboutInfoUpdated',
+        onAboutInfoUpdatedEvent);
+
+    // Register to receive a stream of event notifications.
+    chrome.sync.registerForEvents();
+
+    // Request an about info update event to initialize the page.
+    chrome.sync.requestUpdatedAboutInfo();
   }
 
   return {
     onLoad: onLoad,
     addExpandListener: addExpandListener
   };
 });
 
 document.addEventListener(
     'DOMContentLoaded', chrome.sync.about_tab.onLoad, false);