Last Comment Bug 741670 - Add-on sync should ignore hotfixes
: Add-on sync should ignore hotfixes
Status: RESOLVED FIXED
[qa-]
:
Product: Cloud Services
Classification: Client Software
Component: Firefox Sync: Backend (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Gregory Szorc [:gps]
:
:
Mentors:
Depends on:
Blocks: 534956
  Show dependency treegraph
 
Reported: 2012-04-02 18:46 PDT by Matthew N. [:MattN] (PM me if requests are blocking you)
Modified: 2012-05-30 10:25 PDT (History)
8 users (show)
MattN+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
fixed


Attachments
Do not sync hotfix extensions, v1 (4.14 KB, patch)
2012-04-02 19:43 PDT, Gregory Szorc [:gps]
rnewman: review+
Details | Diff | Splinter Review
Do not sync hotfix extensions, v1 (4.54 KB, patch)
2012-04-02 20:25 PDT, Gregory Szorc [:gps]
rnewman: review+
blair: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Matthew N. [:MattN] (PM me if requests are blocking you) 2012-04-02 18:46:17 PDT
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] 2012-04-02 19:13:33 PDT
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] (PM me if requests are blocking you) 2012-04-02 19:19:00 PDT
(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] 2012-04-02 19:23:39 PDT
(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] 2012-04-02 19:43:33 PDT
Created attachment 611698 [details] [diff] [review]
Do not sync hotfix extensions, v1

This seems almost too easy.
Comment 5 Richard Newman [:rnewman] 2012-04-02 19:57:39 PDT
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] 2012-04-02 20:25:47 PDT
Created attachment 611706 [details] [diff] [review]
Do not sync hotfix extensions, v1

Add test using int pref per rnewman's feedback.
Comment 7 Gregory Szorc [:gps] 2012-04-03 08:57:39 PDT
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] (PM me if requests are blocking you) 2012-04-03 12:47:18 PDT
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] 2012-04-03 13:19:12 PDT
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] (PM me if requests are blocking you) 2012-04-03 13:50:43 PDT
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] 2012-04-04 04:48:52 PDT
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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-26 13:58:21 PDT
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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-30 10:25:47 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.