769424 - Flush sync data prior to suspend/resume

Status: RESOLVED INCOMPLETE

(Firefox for Metro Graveyard :: Sync, defect)

Description

[Jim Mathies [:jimm]]

The general idea here is - 

1) metrofx should flush changes via sync prior to suspend
2) on resume, metrofx should pull changes

We also probably want to ping running instances of the desktop when changes are detected and metrofx is suspended. When the desktop picks up focus it can then do a pull as well.

This should more or less keep the two browsers in sync. The overall goal here is to test the concept to see how well it works.

Comment 1

[Brian R. Bondy [:bbondy]]

You'll want to listen for suspend_process_notification and resume_process_notification

Comment 2

[Jim Mathies [:jimm]]

I have two experimental implementations of this logic. The first used cross process signaled events, which I might end up going back to at some point. This though grew in complexity rather quickly, so I flipped to using simple Windows messaging for "signals" instead. This appears to be working reliably and is less complex than the events code.

Currently this code lives down in widget.  This though is the wrong place for it. I think this needs to be broken up into two components which I'm looking at currently, specifically:

1) lower level cross-process signaling module that can be leveraged by front end code.
2) front end sync related changes that:
 a) have information about the sync account in use
 b) will know about the results of the sync through bug 770298
 c) based on a & b will control whether or not the other process gets signaled

Also I've updated the win8 wiki with a summary of the issues revolving around this whole plan - 
https://wiki.mozilla.org/Firefox/Windows_8_Integration#Sync_specific

Comment 3

[Jim Mathies [:jimm]]

Attachment: fx frontend patch

Comment 4

[Jim Mathies [:jimm]]

Attachment: fx frontend patch

Comment 5

[Jim Mathies [:jimm]]

Attachment: metro frontend patch

Comment 6

[Jim Mathies [:jimm]]

Attachment: backend patch

Comment 7

[Jim Mathies [:jimm]]

A few notes on this – the initial implementation  of this used events and pipes to transfer data around. This became overly complex pretty quick and seemed like overkill. I think though down the road this work could be useful. One of the nice things about the pipe method is we can share a large quantity of string data between two browsers. Which we might use to share streams of sync data (json strings for example) between two browsers running on the same machine which could remove our reliance on networking. Sync would also be more real time. However the current sync client doesn’t support any of this at this time, so I pushed this out to future.

The impl. posted here is much simpler. The only string data shared between the two browsers is a sync account string which is used to be sure both browsers the browsers that sync are using the same account. The backend is a simple static class that uses the registry and windows messaging to trigger events. The language is also really simple as well, basically involving two conversations:

On suspend:
metro:   suspend event -> sync -> kMsgDesktopShouldSync
desktop: kMsgDesktopShouldSync -> sync event

On resume:
metro:   resume -> kMsgDesktopShouldSyncAndReply
desktop: kMsgDesktopShouldSyncAndReply -> sync -> kMsgMetroSyncComplete
metro:   kMsgMetroSyncComplete –> sync

The driver is always the metro browser and is based on the suspend / resume events we get from metro. This scheme also assumes only one desktop browser is involved, although nothing will break if two desktop browsers are running in separate profiles both synced to the same account. (An outlier case for sure.)

The nice thing about this is that it can be expanded to handle sync setup fairly easily. For example if on initial install one browser signs up/into sync, that browser can populate its account info in the registry so the other browser can pick it up and set it in its prefs. The front end could then adapt it’s sync ui to this for simple setup in the browser that hasn’t registered yet. This relationship can work both ways depending on which browser signs up/in first.

This scheme does hit the sync service quite a bit. One of the things I noticed was that when Weave.sync() is called a network hit always results. Usually no data is transferred, but a hit results none the less. This is something we are going to want to minimize. The metro browser can go in and out of suspend state independent of the desktop coming up – switching around to different apps  will cause this, so in this use case (most common) on each suspend / resume a call to sync() is made. For testing purposes though this generally works pretty well.

The plan at this point is to land this on elm, and potentially merge it into mc with the elm merge. Note the code here is ifdef’d to MOZ_METRO builds which will be off on mc once we merge. So I don’t see any harm in this. However once we get to the point of turning these builds on on mc, a decision will need to be made as to whether we want to go this route, and what changes will need to be made before turning it on in production.

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 643784 [details] [diff] [review]
fx frontend patch

gps, based on the description below, would it be ok to land this on elm? Questions, comments, etc.? I still plan to set up a sync meeting. Will schedule that soon.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 643784 [details] [diff] [review]
fx frontend patch

Review of attachment 643784 [details] [diff] [review]:
-----------------------------------------------------------------

I would really like us to have that meeting before we start landing code for this. Basically, I want to know what the end state we're working towards will be so I have context when doing all the reviews.

Comment 10

[Jim Mathies [:jimm]]

We're going to go with some form of local sync (bug 768638). Resolving this out.

Comment 11

[Richard Newman [:rnewman]]

More broadly, there are a set of events that would need to be cross-pollinated: not just Sync configuration events (start-over, etc.), but also "Clear Private Data" and friends.

Comment 12

[Jim Mathies [:jimm]]

(In reply to Richard Newman [:rnewman] from comment #11)
> More broadly, there are a set of events that would need to be
> cross-pollinated: not just Sync configuration events (start-over, etc.), but
> also "Clear Private Data" and friends.

I was thinking this service could be very generalized so that it isn't specific to sync. 

The scale of complexity here starts simple and grows pretty fast depending on the requirements. Some factors to consider:

1) data sharing vs. simple triggers
2) dynamically defined events vs. static
3) number of players: one-to-one vs. one-to-many

On windows the simplest level is a set of triggers that are static with data sharing being handled outside the system. Once you start sharing data though or creating dynamic events that you define at runtime you have to consider using pipes for communication, and that is when things get a bit more complex, especially if the number of processes is > 2.

Comment 13

[Richard Newman [:rnewman]]

(In reply to Jim Mathies [:jimm] from comment #7)

> This scheme does hit the sync service quite a bit. One of the things I
> noticed was that when Weave.sync() is called a network hit always results.
> Usually no data is transferred, but a hit results none the less. This is
> something we are going to want to minimize.

Just a random note here: I have a sync-on-quit addon that might be of academic interest. 

  https://github.com/rnewman/sync-on-exit

If you want to *sometimes* avoid network access, you can decide if your rule is "we want to sync if there are *outgoing changes*", or if you want to capture incoming changes too (which requires network access).

The design I would prefer for this is to provide an API in SyncScheduler to which you can express your requirement ("please sync right now if there's outgoing data"), rather than to duplicate scheduling and backoff logic elsewhere.

I am happy to review that change if someone wants to do it soon, or I'm happy to turn an articulation of priority into an item in our schedule!

Comment 14

[Jim Mathies [:jimm]]

general->sync 10 bugs

Comment 15

[Jim Mathies [:jimm]]

I'm not sure if we need this currently. Expected reliance on sync for the first train ride has decreased since we first started investigating. In the future we would like both desktop and metro to be in sync at all times. (This need will hopefully be addressed in sync 2.0.) But for now, we are isolating the two browser to an extent. Desktop and Metro bookmarks do not commingle in the same UI, and data sets like passwords and auto-complete update infrequently such that it's unlikely users would notice.

I'm tempted to moderate this bug somewhat, maybe by making it metro only - a "sync on suspend/resume in metrofx only" feature without desktop changes, or closing it out as wontfix and relying instead on the normal 90 second sync behavior we already have.

Asa, any thoughts?

Comment 16

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #15)
> I'm tempted to moderate this bug somewhat, maybe by making it metro only - a
> "sync on suspend/resume in metrofx only" feature without desktop changes, or
> closing it out as wontfix and relying instead on the normal 90 second sync
> behavior we already have.

Note that if the browser is suspended beyond the 90 second timeout it'll sync as soon as it resumes on its own, so we already have partial support here, and it would be trivial to schedule a sync as soon as we resume just to be safe.

Syncing on suspend is bit more tricky, we can't call sync() directly (see bug 809171), we have to schedule a sync and wait for it to complete. We can probably do this assuming we can rely on sync observer events to tell us when the sync completes or errors out.

Comment 17

[Asa Dotzler [:asa]]

I don't think we should worry about this for v1.

Comment 18

[Marco Mucci [:MarcoM]]

Hey Jim, based on Asa's comment do you want to move this to V2: Bug 861680

Comment 19

[Firefox Bug Husbandry Bot]

Mass close of bugs in obsolete product https://bugzilla.mozilla.org/show_bug.cgi?id=1350354