sync: Buffer Protocol Events for about:sync page

sync: Buffer Protocol Events for about:sync page

Allows the about:sync page to present the last six network events as
soon as it's opened.

Adds the ProtocolEventBuffer to the SyncManagerImpl.  This class holds
on to a few protocol events and returns them on demand.

Modifies the interface to enable SyncBacknedHost protocol event
forwarding.  By default, it does not forward any events.  There are now
separate registration and unregistration functions.  The registration
function will send the set of buffered notifications to the UI thread,
and also register it to receive incoming events in the future.  It
continues to forward events until the number of registration calls is
matched by the number of un-registration calls.

Makes about:sync's registration to receive events explicit.  This was
a long-time TODO.  If we did not fix this issue, then the about:sync
page could receive the initial set of buffered invalidations before it
had its event listeners defined and registered.

Makes the about:sync page keep track of known events and avoid adding
duplicates to the list.  Since the registration of a new event listener
causes events to be distributed to all listeners, we must add this logic
to ensure that opening one new about:sync tab will not cause previously
opened tabs to display duplicate events.

BUG=329373, 349301
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260726

[rlarocque, 2014-03-27 00:05:35]

Here's the next step after https://codereview.chromium.org/210233005/.

To be honest, I'm not very enthusiastic about this change.  Keeping a buffer
means we need to pay a cost for this feature even if about:sync is never open. 
On the other hand, the "traffic logger" currently supports this feature, and it
will be easier to remove it if we can say that the protocol events'
functionality is a superset of what the traffic logger offers.

Once this is done, we can remove the traffic logger and a bunch of fields from
about:sync's about tab.

Please review.

[rlarocque, 2014-03-27 00:11:36]

Almost forgot:

jhawkins: Please review chrome/browser/resources/ and chrome/browser/ui/webui/.
timsteele: Please review the rest.

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

https://codereview.chromium.org/212603007/diff/1/chrome/browser/resources/syn...
File chrome/browser/resources/sync_internals/about.js (right):

https://codereview.chromium.org/212603007/diff/1/chrome/browser/resources/syn...
chrome/browser/resources/sync_internals/about.js:22: /** We may receive
re-delivered events.  Keep a record of ones we've seen. */
Can we not fix the issue of re-delivered events?

https://codereview.chromium.org/212603007/diff/1/chrome/browser/resources/syn...
chrome/browser/resources/sync_internals/about.js:121: function
addExpandListener(element) {
Who calls this method?

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

https://codereview.chromium.org/212603007/diff/1/chrome/browser/resources/syn...
File chrome/browser/resources/sync_internals/about.js (right):

https://codereview.chromium.org/212603007/diff/1/chrome/browser/resources/syn...
chrome/browser/resources/sync_internals/about.js:22: /** We may receive
re-delivered events.  Keep a record of ones we've seen. */
On 2014/03/27 21:01:25, James Hawkins wrote:
> Can we not fix the issue of re-delivered events?

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.

https://codereview.chromium.org/212603007/diff/1/chrome/browser/resources/syn...
chrome/browser/resources/sync_internals/about.js:121: function
addExpandListener(element) {
On 2014/03/27 21:01:25, James Hawkins wrote:
> Who calls this method?

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....

[James Hawkins, 2014-03-27 21:21:44]

LGTM

[rlarocque, 2014-03-28 18:12:44]

Tim: ping.

[tim (not reviewing), 2014-03-28 20:45:42]

It seems like there's a lot of overlap with TrafficRecorder.  
Is there any way you can factor these together even now, to avoid having to have
two recording subsystems (and APIs for embedders of SyncManager to care about)?

It'd be nice for code organization if this kept sync engine debug recording code
together, since there's already plumbing in src/sync for debug recording. I
could have imagined a debug event "sink" where we can more easily (i.e less
places to understand + change) control what things are recording when (traffic
from SyncerProtoUtil, events from SyncManager), or alternatively fully embracing
PEvents as a replacement for TR and choosing between one of the two systems at
runtime if you don't think we need both systems around long term.

https://codereview.chromium.org/212603007/diff/1/sync/internal_api/protocol_e...
File sync/internal_api/protocol_event_buffer.h (right):

https://codereview.chromium.org/212603007/diff/1/sync/internal_api/protocol_e...
sync/internal_api/protocol_event_buffer.h:22: static const size_t kBufferSize;
do you actually use this anywhere besides this class / the cc file?

[rlarocque, 2014-03-28 21:00:34]

On 2014/03/28 20:45:42, timsteele wrote:
> It seems like there's a lot of overlap with TrafficRecorder.  
> Is there any way you can factor these together even now, to avoid having to
have
> two recording subsystems (and APIs for embedders of SyncManager to care
about)?

The overlap is intentional.  As I mentioned in the original review request, I
dislike this buffer but decided to add it only because the TrafficRecorder has
this feature.  The plan is to land this change then remove the TrafficRecorder. 
This solution is more useful and less expensive, even with the buffer.

> It'd be nice for code organization if this kept sync engine debug recording
code
> together, since there's already plumbing in src/sync for debug recording. I
> could have imagined a debug event "sink" where we can more easily (i.e less
> places to understand + change) control what things are recording when (traffic
> from SyncerProtoUtil, events from SyncManager), or alternatively fully
embracing
> PEvents as a replacement for TR and choosing between one of the two systems at
> runtime if you don't think we need both systems around long term.

Which debug recording code are you referring to?

[rlarocque, 2014-03-28 21:00:48]

https://codereview.chromium.org/212603007/diff/1/sync/internal_api/protocol_e...
File sync/internal_api/protocol_event_buffer.h (right):

https://codereview.chromium.org/212603007/diff/1/sync/internal_api/protocol_e...
sync/internal_api/protocol_event_buffer.h:22: static const size_t kBufferSize;
On 2014/03/28 20:45:42, timsteele wrote:
> do you actually use this anywhere besides this class / the cc file?

I exposed it for tests.  It's a const, so I figured it would be harmless.

[tim (not reviewing), 2014-03-28 21:30:08]

On 2014/03/28 21:00:34, rlarocque wrote:
> On 2014/03/28 20:45:42, timsteele wrote:
> > It seems like there's a lot of overlap with TrafficRecorder.  
> > Is there any way you can factor these together even now, to avoid having to
> have
> > two recording subsystems (and APIs for embedders of SyncManager to care
> about)?
> 
> The overlap is intentional.  As I mentioned in the original review request, I
> dislike this buffer but decided to add it only because the TrafficRecorder has
> this feature.  The plan is to land this change then remove the
TrafficRecorder. 
> This solution is more useful and less expensive, even with the buffer.
> 
> > It'd be nice for code organization if this kept sync engine debug recording
> code
> > together, since there's already plumbing in src/sync for debug recording. I
> > could have imagined a debug event "sink" where we can more easily (i.e less
> > places to understand + change) control what things are recording when
(traffic
> > from SyncerProtoUtil, events from SyncManager), or alternatively fully
> embracing
> > PEvents as a replacement for TR and choosing between one of the two systems
at
> > runtime if you don't think we need both systems around long term.
> 
> Which debug recording code are you referring to?

TrafficRecorder. BTW, yeah, I had read your note about intentional overlap, but
you could have still chosen to use TrafficRecorder to buffer your events; it is
currently the thing in the sync engine responsible for recording debug events in
a circular buffer and this seems to fit in pretty well (regardless of / separate
from the question of whether we want to remove the other circular buffer it
holds later on because we don't want to watch ClientToServerMessages anymore..]

But OK, if we're committed to answering the latter question and killing TR and
that's next up, LGTM.

[rlarocque, 2014-03-28 21:38:55]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-03-28 21:42:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/212603007/1

[commit-bot: I haz the power, 2014-03-28 21:42:37]

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

[commit-bot: I haz the power, 2014-03-28 21:42:38]

Failed to apply patch for sync/sync_internal_api.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file sync/sync_internal_api.gypi
  Hunk #1 FAILED at 40.
  1 out of 1 hunk FAILED -- saving rejects to file
sync/sync_internal_api.gypi.rej

Patch:       sync/sync_internal_api.gypi
Index: sync/sync_internal_api.gypi
diff --git a/sync/sync_internal_api.gypi b/sync/sync_internal_api.gypi
index
8f4644285924a842683955ae1161ab9dfa346538..a701486bd7d5aa3bfeb96a910de879fa386fecb1
100644
--- a/sync/sync_internal_api.gypi
+++ b/sync/sync_internal_api.gypi
@@ -40,6 +40,8 @@
     'internal_api/js_sync_encryption_handler_observer.h',
     'internal_api/js_sync_manager_observer.cc',
     'internal_api/js_sync_manager_observer.h',
+    'internal_api/protocol_event_buffer.cc',
+    'internal_api/protocol_event_buffer.h',
     'internal_api/public/base/ack_handle.cc',
     'internal_api/public/base/ack_handle.h',
     'internal_api/public/base/cancelation_observer.cc',

[rlarocque, 2014-03-31 21:36:52]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-03-31 21:37:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/212603007/20001

[commit-bot: I haz the power, 2014-04-01 01:20:16]

Change committed as 260726