741670 - Add-on sync should ignore hotfixes

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Matthew N. [:MattN]]

Bug 694068 added support for downloading an extension as a hotfix.  The identifier of the extension is specified in the pref extensions.hotfix.id.  Add-on sync should not sync the hotfix add-on so that they can be targeted at specific operating systems and OS versions or other metrics not profile-specific.  

The preferred way of achieving this is to have the hotfix uninstall itself when it doesn't apply to that specific computer.  If the uninstall is synced them the hotfix may be removed from computers where it is needed.

Comment 1

[Gregory Szorc [:gps]]

To clarify, the requirement is that Sync should ignore all events for an extension, addon, where:

  addon.id == Prefs.get("extensions.hotfix.id")

Please confirm.

Comment 2

[Matthew N. [:MattN]]

(In reply to Gregory Szorc [:gps] from comment #1)
> To clarify, the requirement is that Sync should ignore all events for an
> extension, addon, where:
> 
>   addon.id == Prefs.get("extensions.hotfix.id")

From my understanding of discussions with Unfocused and Mossop, that is correct.  Note that each application needs to opt-in to this hotfix feature so the pref may not be present in some applications. (This only matters if other applications use Sync).

Comment 3

[Dave Townsend [:mossop]]

(In reply to Gregory Szorc [:gps] from comment #1)
> To clarify, the requirement is that Sync should ignore all events for an
> extension, addon, where:
> 
>   addon.id == Prefs.get("extensions.hotfix.id")
> 
> Please confirm.

Correct. And if the patch to do it is pretty low risk I think it'd be well worth backporting it to aurora and beta in time to make the next release.

Comment 4

[Gregory Szorc [:gps]]

Attachment: Do not sync hotfix extensions, v1

This seems almost too easy.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 611698 [details] [diff] [review]
Do not sync hotfix extensions, v1

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

Looks OK to me, though I can't vet the actual logic that's being implemented.

::: services/sync/modules/engines/addons.js
@@ +571,5 @@
>        return false;
>      }
>  
> +    // Ignore hotfix extensions (bug 741670). The pref may not be defined.
> +    if (this._extensionsPrefs.get("hotfix.id", null) == addon.id) {

Just about the only time I'm glad for all of Sync's wrapper classes; if we were using the prefs service directly, this could throw if it were having a bad day.

::: services/sync/tests/unit/test_addons_store.js
@@ +387,5 @@
> +
> +  // Basic sanity check.
> +  do_check_true(store.isAddonSyncable(dummy));
> +
> +  prefs.set("hotfix.id", dummy.id);

Could you also try a test where you set this to an int pref directly through the preferences service? I want to make sure that we don't die loudly if something screws up that pref.

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIPrefBranch#setIntPref%28%29

Comment 6

[Gregory Szorc [:gps]]

Attachment: Do not sync hotfix extensions, v1

Add test using int pref per rnewman's feedback.

Comment 7

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c62d71bac750

Skipped services-central since this one is on the fast track.

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 611706 [details] [diff] [review]
Do not sync hotfix extensions, v1

[Approval Request Comment]
Regression caused by (bug #): bug 534956 (not exactly a regression but had unintended consequences for hotfixes)
User impact if declined: Hotfix uninstallation will be synced across devices which means they can't target specific non-profile characteristics (ie. operating system version).  See bug 741004 comment 2.
Testing completed (on m-c, etc.): m-c starting today
Risk to taking this patch (and alternatives if risky): Fairly low risk checking of a pref and excluding hotfixes.  Worst case would be broken add-on sync.  Side effect is that hotfix add-ons may take longer to propagate.
String changes made by this patch: None

Comment 9

[Alex Keybl [:akeybl]]

Comment on attachment 611706 [details] [diff] [review]
Do not sync hotfix extensions, v1

[Triage Comment]
I'm comfortable with the risk here, and this is in support of bug 741004 - let's land on Beta 12 today.

Comment 10

[Matthew N. [:MattN]]

Note that I thought this landed directly on m-c but it's actually still on inbound.
https://hg.mozilla.org/releases/mozilla-aurora/rev/ff9768ab37b7
https://hg.mozilla.org/releases/mozilla-beta/rev/a3e31399d17c

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/c62d71bac750

please do not add [inbound] in the whiteboard, see http://blog.bonardo.net/2012/03/23/how-you-can-help-mozilla-inbound-sheriffs-when-pushing for suggestions

Comment 12

[u279076]

Since there is a hotfix live for Firefox 12 on Windows 2000, I assume we can use that to verify for Firefox 12. What about for Firefox 13?

Comment 13

[u279076]

There's been no response or direction as to how QA can test this. Please mark this bug as [qa+] if/when instructions can be provided. Thank you.