The default bug view has changed. See this FAQ.

Add-on sync should ignore hotfixes

RESOLVED FIXED in Firefox 12

Status

Cloud Services
Firefox Sync: Backend
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: MattN, Assigned: gps)

Tracking

unspecified
mozilla14
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox12+ fixed, firefox13 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

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.
(Assignee)

Comment 1

5 years ago
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.
(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).
(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.
(Assignee)

Comment 4

5 years ago
Created attachment 611698 [details] [diff] [review]
Do not sync hotfix extensions, v1

This seems almost too easy.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #611698 - Flags: review?(rnewman)
Attachment #611698 - Flags: review?(bmcbride)
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
Attachment #611698 - Flags: review?(rnewman) → review+
(Assignee)

Comment 6

5 years ago
Created attachment 611706 [details] [diff] [review]
Do not sync hotfix extensions, v1

Add test using int pref per rnewman's feedback.
Attachment #611698 - Attachment is obsolete: true
Attachment #611698 - Flags: review?(bmcbride)
Attachment #611706 - Flags: review?(rnewman)
Attachment #611706 - Flags: review?(bmcbride)
Attachment #611706 - Flags: review?(rnewman) → review+
Attachment #611706 - Flags: review?(bmcbride) → review+
(Assignee)

Comment 7

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

Skipped services-central since this one is on the fast track.
Whiteboard: [inbound]
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
Attachment #611706 - Flags: approval-mozilla-beta?
Attachment #611706 - Flags: approval-mozilla-aurora?

Comment 9

5 years ago
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.
Attachment #611706 - Flags: approval-mozilla-beta?
Attachment #611706 - Flags: approval-mozilla-beta+
Attachment #611706 - Flags: approval-mozilla-aurora?
Attachment #611706 - Flags: approval-mozilla-aurora+
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
status-firefox12: --- → fixed
status-firefox13: --- → fixed
tracking-firefox12: --- → ?
Flags: in-testsuite+
Target Milestone: --- → mozilla14
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
Whiteboard: [inbound]

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
tracking-firefox12: ? → +
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?
Whiteboard: [qa+]
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.
Whiteboard: [qa+] → [qa-]
You need to log in before you can comment on or make changes to this bug.