Open Bug 1285866 Opened 4 years ago Updated 3 years ago

AddonManager's repository sometimes doesn't have all addons, making Sync's use of it problematic

Categories

(Firefox :: Sync, defect, P3)

defect

Tracking

()

People

(Reporter: markh, Unassigned)

References

Details

(Whiteboard: [sync:addons])

STR:

* Flip pref "extensions.getAddons.cache.enabled" to false, install an addon, then flip the pref back to true.
** Alternatively: with an addon already installed, delete addons.json from your profile directory and restart.
* Sync addons.

Expected:
* Addon is written to Sync storage and synced correctly.

Actual:
* Sync writes a log message:
> Sync.Store.Addons	DEBUG	{some-addon-id} not syncable: add-on not found in add-on repository.

Best I can tell, the addons repository does not say it guarantees information about every installed addon is available, but instead it populates the DB as *it* needs to. For example, if you tell the addons manager to update installed addons, it ends up writing new entries for addons it did find updates for, but not the rest. If you install a new addon, that new addon alone is written. Once addons.json has been deleted, I can't seem to convince Firefox to fill addons.json with all installed addons.

The problem is that Sync refuses to upload addons that aren't in the repository. IIUC, in a profile that has never seen a corrupt addons.json will typically have all such addons - but egde-cases like the above are a problem.

IIUC, this check is, basically, "is this an AMO hosted addon" (while respecting prefs that allow an alternate store).

(Worse, IIUC, a *corrupt* addons.json will not repair the cache, but instead just work as though caching is disabled. With the patch in bug 1275139, I think that Sync will suddenly start Syncing all addons (as it sees the cache is disabled, assumes that's due to the user flipping that pref, so allow pretty-much any addon)

The way I see it, we have a couple of options:

* Call AddonRepository.cacheAddons() with all the IDs we care about. This is potentially expensive, so we could consider doing that only after we find an addon is *not* in the repository rather than unconditionally? But for addons not installed via AMO, doing this each and every Sync doesn't sound ideal.

* Use some different technique to ask "is this an AMO addon?" - but I'm not sure what that would be.

Greg/Richard, is there something above that I missed from the sync POV?

Robert, do you have any thoughts here from the addon-manager's POV?
Flags: needinfo?(rnewman)
Flags: needinfo?(rhelmer)
Flags: needinfo?(gps)
That is indeed the point of the check. The check exists for two reasons:

1. To make this malware vector slightly harder to use. (I won't go into details, obviously.)
2. To avoid failure on other devices when trying to install non-AMO add-ons, because we don't know how to fetch them.

I think that I agree with your conclusion. I would propose that:

* A corrupt cache should repair, not fall back to it being disabled, unless a cycle occurs.
* Sync needs a reliable directory, so we should make that happen. Building the cache might be expensive, but so is trawling the disk each time! And this can be mitigated: it probably only needs to be rebuilt once per session, and presumably the add-on manager knows which add-ons have loaded etc., so …
Flags: needinfo?(rnewman)
I defer to rnewman.
Flags: needinfo?(gps)
markh and I discussed this in IRC a little bit - one approach would be to add a method to the public `Addon` object which would only return true if the add-on was listed on AMO.

This way you could just check something like `addon.isHosted` (better name wanted :) - we could hide the details of using AddonRepository behind this. This sort of use case ("is this add-on hosted on AMO and not blacklisted") has come up before so I think it would appropriate for the public AddonManager API.

We could also/instead just have a more efficient way for AddonRepository to return the set of add-ons which are both hosted on AMO and also locally installed, but the above makes it easy to answer the same sort of question I think.
Flags: needinfo?(rhelmer)
Priority: -- → P2
Depends on: 1286785
Priority: P2 → P3
FTR - see bug 1272446 comment 18 for a pointer to how fixing this should be a little simpler now.
(In reply to Robert Helmer [:rhelmer] from comment #3)
> This way you could just check something like `addon.isHosted` (better name
> wanted :)

Maybe addon.isInRepo(sitory)?

So as to the implementation of this...

(In reply to Mark Hammond [:markh] from comment #4)
> FTR - see bug 1272446 comment 18 for a pointer to how fixing this should be
> a little simpler now.

Now I'm not sure how relevant that bug actually is :)

This bug is describing 2 different issues - comment 0 describes:

* flipping a pref such that an individual addon is missing from addons.json
* a bad addons.json such that no addons are listed.

ISTM that bug 1272446 is almost insisting AMO addons are listed in addons.json? If so, this implies to me the fix here is:

* See if we can kill the "extensions.getAddons.cache.enabled" pref, or otherwise declare that a single missing addon from addons.json is outside the scope of this bug.
* If addons.json is missing or corrupt, treat it like bug 1272446 - somewhat revisiting bug 1286785, where we just init an empty addons.json in these cases.
* Implement addon.isInRepo using exclusively what's listed in addons.json

Rob, does that sound right?
Needinfo'ing myself to respond to comment 5 when I have a moment to think through this :)
Flags: needinfo?(rhelmer)
(In reply to Mark Hammond [:markh] from comment #5)
> (In reply to Robert Helmer [:rhelmer] from comment #3)
> > This way you could just check something like `addon.isHosted` (better name
> > wanted :)
> 
> Maybe addon.isInRepo(sitory)?
> 
> So as to the implementation of this...
> 
> (In reply to Mark Hammond [:markh] from comment #4)
> > FTR - see bug 1272446 comment 18 for a pointer to how fixing this should be
> > a little simpler now.
> 
> Now I'm not sure how relevant that bug actually is :)
> 
> This bug is describing 2 different issues - comment 0 describes:
> 
> * flipping a pref such that an individual addon is missing from addons.json
> * a bad addons.json such that no addons are listed.
> 
> ISTM that bug 1272446 is almost insisting AMO addons are listed in
> addons.json? If so, this implies to me the fix here is:


Yes, there are really two different addons "databases":

* extensions.json which is built from the actually installed manifests
* addons.json which is periodically synced from AMO

So the latter by definition should only contain add-ons present in AMO, AFAICT. The problem is that it may not be up-to-date with what the user has installed, where the first one should be (except in the cases where an on-disk manifest has changed or has a previously-unused setting in the manifest, which is what bug 1272446 mitigates by re-scanning on addons DB schema changes)

addons.json should at least be eventually consistent, so I think it's OK to use for sync purposes, keeping this in mind. See bug 586591 for the original implementation notes ("Update the database when background updates occur (once a day by default)")

Since users installing from AMO will almost certainly be able to reach AMO (but not guaranteed, they could be installing offline for some reason) we could try writing to addons.json anytime we touch extensions.json too if we aren't already. The users who can't reach AMO probably can't reach Sync servers either, so it's probably not a useful edge case to worry about (and it's eventually consistent)

> * See if we can kill the "extensions.getAddons.cache.enabled" pref, or
> otherwise declare that a single missing addon from addons.json is outside
> the scope of this bug.
> * If addons.json is missing or corrupt, treat it like bug 1272446 - somewhat
> revisiting bug 1286785, where we just init an empty addons.json in these
> cases.
> * Implement addon.isInRepo using exclusively what's listed in addons.json
> 
> Rob, does that sound right?

Yes I think this will work. The only other approach I can see is pulling metadata from AMO in sync code real-time, but we're better off just getting it right in one place in addons manager and providing a reasonable API for sync to use as you've proposed.

Also see above about attempting to write to addons.json every time we write to extensions.json - "extensions.getAddons.cache.enabled" is part of the puzzle but I'm not sure I quite understand the point of this pref. I notice this came up in the review in 586591 comment 4.
Flags: needinfo?(rhelmer)
(In reply to Robert Helmer [:rhelmer] from comment #7)
> (In reply to Mark Hammond [:markh] from comment #5)
> > (In reply to Robert Helmer [:rhelmer] from comment #3)
> > > This way you could just check something like `addon.isHosted` (better name
> > > wanted :)
> > 
> > Maybe addon.isInRepo(sitory)?
> > 
> > So as to the implementation of this...
> > 
> > (In reply to Mark Hammond [:markh] from comment #4)
> > > FTR - see bug 1272446 comment 18 for a pointer to how fixing this should be
> > > a little simpler now.
> > 
> > Now I'm not sure how relevant that bug actually is :)
> > 
> > This bug is describing 2 different issues - comment 0 describes:
> > 
> > * flipping a pref such that an individual addon is missing from addons.json
> > * a bad addons.json such that no addons are listed.
> > 
> > ISTM that bug 1272446 is almost insisting AMO addons are listed in
> > addons.json? If so, this implies to me the fix here is:
> 
> 
> Yes, there are really two different addons "databases":
> 
> * extensions.json which is built from the actually installed manifests
> * addons.json which is periodically synced from AMO
> 
> So the latter by definition should only contain add-ons present in AMO,
> AFAICT. The problem is that it may not be up-to-date with what the user has
> installed, where the first one should be (except in the cases where an
> on-disk manifest has changed or has a previously-unused setting in the
> manifest, which is what bug 1272446 mitigates by re-scanning on addons DB
> schema changes)
> 
> addons.json should at least be eventually consistent, so I think it's OK to
> use for sync purposes, keeping this in mind. See bug 586591 for the original
> implementation notes ("Update the database when background updates occur
> (once a day by default)")
> 
> Since users installing from AMO will almost certainly be able to reach AMO
> (but not guaranteed, they could be installing offline for some reason) we
> could try writing to addons.json anytime we touch extensions.json too if we
> aren't already.

So this *should* be what's happening now (addons.json is written at install time, and also when the addons timer fires), if that's not the case it's a bug we should fix. Likewise, not recovering from a 0-byte or corrupt addons.json is a bug and we should fix this. I'll try to repro these and file.

Also - is there any reason we need a separate getter for this, or is being AMO-hosted one of the criteria for `isSyncable()` sufficient?
(In reply to Robert Helmer [:rhelmer] from comment #8)
> So this *should* be what's happening now (addons.json is written at install
> time, and also when the addons timer fires), if that's not the case it's a
> bug we should fix.

This seems to work fine, except for addons that happen to be installed while "extensions.getAddons.cache.enabled" is false. It may be that we don't think that work handling though.

> Likewise, not recovering from a 0-byte or corrupt
> addons.json is a bug and we should fix this. I'll try to repro these and
> file.

I'm confident that bug exists. I believe fixing it would make this bug go away all by itself - although I agree with earlier comments that having this check inside the addons code is better than sync trying to work that out for itself.

> Also - is there any reason we need a separate getter for this, or is being
> AMO-hosted one of the criteria for `isSyncable()` sufficient?

The separate getter came from comment 3. There is exactly 1 edge-case where we skip the repo check, which would make adding it to the getter problematic - that edge-case is in the addons "validator" at https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/engines/addons.js#809, but TBH I can't see why we skip the check in that case - I first thought it was to ensure addons which aren't in the repo haven't erroneously made it to the sync server, but it's not clear to me how that would work in practice - Thom, can you enlighten us?
Flags: needinfo?(tchiovoloni)
I believe it was because it was causing a lot of false positives when testing (and I attributed it at the time to this bug, as well as the fact that having just synced doesn't guarantee the addons repo is up to date).

That said, I can no longer replicate that issue, and wouldn't be opposed to removing that case.
Flags: needinfo?(tchiovoloni)
(In reply to Thom Chiovoloni [:tcsc] from comment #10)
> That said, I can no longer replicate that issue, and wouldn't be opposed to
> removing that case.

Awesome, thanks, so that means:

(In reply to Robert Helmer [:rhelmer] from comment #8)
> Also - is there any reason we need a separate getter for this, or is being
> AMO-hosted one of the criteria for `isSyncable()` sufficient?

Adding it just to the isSyncable getter would be fine.
Whiteboard: [sync:addons]
You need to log in before you can comment on or make changes to this bug.