534956 - Sync add-ons

Status: VERIFIED FIXED

(Firefox :: Sync, enhancement, P4)

Description

[Anant Narayanan [:anant]]

We should work on getting Addon Sync into Weave. 

What we need: https://wiki.mozilla.org/User:Mconnor/AddonSync
Possible structure on server side: https://wiki.mozilla.org/Labs/Weave/WEP/109

Comment 1

[Anant Narayanan [:anant]]

Changes are going into this user repo: http://hg.mozilla.org/users/anarayanan_mozilla.com/weave-addons/

Step 1, i.e. getting a list of enabled addons and storing them indexed by client on the server is done: http://hg.mozilla.org/users/anarayanan_mozilla.com/weave-addons/rev/0bac4011d21a

Next steps are documented on Connor's wiki page, repeating here for convenience:

- Add code that compares the list of local addons to the remote ones along with the application type to determine what addons must be installed
- Add UI to prompt the user if that list is of length > 0
- If a user explicitly chooses not to install an addon on the client, mark as 'donotwant' on the server, and add a UI hook to allow users to change their mind later.

So far, so good. The tricky part is detecting uninstalled addons and removing them from all client. Simply comparing the list of addons between, say client A and B, will not suffice because an addon missing from list B may mean that it was uninstalled at B *or* that it was installed at A. We probably need timestamps or extra flags to disambiguate.

Comment 2

[Maaren]

Bug 428380 is a duplicate?

Comment 4

[Anant Narayanan [:anant]]

I merged the http://hg.mozilla.org/users/anarayanan_mozilla.com/weave-addons/ user repo with Weave trunk and added in a bunch of more fixes. Most prominently, the URL of the XPI of each extension (if it can be found via addons.mozilla.org) is now part of the record. This should complete the engine part of the problem.

Next steps: figuring out the right Ux for prompting the user to install addons. Currently, an 'information' notification is generated if addons not present locally are detected. Upon clicking this notification, user is prompted a dialog where choices can be made. Further changes to this can be easily made by modifying addon-dialog.js/xul in the chrome directory.

Comment 6

[Ed Lee :Mardak]

Attachment: WIP v1

Attaching patch for thunder's reference. (I haven't touched this for a while, but still seems to apply to trunk tip.)

Comment 7

[Roger Ye]

Thanks for having this, below is the idea I've got.

1. Weave should monitor the list of locally installed add-on's, and should follow the local add-on changes (install/uninstall) then react respectively.

2. When new add-on installed, ask the user if the new add-on should be automatically installed on other weave clients

3. When uninstalling an add-on, choices are: a) local change, don't change server data; b) change server data, but ask on other clients whether to uninstall; c) change server data, and uninstall automatically (like an untrusted add-on)

Thanks

Comment 9

[Philipp von Weitershausen [:philikon]]

This is on me now.

Comment 10

[Philipp von Weitershausen [:philikon]]

Attachment: WIP v2

Here's a WIP, based on Firefox 4+'s add-on manager. Pretty sure this doesn't work yet (*shakes fist at AddonRepository*), but it illustrates the implementation roughly.

Comment 11

[Philipp von Weitershausen [:philikon]]

Attachment: WIP v3

Bunch of typo fixes. This version actually works. Sorta. A bit. (Not really.)

Requires a small change in AddonRepository (separate patch)

Comment 12

[Philipp von Weitershausen [:philikon]]

Attachment: patch AddonRepository.getAddonsByIDs()

Make the addons that are returned by AddonRepository.getAddonsByIDs() have a non-null .install.

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 522713 [details] [diff] [review]
patch AddonRepository.getAddonsByIDs()

Sorry for the delay but I think this looks ok. I'm concerned that we shouldn't be sending the timing metrics along with your requests.

Comment 14

[Philipp von Weitershausen [:philikon]]

(In reply to comment #13)
> Sorry for the delay but I think this looks ok. I'm concerned that we shouldn't
> be sending the timing metrics along with your requests.

I agree. I was just merely trying to solicit feedback whether this would be a change that you guys were comfortable with. The next thing would indeed be making the metrics stuff optional.

Comment 17

[Gregory Szorc [:gps]]

Attachment: WIP v4

This is my current work in progress. It is based on philikon's patch, but significant parts are rewritten. I don't claim it works, but it's getting there.

It is no way ready for a review. Just uploading my progress before the weekend in case my laptop gets swallowed by a bear.

Comment 18

[Jonathan Griffin (:jgriffin)]

I should be able to update TPS to handle testing this, when it's ready.

Comment 19

[Gregory Szorc [:gps]]

Attachment: WIP v5

Here's the latest patch. It is still far from complete. Differences from last version:

* It actually works! (Well, the install case does, anyway.)
* Add-ons installed after restart are detected
* Application ID recorded

There's still a lot missing, including UX prompting to restart to finish installation and a robust unit test suite.

The patch is being posted for informal progress review.

Comment 20

[Gregory Szorc [:gps]]

(In reply to Dave Townsend (:Mossop) from comment #13)
> Comment on attachment 522713 [details] [diff] [review]
> patch AddonRepository.getAddonsByIDs()
> 
> Sorry for the delay but I think this looks ok. I'm concerned that we
> shouldn't be sending the timing metrics along with your requests.

As it was explained to me, the "timing metric" that gets sent is along the lines of "hey, AMO, tell me about the extensions with these IDs." And, AMO happens to use the counts of which extensions were queried about to determine and report usage.

Well, my take on that is: what's the problem? If Sync is sending a request to AMO, it will be for extensions that /should/ be installed on the client. Assuming the extension is installed without error (again, it /should/ if we do our job correctly), the net result is the daily ping is started a day earlier than it normally would. Big deal: we start something a little earlier that would be started anyway. If the extension is not installed correctly, we just get a single extra count on the day we attempted the install. Sync should not try reinstalling the extension (until the modified time of the record is updated, anyway), so we wouldn't see daily pings from clients that failed install. The net result here is a single extra query on one day.

Now, I don't know exactly what goes into the "timing metric" that goes out and how it is used. But from what has been explained to me, I think Sync's sending of the "timing metric" is a non-issue. This is where someone with detailed knowledge of the metric should step in...

Comment 21

[Dave Townsend [:mossop]]

(In reply to Gregory Szorc [:gps] from comment #20)
> (In reply to Dave Townsend (:Mossop) from comment #13)
> > Comment on attachment 522713 [details] [diff] [review]
> > patch AddonRepository.getAddonsByIDs()
> > 
> > Sorry for the delay but I think this looks ok. I'm concerned that we
> > shouldn't be sending the timing metrics along with your requests.
> 
> As it was explained to me, the "timing metric" that gets sent is along the
> lines of "hey, AMO, tell me about the extensions with these IDs." And, AMO
> happens to use the counts of which extensions were queried about to
> determine and report usage.

We also send performance data about how long Firefox took to startup with those add-ons installed. If you ping AMO with a list of add-ons that aren't installed then those numbers are misleading.

> Well, my take on that is: what's the problem? If Sync is sending a request
> to AMO, it will be for extensions that /should/ be installed on the client.
> Assuming the extension is installed without error (again, it /should/ if we
> do our job correctly), the net result is the daily ping is started a day
> earlier than it normally would. Big deal: we start something a little
> earlier that would be started anyway. If the extension is not installed
> correctly, we just get a single extra count on the day we attempted the
> install. Sync should not try reinstalling the extension (until the modified
> time of the record is updated, anyway), so we wouldn't see daily pings from
> clients that failed install. The net result here is a single extra query on
> one day.

We see two pings from the client on days when sync attempts to do this, regardless of whether it succeeds or fails I think which will cause some add-ons to get double counted on those days.

Sure, the number of cases we do this is probably small, but data like this is becoming vital to help understand our user-base, I don't want to see us add a feature which makes our data worse even if it is only a little bit.

Comment 22

[Gregory Szorc [:gps]]

OK, so we have an open issue regarding the performance data and pings w.r.t. add-on sync. Are there JS/HTTP APIs to support doing what we need? If not, which are needed (so I can file the appropriate bugs)?

Comment 23

[Dave Townsend [:mossop]]

(In reply to Gregory Szorc [:gps] from comment #22)
> OK, so we have an open issue regarding the performance data and pings w.r.t.
> add-on sync. Are there JS/HTTP APIs to support doing what we need? If not,
> which are needed (so I can file the appropriate bugs)?

I'm not sure what you're asking for, the patch needs to be improved such that metadata requests only include the performance numbers when it is the regular daily update ping.

Comment 24

[Gregory Szorc [:gps]]

Attachment: Add-on engine v1

The implementation of the add-on Sync engine is progressing nicely. The attached is the implementation of the engine and some tests, which all pass. This won't work unless some other patches for AddonManager are applied. If you want to be brave, see my patch queue at https://hg.mozilla.org/users/gszorc_mozilla.com/mq-sc/. You'll need every patch beginning with "addonsync." They should apply cleanly in any order.

There are a number of outstanding issues with this patch. Some are documented as TODOs at the top of the engine file.

But, I feel it is far enough along for an informal review. I'm sure there are plenty of nits you don't have to remind me about. I'll leave it up to the informal reviews to decide how much nitpicking they want to do now versus leave for the real review.

Comment 25

[Philipp von Weitershausen [:philikon]]

Comment on attachment 559322 [details] [diff] [review]
Add-on engine v1

Given that there are still some UI and test pieces missing, please separate this patch into three parts: implementation, UI, and tests. That makes review much easier and lets us rev those parts independently. Oh and then there are TPS tests, which would be a fourth part.

>+// TODO need TPS unit tests

Oh yeah, that... That'd be a fourth part then.

>+"use strict";

<3

>+function AddonsEngine() {
>+  SyncEngine.call(this, "Addons");
>+
>+  // This assumes that the engine is instantiated at most once in each app.
>+  // If this ever changes, this will yield duplicate change records.
>+  this._tracker._trackStartupChanges(this);

Yes, you can very much make that assumption. We make it in a lot of other engines, too.

>+  getChangedIDs: function getChangedIDs() {
>+    let changed = this._tracker.changedIDs;

I kind of feel like using the default impl here:

  let changed = SyncEngine.prototype.getChangedIDs.call(this);

>+    let addons = this._store._getAddons();
>+    for (let i = 0; i < addons.length; i++) {
>+      let addon = addons[i];
>+
>+      // TODO do we need to filter add-ons beyond _getAddons()?
>+      let updated = addon.updateDate;
>+      let installed = addon.installDate;
>+
>+      let useDate = (updated > installed ? updated : installed) / 1000;
>+
>+      if (useDate > lastSync && !(addon.syncGUID in changed)) {
>+        this._log.trace("Supplementing untracked add-on change: "
>+                        + addon.syncGUID);
>+        changed[addon.syncGUID] = useDate;
>+      }
>+    }

This means we're getting *all* addons for each sync, even though for the absolute vast majority of syncs nothing will have changed. This isn't going to work. We should find a way to either track those or at the most do a SQL range query. Actually, we should just track period. Addons change so rarely, any kind of DB I/O isn't justified IMHO.

>+  isAddonSyncable: function isAddonSyncable(addon) {
>+    const syncable_types = ["extension", "theme", "locale"];

Please define this on the prototype to avoid recreating this array each time this function is called (which is a lot). And no, the JIT is smart enough to figure that out on its own.

>+
>+    if (!addon) return false;
>+    if (!addon.type in syncable_types) return false;

Nit: coding style. Please use curly braces and newlines.

Also, Pythonista rookie mistake: the 'in' operator doesn't think what you think it does. In JS it always checks for *keys* which on an array are the indices. You want syncable_types.indexOf(addon.type) != -1

Lastly, even if the 'in' were correct here, the ! operator binds stronger than 'in', so you would have to use an extra pair of parentheses here.

>+    if (addon.scope | AddonManager.SCOPE_PROFILE) {
>+      return true;
>+    }
>+
>+    return false;
>+  },

I feel like we can write this method as

  return addon &&
         syncable_types.indexOf(addon.type) != -1 &&
         addon.scope | AddonManager.SCOPE_PROFILE;

>+  /**
>+   * Applies all incoming record changes to the local client.
>+   *
>+   * The logic for applying incoming records is split up into two phases:
>+   *
>+   *  1. Collect all needed changes for incoming records
>+   *  2. Apply them
>+   *
>+   * It is written this way to make the logic clearer and to make testing
>+   * easier.

Assembling various lists in _assembleChangesFromRecords() and then acting upon them is elegant in a way, but we're creating lots of overhead. Like we've said so often, addons sync will not have many records and not incur many changes, so it will hardly push us past existing performance boundaries. Also, many of your object creations are O(1) and not O(n), so I'm not as worried.

That said, if I had written that code, I would probably have done this:

1) Loop over the incoming records and immediately process enabling/disabling, uninstalling and GUID changes.
2) Filter out the items handled in step (1) in place. See the history engine's applyIncomingBatch for an example.
3) Remaining list only contains addons to install. Handle those like you do already.

Note that once you get the list of records, they're yours, so you can just add properties to them, e.g. store the corresponding addon object, etc.

Anyway, I appreciate the testability of your approach, so I'm fine with it. . Just keep in mind for future references that there are edge cases out there and perf matters. People *do* have tens of thousands of bookmarks and they *do* have hundreds of tabs, etc.

>+  _assembleChangesFromRecords:
>+    function _assembleChangesFromRecords(records, addons) {
>+
>+    if (addons === undefined) {
>+      addons = this._getAddons();
>+    }
>+
>+    let addons_by_id    = {}; // Add-on ID to add-on object
>+
>+    for each (let addon in addons) {
>+      addons_by_id[addon.id] = addon;
>+    }

You're also doing this in _applyChanges. Can you not assemble this map in applyIncomingBatch and then pass the map to both _assembleChangesFromRecords and _applyChanges?

>+    let uninstall_ids = {};
>+    let updated_guids = {};
>+    let install_ids   = {};
>+    let enable_ids    = {};
>+    let disable_ids   = {};

You're using most of those essentially as arrays, not as maps. (Only install_ids and updated_guids are maps.)

>+
>+    // Examine each incoming record and record actions that need to be
>+    // taken. It is important that this actual loop not do anything too
>+    // expensive, as it executes synchronously to the event loop.
>+    for each (let record in records) {
>+      let guid = record.id;
>+      let id = record.extension_id;

Can you name that variable 'ext_id' or something like that please?

>+    // Uninstall requests take priority over all others.
>+    for each (let id in Object.keys(uninstall_ids)) {

This is twice as complicated as it needs to be. You can just write:

  for (let id in uninstall_ids) {

You do those all across the code, please fix all instances.

>+  _applyChanges: function _applyChanges(changes, addons) {
>+    if (addons === undefined) {
>+      addons = this._getAddons();
>+    }
>+
>+    let addonMap = {};
>+    for each (let addon in addons) {
>+      addonMap[addon.id] = addon;
>+    }
>+
>+    // TODO all the XPIProvider calls need to be asynchronous
>+
>+    // _assembleChangesFromRecords() ensures that all requested changes aren't
>+    // in a state of conflict. So, it should be safe to attempt any change
>+    // operation in any order. That being said, we start with the changes least
>+    // likely to cause errors just so we error on the side of getting as much
>+    // done as possible.
>+
>+    // GUID changes are pretty cheap, so we start with them.
>+    // Unfortunately, their API is synchronous.

As in, this will incur disk I/O?

>+    // Finally, we install add-ons.
>+    // We will eventually want to install add-ons that are not on AMO. Until
>+    // then, we search for an add-on using its registered channels and hope
>+    // to find something.
>+    let install_ids = Object.keys(changes.install);
>+    if (install_ids.length) {
>+      this._log.info("Attempting to install add-ons: " + install_ids);
...
>+    }

I don't see anywhere in this block where you assign the newly installed add-on the syncGUID that we're getting from upstream.

>+  /**
>+   * Obtains changes made during startup and adds them to the tracker.
>+   *
>+   * This is typically called when the engine first starts upon application
>+   * start-up. It only needs to be called once during the lifetime of the
>+   * application.
>+   */
>+  _trackStartupChanges: function _trackStartupChanges(engine) {

Could also do this instead of the parameter:

  let engine = Weave.Engines.get("addons");

Ugly as fuck, I know, but we do it in a bunch of places already. Do you know understand what I meant with leaky abstractions? :)

>+    const change_map = {
>+      installed:   AddonManager.STARTUP_CHANGE_INSTALLED,
>+      changed:     AddonManager.STARTUP_CHANGE_CHANGED,
>+      uninstalled: AddonManager.STARTUP_CHANGE_UNINSTALLED,
>+      disabled:    AddonManager.STARTUP_CHANGE_DISABLED,
>+      enabled:     AddonManager.STARTUP_CHANGE_ENABLED
>+    };

Ditto about defining that const outside the function.

>+    let changed_ids = {};
>+
>+    for each (let type in change_map) {

Why is change_map a map and not just an array?

>+      for each (let id in AddonManager.getStartupChanges(type)) {

Is this call going to do disk I/O. If so, _trackStartupChanges should not be in the same call path as Sync startup. IOW, the engine should call it on the next event loop tick.

>+        changed_ids[id] = true;
>+      }
>+    }
>+
>+    let updated = false;
>+
>+    let log = this._log;
>+    let store = engine._store;
>+    let tracker = this;

Not sure why, but ok.

>+    for each (let id in Object.keys(changed_ids)) {
>+      let cb = Async.makeSyncCallback();
>+      AddonManager.getAddonByID(id, cb);
>+      let addon = Async.waitForSyncCallback(cb);
>+      if (!addon) return;
>+      if (!store.isAddonSyncable(addon)) return;

Did you mean 'continue' instead of 'return' here? Also, coding style!

>+      log.debug("Marking add-on as changed from startup: " + id);
>+      tracker.addChangedID(addon.syncGUID);
>+      updated = true;

Feels like you could increment the score here and then bail (return) immediately.

>+  _trackAddon: function _trackAddon(addon, action, params) {
>+    this._log.trace(action + " called for " + addon.id);
>+
>+    //if (addon.scope !== AddonManager.SCOPE_PROFILE) {
>+    //  return;
>+    //}

...

>+    if (!addon.syncGUID) {
>+      this._log.info("Add-on has no GUID: " + addon.id);
>+      return;
>+    }

Could this ever occur? If so, just generate a GUID, assign the add-on one, and then track it!

>+  decodeBase64url: function decodeBase64url(data) {
>+    return atob(data.replace('-', '+', 'g').replace('_', '/', 'g'));
>+  },

Do we still need this? I don't think we do...

>-var gProfD;
>+let gSyncProfile;
> 
> do_load_httpd_js();
>-gProfD = do_get_profile();
>+gSyncProfile = do_get_profile();

You can combine those two lines, btw (the 'let' and the assignment). There used to be compat stuff for the old extension in between there that got removed.


I'm skipping the rest of the tests for now because I'm tired. So I'm leaving the feedback? flag there. Like I said, next time please create separate patches so that they're easier to digest :)

Comment 26

[Gregory Szorc [:gps]]

Attachment: Add-on engine, v2

OK, new direction for the implementation. All the crazy work around tracking add-on changes inside the engine has been replaced by an AddonsReconciler class. This is effectively a copy of relevant data from the extensions database. It also includes a log of changes to add-ons over time.

Now, when sync runs, it just says "get changes since last sync time" and things just work. As the documentation says, this should (arguably) be part of the AddonManager API. We decided it would be too much work to bake it there if we have any intent of landing add-on sync in Firefox 11. If/when it lands in AddonManager, we can refactor Sync to use it.

Known issues:

* Test code is in shambles. xpcshell tests are bit rotted. TPS tests fail horribly.
* There are probably corner cases in AddonsReconciler. These will get covered by tests.
* I haven't massaged code for style lately. Likely lots of nits.

I'm mainly interested in feedback on the interaction with AddonManager and AddonRepository. Is there anything obvious I'm doing wrong?

Comment 27

[Dave Townsend [:mossop]]

Comment on attachment 577811 [details] [diff] [review]
Add-on engine, v2

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

Only really looked over the interactions with the add-ons manager and they are mostly fine. A couple of notes that may or may not require changes but the direction here seems good.

The nested event loops in services code makes me die a little inside.

::: services/sync/modules/addonsreconciler.js
@@ +268,5 @@
> +        this._listeners.splice(pos, 1);
> +      } else {
> +        pos++;
> +      }
> +    }

Array.filter ftw!

@@ +464,5 @@
> +      // We assume that every event for non-restartless add-ons is
> +      // followed by another event and that this follow-up event is the most
> +      // appropriate to react to. Currently we ignore onEnabling, onDisabling,
> +      // and onUninstalling for non-restartless add-ons.
> +      if (requiresRestart != undefined && !requiresRestart) {

Just requiresRestart === false

::: services/sync/modules/engines/addons.js
@@ +314,5 @@
> +    this._log.debug("Uninstalling add-on: " + addon.id);
> +    addon.uninstall();
> +
> +    // This may take a while, so we pause the event loop.
> +    this._sleep(0);

uninstall generally happens synchronously so I'm not sure why you're waiting, but if you want to wait wait till the appropriate methods are sent rather than guessing

@@ +328,5 @@
> +    if (record.enabled == addon.userDisabled) {
> +      this._log.info("Updating userEnabled flag: " + addon.id);
> +
> +      addon.userDisabled = !record.enabled;
> +      this._sleep(0);

Same as above

@@ +493,5 @@
> +    AddonRepository.getCachedAddonByID(addon.id, cb);
> +    let result = Async.waitForSyncCallback(cb);
> +
> +    return result && result.sourceURI &&
> +           result.sourceURI.host == ADDON_REPOSITORY_WHITELIST_HOSTNAME;

I'm not sure we care that the sourceURI is AMO do we? We probably care that both machines involved in the sync use the same addonrepository url.

@@ +519,5 @@
> +    }
> +
> +    this._log.debug("Manually obtaining install for " + addon.id);
> +
> +    // TODO do we need extra verification on sourceURI source?

Yes, you need to verify it is https: as you have no hash here

@@ +569,5 @@
> +      try {
> +        this._log.info("Installing " + addon.id);
> +
> +        let restart = addon.operationRequiringRestart &
> +          AddonManager.OP_NEEDS_RESTART_INSTALL;

This isn't going to be valid until after the xpi file has been downloaded and you can get it from the new addon object, in the onDownloadEnded method of an install listener for example

@@ +632,5 @@
> +            if (ourResult.errors.length > 0) {
> +              cb("1 or more add-ons failed to install", ourResult);
> +            } else {
> +              cb(null, ourResult);
> +            }

I wanted to make sure you were aware that at this point it is unlikely that any of the add-ons you tried to install have actually installed, they will all still be in the process of being downloaded.

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +1412,5 @@
> +   *
> +   * The object is copied from the source of truth, so it is safe to
> +   * mutate.
> +   */
> +  getAllStartupChanges: function AM_getAllStartupChanges() {

This seems unnecessary now?

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +7553,2 @@
>      XPIDatabase.setAddonSyncGUID(aAddon, val);
> +    }

No braces around this

Comment 28

[Gregory Szorc [:gps]]

Attachment: Add-ons engine and related changed, v3

Here is the latest code. I've made a lot of testing changes since the last patch.

rnewman: I would like detailed feedback on this since I hope to land it by next Friday. Obviously I need to separate things into patches. I'll be doing that for the next round.

I still haven't gone through looking for style nits, so you can omit those, since I'll theoretically find them when I go through.

The xpcshell tests all pass. I still need to expand test coverage there. And, there are a few tests commented out. TPS tests are still having issues. Those will be a large focus of mine next week.

Comment 29

[Gregory Szorc [:gps]]

(In reply to Dave Townsend (:Mossop) from comment #27)
> The nested event loops in services code makes me die a little inside.

They make me die a little too. Until we have async logic for the sync engines, our hands our tied and there is not much we can do.

> uninstall generally happens synchronously so I'm not sure why you're
> waiting, but if you want to wait wait till the appropriate methods are sent
> rather than guessing

Can you elaborate on "generally?" I noticed they were generally synchronous too. But, some tests were failing, so I've had to resort to waiting on callbacks in test code. I'll update code in the sync engine to wait as well.

> 
> @@ +493,5 @@
> > +    AddonRepository.getCachedAddonByID(addon.id, cb);
> > +    let result = Async.waitForSyncCallback(cb);
> > +
> > +    return result && result.sourceURI &&
> > +           result.sourceURI.host == ADDON_REPOSITORY_WHITELIST_HOSTNAME;
> 
> I'm not sure we care that the sourceURI is AMO do we? We probably care that
> both machines involved in the sync use the same addonrepository url.

I /think/ we do. In the security review, it was insisted that we only install add-ons from a trusted source. I suppose I could ask security if we could define this trusted source to be the value of a preference...

> 
> ::: toolkit/mozapps/extensions/AddonManager.jsm
> @@ +1412,5 @@
> > +   *
> > +   * The object is copied from the source of truth, so it is safe to
> > +   * mutate.
> > +   */
> > +  getAllStartupChanges: function AM_getAllStartupChanges() {
> 
> This seems unnecessary now?

Yup.

Comment 30

[Dave Townsend [:mossop]]

(In reply to Gregory Szorc [:gps] from comment #29)
> > uninstall generally happens synchronously so I'm not sure why you're
> > waiting, but if you want to wait wait till the appropriate methods are sent
> > rather than guessing
> 
> Can you elaborate on "generally?" I noticed they were generally synchronous
> too. But, some tests were failing, so I've had to resort to waiting on
> callbacks in test code. I'll update code in the sync engine to wait as well.

Right now they are always synchronous, at some point that might change though so if you relied on that behaviour now we'd have to make changes later. Of course that assumes we ever get around to doing proper async I/O there, and who knows when that might happen!

> > @@ +493,5 @@
> > > +    AddonRepository.getCachedAddonByID(addon.id, cb);
> > > +    let result = Async.waitForSyncCallback(cb);
> > > +
> > > +    return result && result.sourceURI &&
> > > +           result.sourceURI.host == ADDON_REPOSITORY_WHITELIST_HOSTNAME;
> > 
> > I'm not sure we care that the sourceURI is AMO do we? We probably care that
> > both machines involved in the sync use the same addonrepository url.
> 
> I /think/ we do. In the security review, it was insisted that we only
> install add-ons from a trusted source. I suppose I could ask security if we
> could define this trusted source to be the value of a preference...

I think that would be better, we try to avoid hardcoding anything to AMO as other applications don't necessarily use them. They're generally fine with prefs for this stuff.

Comment 31

[Dave Townsend [:mossop]]

(In reply to Dave Townsend (:Mossop) from comment #30)
> (In reply to Gregory Szorc [:gps] from comment #29)
> > > @@ +493,5 @@
> > > > +    AddonRepository.getCachedAddonByID(addon.id, cb);
> > > > +    let result = Async.waitForSyncCallback(cb);
> > > > +
> > > > +    return result && result.sourceURI &&
> > > > +           result.sourceURI.host == ADDON_REPOSITORY_WHITELIST_HOSTNAME;
> > > 
> > > I'm not sure we care that the sourceURI is AMO do we? We probably care that
> > > both machines involved in the sync use the same addonrepository url.
> > 
> > I /think/ we do. In the security review, it was insisted that we only
> > install add-ons from a trusted source. I suppose I could ask security if we
> > could define this trusted source to be the value of a preference...
> 
> I think that would be better, we try to avoid hardcoding anything to AMO as
> other applications don't necessarily use them. They're generally fine with
> prefs for this stuff.

Actually, I guess I wonder why we need to validate the host of the install URL when the XML came from our trusted source already. I forget why that was a requirement exactly.

Comment 32

[Richard Newman [:rnewman]]

Comment on attachment 578788 [details] [diff] [review]
Add-ons engine and related changed, v3

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

Oh jesus, that's a lot of touched files.

Great work. This is a sizable undertaking!

Meta-comments:

* This was 3,000 lines of changes. I might have missed something.
* I'd like jgriffin to give a once-over on the TPS changes and the tests. I've read them, they look good to me, but I'm a TPS noob.

gps, I know you haven't done a nits pass yet, but I've pointed out things as I spotted them. No harm in getting them out of my brain. No offense intended.

::: browser/app/profile/firefox.js
@@ +926,5 @@
>  #endif
>  
>  #ifdef MOZ_SERVICES_SYNC
>  // The sync engines to use.
> +pref("services.sync.registerEngines", "Bookmarks,Form,History,Password,Prefs,Tab,Addons");

See also mobile.js, which should at least be kept up-to-date even if it's no longer the default channel.

::: browser/base/content/syncSetup.xul
@@ +430,5 @@
>                        checked="true"/>
> +            <checkbox label="&engine.addons.label;"
> +                      accesskey="&engine.addons.accesskey;"
> +                      id="engine.addons"
> +                      checked="true"/>

If I recall correctly from product discussions yesterday, we're planning to ship with this feature disabled by default and non-discoverable. If that's the case, you'll want to hide this checkbox (or not include it at all).

::: browser/components/preferences/sync.xul
@@ +61,5 @@
>        <preference id="engine.history"   name="services.sync.engine.history"   type="bool"/>
>        <preference id="engine.tabs"      name="services.sync.engine.tabs"      type="bool"/>
>        <preference id="engine.prefs"     name="services.sync.engine.prefs"     type="bool"/>
>        <preference id="engine.passwords" name="services.sync.engine.passwords" type="bool"/>
> +      <preference id="engine.addons"    name="services.sync.engine.addons" type="bool"/>

Please align that "type" attribute!

@@ +156,5 @@
> +                <richlistitem>
> +                  <checkbox label="&engine.addons.label;"
> +                            accesskey="&engine.addons.accesskey;"
> +                            preference="engine.addons"/>
> +                </richlistitem>

Same checkbox remark applies here.

::: services/sync/modules/addonsreconciler.js
@@ +97,5 @@
> + * get called directly).
> + *
> + * When an add-on is installed, listeners are called in the following order:
> + *
> + *  IL.onInstallStarted, AL.onInstalling, IL.onInstallEnded, AL.onInstalled

What happens if an install fails? Does it appear as a cancel or something else?

Good to document this here, and make sure it's handled!

@@ +101,5 @@
> + *  IL.onInstallStarted, AL.onInstalling, IL.onInstallEnded, AL.onInstalled
> + *
> + * For non-restartless add-ons, an application restart may occur between
> + * IL.onInstallEnded and AL.onInstalled. Unfortunately, Sync likely will
> + * not be lodaded when AL.onInstalled is fired shortly after application

s/lodaded/loaded

@@ +104,5 @@
> + * IL.onInstallEnded and AL.onInstalled. Unfortunately, Sync likely will
> + * not be lodaded when AL.onInstalled is fired shortly after application
> + * start, so it won't see this event. Therefore, for add-ons requiring a
> + * restart, Sync treats the IL.onInstallEnded event as good enough to
> + * denote an install. For restartless add-ons, Sync assumes AL.onInstalled

s/denote/indicate

@@ +105,5 @@
> + * not be lodaded when AL.onInstalled is fired shortly after application
> + * start, so it won't see this event. Therefore, for add-ons requiring a
> + * restart, Sync treats the IL.onInstallEnded event as good enough to
> + * denote an install. For restartless add-ons, Sync assumes AL.onInstalled
> + * will follow shortly after IL.onInstallEnded and thus is ignores

s/thus is/thus it

@@ +161,5 @@
> +
> +  let us = this;
> +  Svc.Obs.add("xpcom-shutdown", function() {
> +    us.stopListening();
> +  });

Svc.Obs.add("xpcom-shutdown", this.stopListening.bind(this));

@@ +218,5 @@
> +        }
> +
> +        for each (let change in json.changes) {
> +          this._changes.push([new Date(change[0]), change[1], change[2]]);
> +        }

Destructuring ftw:

  for each (let [time, change, id] in json.changes) {
    this._changes.push([new Date(time), change, id]);
  }

A better alternative might be to just use timestamps instead of Dates, which avoids you having to do any processing at all. The rest of Sync uses timestamps.

@@ +221,5 @@
> +          this._changes.push([new Date(change[0]), change[1], change[2]]);
> +        }
> +
> +        if (callback) {
> +          callback(false, true);

I generally prefer 'null' instead of 'false' for an error argument that's not an error. false implies that it's a boolean flag, which it's not.

@@ +227,5 @@
> +      } else {
> +        if (callback) {
> +          callback(false, false);
> +        }
> +      }

Let's invert these two main clauses: "return early":

  if (!json) {
    if (callback) {
      callback(null, false);
    }
    return;
  }
  this._addons = …

@@ +248,5 @@
> +    for (let [id, record] in Iterator(this._addons)) {
> +      state[id] = {};
> +      for (let [k, v] in Iterator(record)) {
> +        if (k == "modified") {
> +          state[id][k] = v.getTime();

See above.

@@ +257,5 @@
> +      }
> +    }
> +
> +    for each (let change in this._changes) {
> +      state.changes.push([change[0].getTime(), change[1], change[2]]);

… and again.

@@ +260,5 @@
> +    for each (let change in this._changes) {
> +      state.changes.push([change[0].getTime(), change[1], change[2]]);
> +    }
> +
> +    Utils.jsonSave(path || DEFAULT_STATE_FILE, this, state, callback);

Let's use that same idiom as above:

  let file = path || DEFAULT_STATE_FILE;

at the top of the method. Or:

  saveJSON: function saveJSON(path, state, callback) {
    Utils.jsonSave(path || DEFAULT_STATE_FILE, this, state, callback);
  },
  loadJSON: function loadJSON(path, state, callback) {
    Utils.jsonLoad(path || DEFAULT_STATE_FILE, this, state, callback);
  },

which allows you to just do

  this.saveJSON(path, state, callback);

@@ +273,5 @@
> +   * object reflecting the current state of the add-on at the time of the
> +   * change.
> +   */
> +  addChangeListener: function addChangeListener(listener) {
> +    if (!this._listeners.some(function(i) { return i == listener; })) {

if (this._listeners.indexOf(listener) == -1) {

? No point allocating a new closure here.

@@ +301,5 @@
> +   * The reconciler should always be listening. This should only be called when
> +   * the instance is being destroyed.
> +   */
> +  stopListening: function stopListening() {
> +    if (this._listening) {

Let's add a log statement here, if it's such a meaningful event.

@@ +322,5 @@
> +        this.rectifyStateFromAddon(addon);
> +      }
> +
> +      // Look for locally-defined add-ons that don't exist any more and update
> +      // their record

"that no longer exist". And let's have a period at the end :)

@@ +414,5 @@
> +   *   date - Date instance representing when the change occurred
> +   *   change_type - One of CHANGE_* constants.
> +   */
> +  getChangesSinceDate: function getChangesSinceDate(date) {
> +    let length = this._changes.length;

I believe, but haven't checked, that you can simply do

  let length = this._changes.length;
  for (let i = 0; i < length; i++) {
    if (this._changes[i].entry[0] >= date) {
      return this._change.slice(i);
    }
  }
  return [];

@@ +442,5 @@
> +      if (entry[0] < date) {
> +        delete this._changes[0];
> +      } else {
> +        return;
> +      }

Fun little function!

If you invert these clauses, you get

  while (this._changes.length > 0) {
    if (this._changes[0][0] >= date) {
      return;
    }
    delete this._changes[0];
  }

which is even neater.

I'm not concerned about the perf implications of always deleting element 0 (and thus renumbering the rest of the array).

@@ +494,5 @@
> +      // followed by another event and that this follow-up event is the most
> +      // appropriate to react to. Currently we ignore onEnabling, onDisabling,
> +      // and onUninstalling for non-restartless add-ons.
> +      if (requiresRestart === false) {
> +        this._log.debug("Ignoring notification because restartless");

this._log.debug("Ignoring " + action + " for restartless addon.");

::: services/sync/modules/engines/addons.js
@@ +552,5 @@
> +   * Installs an add-on from an AddonSearchResult instance.
> +   *
> +   * When complete it calls a callback with 2 arguments, error and result.
> +   *
> +   * If error is falesy, result is an object. If error is truthy, result is

s/falesy/falsy

@@ +583,5 @@
> +      try {
> +        this._log.info("Installing " + addon.id);
> +
> +        let restart = addon.operationRequiringRestart &
> +          AddonManager.OP_NEEDS_RESTART_INSTALL;

Align.

@@ +616,5 @@
> +  /**
> +   * Uninstalls the Addon instance and invoke a callback when it is done.
> +   *
> +   * @param addon
> +   *        Addon instance ot uninstall.

s/ot/to

@@ +696,5 @@
> +
> +      }.bind(this),
> +
> +      searchFailed: function searchFailed() {
> +        cb("AddonRepository search failed", null);

Please wrap that with a SyncError.

::: services/sync/modules/util.js
@@ +194,5 @@
>      return btoa(bytes).replace('+', '-', 'g').replace('/', '_', 'g');
>    },
>  
> +  decodeBase64url: function decodeBase64url(data) {
> +    return atob(data.replace('-', '+', 'g').replace('_', '/', 'g'));

Please use safeAtoB instead. (It's in this file.)

::: services/sync/services-sync.js
@@ +16,5 @@
>  pref("services.sync.scheduler.idleTime",             300);   // 5 minutes
>  
>  pref("services.sync.errorhandler.networkFailureReportTimeout", 604800); // 1 week
>  
> +pref("services.sync.engine.addons", true);

Should this be preffed on by default?

::: services/sync/tests/tps/test_addon_restartless_xpi.js
@@ +62,5 @@
> +Phase("phase08", [
> +  [Addons.verify, [id], STATE_ENABLED],
> +  [Sync],
> +  [Addons.verifyNot, [id]]
> +]);

I'd like to see an addition here: disable the add-on in one profile, and uninstall it in the other, then sync.

::: services/sync/tests/unit/head_helpers.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";

This has massive knock-on effects. Please remove it for now. Adding it requires manually validating that our entire test suite now passes or fails for the correct reasons, because JS semantics are altered.

Only use strict in your new files.

@@ +86,5 @@
> + * interface with the AddonManager. It should only be called once, or the
> + * universe will end.
> + */
> +function loadAddonTestFunctions() {
> +  const path = "../../../../toolkit/mozapps/extensions/test/xpcshell/head_addons.js";

O-O

Can we at least lift and share the root of this between this function and getAddonInstall?

And make sure all this works when you use various incantations of make -C?

::: services/sync/tests/unit/head_http_server.js
@@ +4,5 @@
>  Cu.import("resource://services-sync/log4moz.js");
>  const SYNC_HTTP_LOGGER = "Sync.Test.Server";
>  const SYNC_API_VERSION = "1.1";
>  
> +Cu.import("resource://services-sync/engines.js");

What's this for?

::: services/sync/tests/unit/test_addons_store.js
@@ +44,5 @@
> +  try {
> +    let server = new nsHttpServer();
> +
> +    let install1_xpi = "../../../../toolkit/mozapps/extensions/test/xpcshell/" +
> +                       "addons/test_install1.xpi";

Yup, lift this path out and make it an accessor/generator function in the helpers: mozAppsXPCShellPath("addons/test_install1.xpi") or something.

@@ +273,5 @@
> +
> +  run_next_test();
> +});
> +
> +/*

?

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +822,5 @@
>    }
>  
>    addon.applyBackgroundUpdates = AddonManager.AUTOUPDATE_DEFAULT;
>  
> +  // Generate random GUID used for Sync.

Please put a pointer back to Utils.makeGUID.

Comment 33

[Gregory Szorc [:gps]]

I heard about shipping disabled by default. But, the non-discoverable bit as a certainty is new to me. We previously discussed shipping non-discoverable if we felt the feature wasn't polished. I wasn't privy to the meeting Friday. Can someone clarify the decision and explain the rationale?

Comment 34

[Richard Newman [:rnewman]]

I had a long comment, but Fennec eated it.

I don't know if "certainty" is the right word; I'm only commenting on hearsay. (One more reason to hate Friday meetings: not everybody is working on the weekend!)

It makes some amount of sense to me: if a client-side feature is incomplete enough to ship disabled, then non-expert users shouldn't have an easy way to turn it on, because we don't have a good channel in which to explain why they shouldn't. But this is a product decision, unfortunately.

Not a difficult code change to make, at least, once we know what we're shipping.

Comment 35

[Philipp von Weitershausen [:philikon]]

The discoverability bit is new to me, too. I talked to Ally on Friday. As far as default settings go, I understood that we'd disable it for existing users but enable it for new users. This is certainly the behaviour I would because it follows the rule of least surprises).

Maybe Ally can tie the UX + PM pieces together to get clarity here. It seems like an utter waste of engineering resources to make a P1 feature undiscoverable. (Also, Sync hasn't had a discoverable new feature in, like, forever, not counting the improvements we made to the setup UI which aren't so much new but just an improvement.)

Comment 36

[Richard Newman [:rnewman]]

(In reply to Philipp von Weitershausen [:philikon] from comment #35)
> It seems
> like an utter waste of engineering resources to make a P1 feature
> undiscoverable.

Yeah, tell me about it :)

Comment 37

[Justin Scott [:fligtar]]

(In reply to Gregory Szorc [:gps] from comment #33)
> I wasn't privy to the meeting Friday. Can someone clarify the decision and explain the rationale?

Hi, I'm the cause of the consternation last week. I'm the PM for Add-ons and I learned about this feature on Thursday when Mossop briefly mentioned he was reviewing a patch for it.

As currently defined/implemented, add-on sync will uninstall add-ons from all devices when uninstalled on one device, and disable add-ons on all devices when disabled on another. It's not possible for me to have differing add-on states on my devices unless I turn off add-on sync.

Some users will want this true sync. I think that most will not[1]. Add-ons are not like history, bookmarks, and passwords; by their nature, people use different add-ons on different devices. The easiest example is Firebug, which I want to have on my work machine and do *not* want on my home machine, especially given performance impacts.

If we were to land this as implemented and turn it on by default for Sync users, I think it would cause a lot of confusion and anger. I saw the proposal for this feature a year ago and suggested that instead, we have a section of the Add-ons Manager that showed "Add-ons from my other devices" providing easy installation of add-ons I know I want, which I think is the primary benefit of add-on sync. We could then have a way to turn on full syncing for those that wanted it. This feedback was apparently lost and I didn't hear about the feature again until a few days ago.

On Friday, mconnor suggested that we keep add-on sync as currently defined but have it off by default, and provide the discovery view for all sync users, even those who have add-on sync off. In that area we can have an easy way to turn on add-on sync for those that do want that. This way we'll get the best of both worlds.

--
[1]: As an additional data point, I ran a poll on the Add-ons Blog about this and most users said they wanted to choose which add-ons to enable on other devices themselves: http://blog.mozilla.com/addons/2011/02/23/poll-how-would-you-use-firefox-add-on-sync/

Comment 38

[Ed Lee :Mardak]

(In reply to Justin Scott [:fligtar] from comment #37)
> As currently defined/implemented, add-on sync will uninstall add-ons from
> all devices when uninstalled on one device, and disable add-ons on all
> devices when disabled on another.
Is the main concern that uninstall and disabled state get synced? As opposed to add-ons automatically getting installed?

Could add-on sync just be implemented to be only additive?

New add-ons installed from one installation will cause it to be installed on other locations. Disabled add-ons will sync and installed as disabled.

Trickiest would be uninstalling add-ons, but perhaps that could just sync as a disabled add-on. Individual machines that explicitly had an add-on uninstalled could remember that it was uninstalled and not try syncing a disabled add-on.

Comment 39

[Ptyxs]

Justin Scott's remarks sound to my ear so much of the obvious for most users that I should be extremely surprised to discover they were not discussed so far.
A mere choice sync add-ons/don't sync add-ons is certainly not what most users need.

Comment 40

[Tiger.711]

It will look like a checkbox "synchronize this add-on" under each addon, if synchronization is enabled?

Comment 41

[Richard Newman [:rnewman]]

(In reply to Tiger.711 from comment #40)
> It will look like a checkbox "synchronize this add-on" under each addon, if
> synchronization is enabled?

Not in this current implementation, no.

Comment 42

[Richard Newman [:rnewman]]

(In reply to Justin Scott [:fligtar] from comment #37)

> On Friday, mconnor suggested that we keep add-on sync as currently defined
> but have it off by default, and provide the discovery view for all sync
> users, even those who have add-on sync off. In that area we can have an easy
> way to turn on add-on sync for those that do want that. This way we'll get
> the best of both worlds.

This sounds like a reasonable compromise to me.

(In reply to Ptyxs from comment #39)
> Justin Scott's remarks sound to my ear so much of the obvious for most users
> that I should be extremely surprised to discover they were not discussed so
> far.
> A mere choice sync add-ons/don't sync add-ons is certainly not what most
> users need.

To be fair, both UX and product have been involved in this project since its inception (though apparently to varying degrees!). Furthermore, the guiding principle of Sync is "make all your devices the same" -- we also get occasional complaints from sophisticated users that they only want to sync some portion of their bookmarks, and we have no intention of supporting that, so it's by no means obvious what the 'correct' expected behavior is.

You're falling into the fallacy of assuming that you are representative of most users. The very fact that you're posting on Bugzilla means that you're not.

Comment 43

[Gregory Szorc [:gps]]

OK, the following is my personal plan of action.

I need to land something on Friday for it to make the final services-central merge train for Firefox 11. I will do everything in my power to land something by Friday. Every missed train is another 6 weeks where Firefox doesn't have this feature. And, every missed train is an opportunity for code to bit rot and create more work for engineering.

Personally, as long as the core add-ons engine and related tests land, I am happy. If the engine needs to be disabled by default or non-discoverable, I understand there are concerns there and these may delay the feature being visible. Luckily, these changes are very trivial to perform and I can make them easily.

As it currently stands, because of bug 608231, new Sync engines are disabled by default for existing sync users. But, for new users, the engine would be enabled. I'll plan on not working around this with this land unless told otherwise. If we remove the checkbox for enabling the engine from the preferences pane, obviously the engine will need to be disabled by default everywhere and the only way to enable would be through about:prefs. I currently plan on including the checkbox.

Currently, everything is unconditionally synced everywhere (to devices of the same application ID): Installs, uninstalls, user disables, and user enables. I don't plan to depart from this previously agreed to plan unless told otherwise. To those deciding the feature, anything is on the table, but complexity may delay feature launch more than it already is. As long as UX, product, and engineering a agree, I don't really care what specifically it is.

There needs to be discussion on this feature this week, I understand. But, I just ask that it not occur in Bugzilla because 1) Bugzilla is a horrible forum 2) it will distract from the review process already underway in this bug. Please start a mailing list thread elsewhere and make a note of it in a subsequent comment.

tl;dr I'm implementing the Connor Plan (as I understand it) unless told otherwise and will land it by EOD Friday. Discuss in a public venue that isn't this bug and post decisions/changes back to this bug.

Comment 44

[Gregory Szorc [:gps]]

(In reply to Richard Newman [:rnewman] from comment #32)
> @@ +218,5 @@
> A better alternative might be to just use timestamps instead of Dates, which
> avoids you having to do any processing at all. The rest of Sync uses
> timestamps.

I disagree. I feel that if a language provides a mechanism to do something, you should use it. In this case, it makes normalization much simpler. While Sync uses numbers as timestamps, it is inconsistent in how it handles them. It uses seconds in places, milliseconds in others (all since UNIX epoch). Some places it rounds, some places it is allowing decimals. I decided to avoid this mess in this module and normalize everything to Date instances. The callers can worry about converting to the appropriate representation for their local needs. Keep in mind that AddonsReconciler is really a utility module and not Sync specific: it just happens to live in Sync's code base ATM.

> 
> ::: services/sync/services-sync.js
> @@ +16,5 @@
> >  pref("services.sync.scheduler.idleTime",             300);   // 5 minutes
> >  
> >  pref("services.sync.errorhandler.networkFailureReportTimeout", 604800); // 1 week
> >  
> > +pref("services.sync.engine.addons", true);
> 
> Should this be preffed on by default?

I think so, yes. Existing users is off by default (due to bug 608231). New users the engine is enabled.

> 
> ::: services/sync/tests/tps/test_addon_restartless_xpi.js
> @@ +62,5 @@
> > +Phase("phase08", [
> > +  [Addons.verify, [id], STATE_ENABLED],
> > +  [Sync],
> > +  [Addons.verifyNot, [id]]
> > +]);

> I'd like to see an addition here: disable the add-on in one profile, and
> uninstall it in the other, then sync.

I'll do this shortly, along with a number of other more advanced TPS tests. It requires getting the TPS tests to work, grrrr.

> > +function loadAddonTestFunctions() {
> > +  const path = "../../../../toolkit/mozapps/extensions/test/xpcshell/head_addons.js";

> And make sure all this works when you use various incantations of make -C?

AFAICT xpcshell or the make runner does a chdir when invoking tests, so pwd doesn't come into play.


I've incorporated your other comments and have pushed the latest code to the Git branch. I'll be working on wrangling the TPS tests into passing and filling out more xpcshell tests. Hopefully I'll have a r? Wednesday (in separate patches, of course).

Comment 45

[Gregory Szorc [:gps]]

We actually want the engine to be disabled by default for existing Sync users, so I'm removing the bug dependency. I'll note in that other bug that when it is addressed, we'll need a story to ensure the add-ons engine isn't enabled for existing users.

Comment 46

[Gregory Szorc [:gps]]

Attachment: Part 0: Rename global variable to avoid collision with add-on tests

This is a trivial part 0 to rename a global variable in the xpcshell tests. In a future commit, a file from the add-on xpcshell tests is imported which also defines this variable. It was easier to rename the variable in sync land than in AddonManager.

I also made a modification to the XUL app initialization which is required to support the add-on manager. This should arguably be part of a later commit, but it doesn't hurt to do it now.

Comment 47

[Gregory Szorc [:gps]]

Attachment: Part 1: Add-ons engine, v1

Here is the core add-ons engine and associated xpcshell tests!

It is missing the following, which will be provided by subsequent patches in this and other bugs:

* TPS tests
* Application integration

I'm still debugging TPS tests. And, application integration may depend on review feedback, so I'm holding back those patches.

Mossop: I have no clue why the enabling/disabling listeners aren't firing. I must be doing something stupid. For now, I've commented out functionality relying on them. The functionality isn't strictly required, so it hopefully won't be a show stopper.

Try in progress at http://tbpl.mozilla.org/?tree=Try&rev=4595d3849c63

Comment 48

[Gregory Szorc [:gps]]

I'm requesting an additional security review for the behavior of the AddonsReconciler. This class effectively maintains a shallow copy of some AddonManager state in a JSON file in the profile directory. This could have privacy implications. I want to be sure the security team is on-board with this.

Comment 49

[Richard Newman [:rnewman]]

Comment on attachment 579386 [details] [diff] [review]
Part 0: Rename global variable to avoid collision with add-on tests

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

You might want to move invalidateCachesOnRestart into the correct patch…

::: services/sync/tests/unit/head_appinfo.js
@@ +26,5 @@
>    logConsoleErrors: true,
>    OS: OS,
>    XPCOMABI: "noarch-spidermonkey",
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIXULAppInfo, Ci.nsIXULRuntime]),
> +  invalidateCachesOnRestart: function invalidateCachesOnRestart() { }

Not quite just a rename, mm? *raises eyebrows*

Comment 50

[Mozilla RelEng Bot]

Try run for 4595d3849c63 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4595d3849c63
Results (out of 107 total builds):
    exception: 3
    success: 97
    warnings: 6
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4595d3849c63

Comment 51

[Mozilla RelEng Bot]

Try run for 4595d3849c63 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4595d3849c63
Results (out of 107 total builds):
    exception: 3
    success: 97
    warnings: 6
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4595d3849c63

Comment 52

[Gregory Szorc [:gps]]

Part 0 (just the variable rename) pushed to s-c: https://hg.mozilla.org/services/services-central/rev/6f6e4ac35d35

Comment 53

[Dave Townsend [:mossop]]

Comment on attachment 579516 [details] [diff] [review]
Part 1: Add-ons engine, v1

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

Looking good

::: services/sync/modules/addonsreconciler.js
@@ +333,5 @@
> +  stopListening: function stopListening() {
> +    if (this._listening) {
> +      this._log.debug("Stopping listening and removing AddonManager listeners.");
> +      AddonManager.removeInstallListener(this);
> +      AddonManager.removeAddonListener(this);

There isn't actually any need for these, when the add-ons manager is shutdown (which could happen before this as you're calling from the same observer notification) it throws away all listeners.

::: services/sync/modules/engines/addons.js
@@ +321,5 @@
> +  create: function create(record) {
> +    // Ideally, we'd set syncGUID and userDisabled on install. For now, we
> +    // make the changes post-installation.
> +    // TODO Set syncGUID and userDisabled in one step, during install.
> +

Can't see where this method is called from, but is it possible that at this point the add-on is already installed (because they installed it on two separate machines manually so they ended up with different syncGUIDs)? In which case you should just use getAddonByID to check before having to install again presumably.

@@ +561,5 @@
> +
> +    if (addon.foreignInstall) {
> +      this._log.debug(addon.id + " not syncable: is foreign install.");
> +      return false;
> +    }

This is not necessarily true. foreignInstall would be true if a user downloaded the add-on from AMO and extracted it to the profile manually for example. I'm fine with ignoring that case, might be worth a comment though.

Comment 54

[Gregory Szorc [:gps]]

Attachment: Add-ons engine, v2

Here is the latest version of the addons engine, support files, and tests. Most of the changes are related to bug fixes for issues discovered through TPS tests.

Between this patch and the ones in bug 704462 and bug 708965, all xpcshell and TPS tests pass and add-on sync works as currently specified AFAIK. I'm hoping to land this in the next services train so it can make Fx11.

Comment 55

[Mike Connor [:mconnor]]

Comment on attachment 580328 [details] [diff] [review]
Add-ons engine, v2

code usage, etc all look good, and the test coverage is solid enough.

Followups:

* Trace logging.  Oh god, let's add some trace logging. We'll be grateful later.  We never can be too verbose, in trace  mode.  Let's get this in ASAP, as debugging with users will be dramatically easier if we can do this.

* Revisit pref-based whitelist and .source == "amo" model, switch to using nsIPermissionManager for xpinstall, since that's the "this domain is allowed to install add-ons"  pref in the UI... and users should be able to maek their own choices here.  Doesn't block, but will go a long way to sorting out the "what about the rest of the ecosystem?"

Comment 56

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 580328 [details] [diff] [review]
Add-ons engine, v2

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

::: services/sync/modules/engines/addons.js
@@ +599,5 @@
> +   * @param  uri
> +   *         nsIURI instance to validate
> +   * @return bool
> +   */
> +  isSourceURITrusted: function isSourceURITrusted(uri) {

What mconnor said.

@@ +880,5 @@
> +    this._log.info("Updating userDisabled flag: " + addon.id + " -> " + value);
> +    addon.userDisabled = !!value;
> +
> +    if (!addon.appDisabled) {
> +      callback(null, addon);

Shouldn't the callback be called here if appDisabled is true, rather than false?

Comment 57

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Er, wtf flag changes. That r+ wipe was unintended :\

Comment 58

[Richard Newman [:rnewman]]

(In reply to Blair McBride (:Unfocused) from comment #57)
> Er, wtf flag changes. That r+ wipe was unintended :\

Oh, Bugzilla and your dastardly form fields :D

Comment 59

[Richard Newman [:rnewman]]

(In reply to Blair McBride (:Unfocused) from comment #56)

I'm sure gps will answer this when he wakes up, but in the mean time:

> > +    if (!addon.appDisabled) {
> > +      callback(null, addon);
> 
> Shouldn't the callback be called here if appDisabled is true, rather than
> false?

Not according to the comment above:

+    // The add-on listeners are only fired if the add-on is active. If not, the
+    // change is silently updated and made active when/if the add-on is active.

I presume that !addon.appDisabled => active.

Comment 60

[Mike Connor [:mconnor]]

Comment on attachment 580328 [details] [diff] [review]
Add-ons engine, v2

Fixing the flag.  *shakes fist at blair*

Comment 61

[Gregory Szorc [:gps]]

Attachment: Add-ons engine, v3

When reconciling my local code vs the submitted patch, I found a few differences:

* AddonsReconciler is now more aggressive about loading state from the file. It also does that transparently from the perspective of the caller.
* Detection of pending add-on installation in reconciler
* More logging in reconciler
* store.applyIncoming doesn't drop records if .deleted is true
* store.itemExists() returns true even if an add-on was uninstalled (fix subtle reconciling bug)
* store.changeItemID() always updates the reconciler
* Change throw in install path to invoke callback with error instead
* Make the result of the install API much richer (contains AddonInstall objects)
* Handle onOperationCancelled better

I didn't realize I had made so many of these small changes after submitting the original patch! Despite the long list, the intradiff isn't too bad.

Comment 62

[Gregory Szorc [:gps]]

Attachment: Part 2: UX for add-on sync

This is the UX and browser integration bits for add-on sync. It adds the checkbox for enabled engines and registers the new engine with the desktop and XUL mobile browsers.

Comment 63

[Gregory Szorc [:gps]]

Pushed to s-c:

https://hg.mozilla.org/services/services-central/rev/33278465af7c
https://hg.mozilla.org/services/services-central/rev/347c55a7b38b

That concludes the patches for this bug. Still waiting on TPS tests and some minor follow-ups. This will be on the next services-central train and should make it in Firefox 11.

Comment 64

[Tracy Walker [:tracy]]

verified on desktop.  will need a full test pass on mobile when a supporting version of mobile is available.

Comment 65

[Gregory Szorc [:gps]]

Pushed to m-c:

https://hg.mozilla.org/mozilla-central/rev/6f6e4ac35d35
https://hg.mozilla.org/mozilla-central/rev/33278465af7c
https://hg.mozilla.org/mozilla-central/rev/347c55a7b38b

Comment 66

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (Windows NT 6.1; rv:11.0a1) Gecko/20111220 Firefox/11.0a1

Scenario:
1. Install 2 add-ons on device A (Addblock and NoScript)
2. Sync both devices => both add-ons are added to device B but restart is needed to finish the install
3. On device B do not restart Firefox
4. On device A, remove an add-on (remove Adblock)and restart Firefox
5. Sync both devices => both add-ons still appear and restart is needed to finish install
6. Restart Firefox on device B

Actual result: The removed add-on (Addblock) is installed on device B and appears in Addons Manager

Expected result: The removed add-on (Addblock) should be removed and no longer appear on device B

Comment 67

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: logs from both devices used in scenario form comment #66

Comment 68

[dindog]

I do a little test, and not sure whether I should file a new bug.

Here are two issue about addon sync:
1. Addons' Disabled/Enabled state not correct after sync...
2. Addons' version also not the original sync machine's version. like firebug 1.9a --> firebug 1.8, Tab Utilities 1.2pre17 to TU 1.1

Comment 69

[Mihaela Velimiroviciu (:mihaelav)]

Bug #712328 was logged for issue in comment #66

Comment 70

[Tracy Walker [:tracy]]

dingdog,

please file a new bug for each issue. attach associated sync-logs to each.

To set up sync logging, see:
https://philikon.wordpress.com/2011/06/13/how-to-file-a-good-sync-bug/

Comment 71

[dindog]

(In reply to Tracy Walker [:tracy] from comment #70)
> dingdog,
> 
> please file a new bug for each issue. attach associated sync-logs to each.
> 
> To set up sync logging, see:
> https://philikon.wordpress.com/2011/06/13/how-to-file-a-good-sync-bug/

Filed, bug 712542, bug 712544

Comment 72

[Jens Hatlak (:InvisibleSmiley)]

While working on porting the UI changes to SM I found that you guys forgot syncQuota.properties. I'll leave it to you to file a bug accordingly since I don't want to receive the bugmail.

(Oh and BTW the bug reference of mozilla-central rev 347c55a7b38b was wrong.)

Comment 73

[Tracy Walker [:tracy]]

verified with m-c builds of 20111223

any issues found with add-ons sync going forward should be filed as new bugs.

Comment 74

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

removing secr:geekboy so we can re-triage this and get it reassigned.

Comment 75

[David Chan [:dchan]]

The security review was completed for Fx 11
https://wiki.mozilla.org/Security/Reviews/Firefox/AddOnSync