Need hotfix to mitigate TMP bustage in bug 813763

RESOLVED FIXED

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: akeybl, Assigned: MattN)

Tracking

unspecified
Points:
---

Firefox Tracking Flags

(firefox17+ fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
We need to create an add-on hotfix which:

* Targets all platforms and Firefox 17
* Uninstalls itself immediately if Tab Mix Plus version 0.4.0.3 or lower is not installed
* The first 2 startups after the hotfix is installed, open up a foreground tab with https://support.mozilla.org/kb/tab-mix-plus-incompatible-firefox-17-use-dev-build
* After those first 2 startups, the hotfix can uninstall itself
(Reporter)

Comment 1

6 years ago
Scenarios to QA, for Windows/Mac/Linux:
* Verify that when TMP >0.4.0.3 is installed, the hotfix uninstalls itself
* Verify that when TMP is not installed, the hotfix uninstalls itself
* Verify that the correct tab is shown for the first 2 startups after the hotfix is installed, followed by an uninstall
QA Contact: anthony.s.hughes

Updated

6 years ago
QA Contact: anthony.s.hughes
QA Contact: anthony.s.hughes
Jorge has proposed hosting the addon ourselves at AMO so perhaps we can point to the amo-hosted addon directly instead of SUMO article?  Will let Jorge confirm if we'll be able to proceed with this proposed plan.
(In reply to Lukas Blakk [:lsblakk] from comment #2)
> Jorge has proposed hosting the addon ourselves at AMO so perhaps we can
> point to the amo-hosted addon directly instead of SUMO article?  Will let
> Jorge confirm if we'll be able to proceed with this proposed plan.

Cancel that - we'll go ahead with the SUMO article unless the dev can be reached today or tomorrow before we are ready to ship 17.0.1 and an update made available.
Luckily this won't conflict with the existing hotfix on the wire.
Assignee: nobody → mnoorenberghe+bmo
Status: NEW → ASSIGNED
Jorge, can you confirm that we can have 2 hotfixes offered by AMO (via VersionCheck.php):
* existing one for 10.0 - 16.*
* this one for 17.0
Flags: needinfo?(jorge)
(In reply to Alex Keybl [:akeybl] from comment #0)
> * Targets all platforms and Firefox 17

What is the exact version range we want this for? 17.0a1 - 17.*?

Should this be delivered to ESR builds?
Flags: needinfo?(release-mgmt)
(Reporter)

Comment 7

6 years ago
(In reply to Matthew N. [:MattN] from comment #6)
> (In reply to Alex Keybl [:akeybl] from comment #0)
> > * Targets all platforms and Firefox 17
> 
> What is the exact version range we want this for? 17.0a1 - 17.*?
> 
> Should this be delivered to ESR builds?

We can deliver this to Firefox 17.* release users specifically to limit the testing scope. If delivering this hotfix to the ESR causes unnecessary complexity, we can drop that as well.
Flags: needinfo?(release-mgmt)
(Reporter)

Comment 8

6 years ago
(In reply to Matthew N. [:MattN] from comment #5)
> Jorge, can you confirm that we can have 2 hotfixes offered by AMO (via
> VersionCheck.php):
> * existing one for 10.0 - 16.*
> * this one for 17.0

If it ends up being that we can only have one hotfix on the wire at a time, no need to bring the 10.0-16.* hotfix into this one. The previous hotfix has been on the wire for a full cycle, and should have served its purpose at this point.
Created attachment 686373 [details] [diff] [review]
WIP v.1

I'd like to review this more myself and go over the various edge cases before calling this v.1.  Feedback from others in the meantime is welcome.
Attachment #686373 - Flags: feedback?
Felipe reminded me of the fact that TMP may be already disabled by the user.  I suggest that we don't notify the user if they disabled the addon themselves (addon.userDisabled = true).  This will avoid needlessly notifying users who once used the addon but disabled it at some point. My understanding is that the compatibility update would set addon.appDisabled = true so we can distinguish between how an addon is disabled.
(In reply to Matthew N. [:MattN] from comment #10)
>  My understanding is that the compatibility update would set addon.appDisabled =
> true so we can distinguish between how an addon is disabled.

Sounds good - QA can you add that scenario to the test plan in comment 1?
(In reply to Lukas Blakk [:lsblakk] from comment #11)
> (In reply to Matthew N. [:MattN] from comment #10)
> >  My understanding is that the compatibility update would set addon.appDisabled =
> > true so we can distinguish between how an addon is disabled.
> 
> Sounds good - QA can you add that scenario to the test plan in comment 1?

Already done. :)
Created attachment 686457 [details] [diff] [review]
v.1 for Fx 17.0 (including ESR)

Not sure who wants to take this review.

I attached the session restore observer earlier in case the notification is fired before the observer gets added.
Attachment #686373 - Attachment is obsolete: true
Attachment #686373 - Flags: feedback?
Attachment #686457 - Flags: review?(gavin.sharp)
Attachment #686457 - Flags: review?(dtownsend+bugmail)
Attachment #686457 - Flags: review?(dolske)
(In reply to Matthew N. [:MattN] from comment #5)
> Jorge, can you confirm that we can have 2 hotfixes offered by AMO (via
> VersionCheck.php):
> * existing one for 10.0 - 16.*
> * this one for 17.0

Yes, that should work fine. Adding the AOM people in case I got this wrong.
Flags: needinfo?(jorge)
Comment on attachment 686457 [details] [diff] [review]
v.1 for Fx 17.0 (including ESR)

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

::: v20120910.01/bootstrap.js
@@ +53,5 @@
> +function uninstall(hotfix, reason) { }
> +
> +function detectAffectedTMP(callback) {
> +  AddonManager.getAddonByID(TAX_MIX_PLUS_ID, function(addon) {
> +    if (!addon || addon.userDisabled) {

Change this to test addon.isActive

@@ +90,3 @@
>  
> +  // nsITimerCallback
> +  notify: function(timer) {

Null out the timer here

@@ +90,5 @@
>  
> +  // nsITimerCallback
> +  notify: function(timer) {
> +    AddonManager.getAddonByID("firefox-hotfix@mozilla.org", function(addon) {
> +      if (!addon) {

You can probably get rid of this test with the other changes here.

@@ +121,5 @@
>    return true;
>  }
>  
> +function uninstallHotfix(hotfix) {
> +  try {

Test hotfixObserver.timer and cancel it if not null
Attachment #686457 - Flags: review?(dtownsend+bugmail) → review+
Created attachment 686873 [details] [diff] [review]
v.2 address review comments

(In reply to Dave Townsend (:Mossop) from comment #15)
> ::: v20120910.01/bootstrap.js
> @@ +53,5 @@
> > +function uninstall(hotfix, reason) { }
> > +
> > +function detectAffectedTMP(callback) {
> > +  AddonManager.getAddonByID(TAX_MIX_PLUS_ID, function(addon) {
> > +    if (!addon || addon.userDisabled) {
> 
> Change this to test addon.isActive

I still want to notify the user if addon.appDisabled = true so I don't think I want isActive.  We changed the compatibility info on AMO so unless users override the compatibility, Tab Mix Plus 0.4.0.3 will be appDisabled. I thought the point of this hotfix is to explain why it is disabled. See comment 10.  Let me know if my understanding is incorrect.
Attachment #686457 - Attachment is obsolete: true
Attachment #686457 - Flags: review?(gavin.sharp)
Attachment #686457 - Flags: review?(dolske)
Attachment #686873 - Flags: review?(dtownsend+bugmail)
Comment on attachment 686873 [details] [diff] [review]
v.2 address review comments

Ok, that's fine to leave as is then
Attachment #686873 - Flags: review?(dtownsend+bugmail) → review+
https://hg.mozilla.org/releases/firefox-hotfixes/rev/412dde7968ff
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Comment on attachment 686873 [details] [diff] [review]
v.2 address review comments

>+function openNotificationTab(hotfix) {
>+  let recentWindow = Services.wm.getMostRecentWindow("navigator:browser");
>+  recentWindow.gBrowser.selectedTab = recentWindow.gBrowser.addTab(NOTIFICATION_URL);

This should have been:

recentWindow.openUILinkIn(NOTIFICATION_URL, "tab");

See the openLinkIn implementation in utilityOverlay.js for how this is a lot smarter than selectedTab = addTab(). For isntance, it will make sure to leave popup windows alone and set the owner tab such that closing the tab brings you back to the previously selected tab.
(In reply to onemen.one from comment #19)
> try Tabmix 0.4.0.3.1
> https://addons.mozilla.org/en-US/firefox/addon/tab-mix-plus/versions/

Please hold off on publishing this while we get testing in bug 813763 for the update that was published (comment 19)
status-firefox17: --- → fixed
You need to log in before you can comment on or make changes to this bug.