A profile in my house if failing to Sync addons on every sync: > 1464053967594 Sync.Engine.Addons DEBUG Switching local ID to incoming: AVOHqDrjCmgy -> EdsvHAQrCuNN > 1464053967600 Sync.AddonsReconciler INFO Saving reconciler state to file: addonsreconciler > 1464053967631 Sync.Engine.Addons WARN Failed to reconcile incoming record EdsvHAQrCuNN: Error: Addon sync GUID conflict for addon app-system-addons:loop@mozilla.org: app-system-defaults:loop@mozilla.org already has GUID EdsvHAQrCuNN (resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:1389:13) JS Stack trace: this.XPIDatabase.setAddonSyncGUID@XPIProviderUtils.js:1389:13 < AddonWrapper.prototype.syncGUID@XPIProvider.jsm:7034:7 < changeItemID@addons.js:442:5 < SyncEngine.prototype._reconcile@engines.js:1352:9 < SyncEngine.prototype._processIncoming/newitems.recordHandler@engines.js:1096:23 < Collection.prototype.recordHandler/this._onProgress@record.js:623:9 < Channel_onDataAvail@resource.js:554:7 < waitForSyncCallback@async.js:98:7 < Res__request@resource.js:398:14 < Res_get@resource.js:425:12 < SyncEngine.prototype._processIncoming@engines.js:1185:18 < SyncEngine.prototype._sync@engines.js:1551:7 < WrappedNotify@util.js:146:21 < Engine.prototype.sync@engines.js:670:5 < _syncEngine@enginesync.js:213:7 < sync@enginesync.js:163:15 < onNotify@service.js:1262:7 < WrappedNotify@util.js:146:21 < WrappedLock@util.js:101:16 < _lockedSync@service.js:1252:12 < sync/<@service.js:1244:14 < WrappedCatch@util.js:75:16 < sync@service.js:1232:5 > 1464053967633 Sync.Engine.Addons DEBUG Records that failed to apply: EdsvHAQrCuNN > 1464053967633 Sync.Engine.Addons INFO Records: 0 applied, 0 successfully, 1 failed to apply, 0 newly failed to apply, 0 reconciled. > 1464053967651 Sync.Engine.Addons ERROR Failed to set previousFailed: null I haven't dug any further, but do note that I only noticed this due to error logs being written, which is the last line above - but that's actually a bug that we fixed in bug 1262329 (there was no error setting previousFailed) - so we should also consider having the WARN with the specific error message log at the ERROR level so this kind of problem always creates error logs. Flagging as P1 because this relates to a system addon rather than one the user chose to install, so it's reasonable to speculate many users have this problem.

Oh, FTR, the log above is from: 1464053965924 Sync.Service DEBUG User-Agent: Firefox/47.0 FxSync/1.49.0.20160512003946. Which is on the beta channel. The second device associated with the account is on the release channel and currently on 45 (an update is pending), and that device is not reporting any addon related problems. It wouldn't surprise me to find that this mismatch will be necessary to reproduce.

Review commit: https://reviewboard.mozilla.org/r/57522/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/57522/

Review commit: https://reviewboard.mozilla.org/r/57524/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/57524/

Review commit: https://reviewboard.mozilla.org/r/57526/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/57526/

Comment on attachment 8759518 [details] Bug 1275139 (part 1) - kill ignoreRepositoryChecking pref, replacing it with AddonRepository.cache. Hey Richard, Sorry to pick on you again, but it seems you did most of the addon work. This patch kills the ignoreRepositoryChecking preference as it is misleading. One of the main things it does is to prevent the call to AddonRepository.getCachedAddonByID() to check the addon. However, this call will *always* return null when AddonRepository's cache is disabled - which it is in all of our test suites (via a extensions.getAddons.cache.enabled preference). Some of the tests set the ignoreRepositoryChecking preference, but in reality, every test would need that if it turned out they ended up trying to call getCachedAddonByID() - and a later version of this patch means the "process incoming" patch also hits this. Interestingly, this found what I consider a read bug in the test suite - test_create_bad_install() in test_addons_store is supposedly trying to test that we elegantly skip an addon where the .xpi is a 404 - however, because that test *did not* set the ignoreRepositoryChecking pref, the addon failed to install due to the http:// URL not being trusted - ie, there was no attempt at all to download the .xpi. Sadly fixing that looks a little painful - it is difficult to differentiate a 404 from any other network error - most network errors probably *should* end up in .failed. However, note that these changes don't cause a change in behaviour - missing .xpi file currently *do* end up in .failed, the test just wasn't checking what it though it was. So I intend to leave that test commented out and open a new bug to try and do something sane with the 404.

Comment on attachment 8759519 [details] Bug 1275139 (part 2) - change tests to demonstrate the problems with system addons. These are test changes to demonstrate what currently happens with addons - currently we both upload system addons to the server, and also process incoming records from the server. If that incoming record (say) has the addon as being disabled, we will happily disable the system addon, which we shouldn't do (the addon manager doesn't expose a way to disable them). The specific problem in this case seems to be loop - it was initially released as a "real" addon (in which case it synced as normal) but then was upgraded to a "system" addon while keeping the same ID. Thus it's somewhat expected that this addon appears on the server (from when it was a real addon), but we should never apply it locally.

Comment on attachment 8759520 [details] Bug 1275139 (part 3) - don't Sync system addons, nor apply them when incoming. This is the actual fix - there are 2 parts: * isAddonSyncable() now returns false for system and hidden addons. If you give f+, I'll chase up with someone on the addons team to ensure this is suitable (ie, maybe the condition for "don't touch this" should be broader) * We now check an incoming record against isAddonSyncable - if it returns false we just ignore the incoming record completely.

Comment on attachment 8759520 [details] Bug 1275139 (part 3) - don't Sync system addons, nor apply them when incoming. This patch is wrong - we don't want to check the cache when we are checking an incoming record. I think I'm probably going to need a new param to isAddonSyncable() to optionally skip the repository even when it is enabled. I'll put a new patch up after I get feedback on the first couple of patches.

(In reply to Mark Hammond [:markh] from comment #5) > Sorry to pick on you again, but it seems you did most of the addon work. That was gps :)

https://reviewboard.mozilla.org/r/57522/#review54832 I'm pretty sure I don't understand this well enough to review it correctly. Mossop, maybe?

Comment on attachment 8759518 [details] Bug 1275139 (part 1) - kill ignoreRepositoryChecking pref, replacing it with AddonRepository.cache. (In reply to Richard Newman [:rnewman] from comment #10) > https://reviewboard.mozilla.org/r/57522/#review54832 > > I'm pretty sure I don't understand this well enough to review it correctly. > Mossop, maybe? Mossop isn't accepting requests, but I think Rob works on addons and might be able to help with "part 3" where I need an authoritative check whether an addon is syncable or not... Hey Rob, are you able to help here, or suggest someone else who can? The tl;dr is that Sync has its own preference for disabling any checks made to the AddonRespository cache. However, that cache is disabled via its own preference in all test environments, which leads to the odd situation where Sync *thinks* it is checking the cache, but in reality the cache is disabled. I'm proposing that Sync *always* checks the repository cache whenever the cache is enabled, period. It's not particularly critical, it's just a foot-gun and the cause of at least 1 test not checking what it things it's checking.

Comment on attachment 8759520 [details] Bug 1275139 (part 3) - don't Sync system addons, nor apply them when incoming. https://reviewboard.mozilla.org/r/57526/#review54986 ::: services/sync/modules/addonsreconciler.js:438 (Diff revision 1) > installed: true, > modified: now, > type: addon.type, > scope: addon.scope, > - foreignInstall: addon.foreignInstall > + foreignInstall: addon.foreignInstall, > + shouldIgnore: addon.isSystem || addon.isHidden, The getter is `addon.hidden` not `isHidden` (unfortunately for consistency with other getters on this object): https://dxr.mozilla.org/mozilla-central/rev/3e8ee3599a67edd971770af4982ad4b0fe77f073/toolkit/mozapps/extensions/internal/XPIProvider.jsm#7262 `hidden` actually covers system add-on (as well as temporary add-ons) so you could just use this and not `isSystem`. ::: services/sync/modules/engines/addons.js:542 (Diff revision 1) > // Currently, we limit syncable add-ons to those that are: > // 1) In a well-defined set of types > // 2) Installed in the current profile > // 3) Not installed by a foreign entity (i.e. installed by the app) > // since they act like global extensions. > - // 4) Is not a hotfix. > + // 4) Is not a hotfix nor a system addon. I would say "a hidden addon" or something to that effect since it covers temporary addons too (and presumably any future hidden addons)

https://reviewboard.mozilla.org/r/57526/#review54986 > The getter is `addon.hidden` not `isHidden` (unfortunately for consistency with other getters on this object): https://dxr.mozilla.org/mozilla-central/rev/3e8ee3599a67edd971770af4982ad4b0fe77f073/toolkit/mozapps/extensions/internal/XPIProvider.jsm#7262 > > `hidden` actually covers system add-on (as well as temporary add-ons) so you could just use this and not `isSystem`. Sorry, misspoke here - the "hidden" situation with temporary add-ons is a little more involved. If an add-on is overridden by a temporary add-on, then the original add-on is marked as hidden. In any case, markh just let me know that part 1 is what he wants feedback on so I'll drop this :) For what it's worth I think we should not sync temporary add-ons, *unless* they are overriding an existing non-hidden add-on. I'm happy to add a new getter that is closer to what you want here too, or possibly adjusting the current `hidden` getter, if that's helpful.

(In reply to Mark Hammond [:markh] from comment #11) > Comment on attachment 8759518 [details] > Bug 1275139 (part 1) - kill ignoreRepositoryChecking pref, replacing it with > AddonRepository.cache. > > (In reply to Richard Newman [:rnewman] from comment #10) > > https://reviewboard.mozilla.org/r/57522/#review54832 > > > > I'm pretty sure I don't understand this well enough to review it correctly. > > Mossop, maybe? > > Mossop isn't accepting requests, but I think Rob works on addons and might > be able to help with "part 3" where I need an authoritative check whether an > addon is syncable or not... > > Hey Rob, are you able to help here, or suggest someone else who can? > > The tl;dr is that Sync has its own preference for disabling any checks made > to the AddonRespository cache. However, that cache is disabled via its own > preference in all test environments, which leads to the odd situation where > Sync *thinks* it is checking the cache, but in reality the cache is disabled. > > I'm proposing that Sync *always* checks the repository cache whenever the > cache is enabled, period. It's not particularly critical, it's just a > foot-gun and the cause of at least 1 test not checking what it things it's > checking. This sounds reasonable to me. I am only passingly familiar with AddonRepository though, I am happy to review but would feel better if Unfocused has a moment to vet this approach first.

(In reply to Robert Helmer [:rhelmer] from comment #14) > I am happy to review but would feel better if > Unfocused has a moment to vet this approach first. In general, yep, that works for Mark's TL;DR in comment 11. Though, I'm not sure it seems entirely appropriate for controlling requireSecureURI, but then neither does the original pref. Note that some people will disable that manually as a privacy measure. So I suspect requireSecureURI may be better served by a different pref - possibly extensions.install.requireSecureOrigin

I chatted a little with Rob about this in London, and we ended up agreeing to add an |isSyncable| getter to XPIProvider and to have that only return true for addons in the profile directory - other addons should be ignored. I also decided that it makes sense for this code to also check is the addon is a hotfix in the interests of trying to move as much policy into the addons code rather than in Sync. Sadly, due to the testing infrastructure used by Sync, I couldn't also remove the hotfix check from Sync - but I did add a comment to this effect. Rob, to punish you for being so helpful I'm going to flag you for review on all patches (Unfocused isn't taking reviews or I'd ask him for part 1). There's no particular urgency, but please let me know if you'd like me to find another reviewer for any of them (*cough*rnewman*cough*)

Comment on attachment 8759518 [details] Bug 1275139 (part 1) - kill ignoreRepositoryChecking pref, replacing it with AddonRepository.cache. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57522/diff/1-2/

Comment on attachment 8759519 [details] Bug 1275139 (part 2) - change tests to demonstrate the problems with system addons. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57524/diff/1-2/

Comment on attachment 8759520 [details] Bug 1275139 (part 3) - don't Sync system addons, nor apply them when incoming. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57526/diff/1-2/

Comment on attachment 8759520 [details] Bug 1275139 (part 3) - don't Sync system addons, nor apply them when incoming. https://reviewboard.mozilla.org/r/57526/#review59400

https://reviewboard.mozilla.org/r/57522/#review60246 ::: services/sync/modules/engines/addons.js:575 (Diff revision 2) > } > > - // We provide a back door to skip the repository checking of an add-on. > - // This is utilized by the tests to make testing easier. Users could enable > - // this, but it would sacrifice security. > - if (Svc.Prefs.get("addons.ignoreRepositoryChecking", false)) { > + // If the AddonRepository's cache isn't enabled (which it typically isn't > + // in tests), getCachedAddonByID always returns null - so skip the check > + // in that case. > + if (!AddonRepository.cacheEnabled) { See bug 1285866 comment 0 - IIUC, a corrupt addons.json in the profile directory will see AddonRepository.cacheEnabled=false, meaning we skip this check. It's not clear to me is a corrupt json file will be repaired on next startup or not? Before this patch we'd simply fail to install the addon in this case :(

Comment on attachment 8759519 [details] Bug 1275139 (part 2) - change tests to demonstrate the problems with system addons. https://reviewboard.mozilla.org/r/57524/#review60356

Comment on attachment 8759518 [details] Bug 1275139 (part 1) - kill ignoreRepositoryChecking pref, replacing it with AddonRepository.cache. https://reviewboard.mozilla.org/r/57522/#review60708 lgtm and matches bug 1275139 comment 15

Pushed by mhammond@skippinet.com.au: https://hg.mozilla.org/integration/fx-team/rev/c4b69acde1ad (part 1) - kill ignoreRepositoryChecking pref, replacing it with AddonRepository.cache. r=rhelmer https://hg.mozilla.org/integration/fx-team/rev/14ef3f85de4e (part 2) - change tests to demonstrate the problems with system addons. r=rhelmer https://hg.mozilla.org/integration/fx-team/rev/7fd2a709bd6c (part 3) - don't Sync system addons, nor apply them when incoming. r=rhelmer

https://hg.mozilla.org/mozilla-central/rev/c4b69acde1ad https://hg.mozilla.org/mozilla-central/rev/14ef3f85de4e https://hg.mozilla.org/mozilla-central/rev/7fd2a709bd6c