816197 - Need hotfix to mitigate TMP bustage in bug 813763

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Alex Keybl [:akeybl]]

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

Comment 1

[Alex Keybl [:akeybl]]

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

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 4

[Matthew N. [:MattN]]

Luckily this won't conflict with the existing hotfix on the wire.

Comment 5

[Matthew N. [:MattN]]

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

Comment 6

[Matthew N. [:MattN]]

(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?

Comment 7

[Alex Keybl [:akeybl]]

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

Comment 8

[Alex Keybl [:akeybl]]

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

Comment 9

[Matthew N. [:MattN]]

Attachment: 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.

Comment 10

[Matthew N. [:MattN]]

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.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

(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?

Comment 12

[u279076]

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

Comment 13

[Matthew N. [:MattN]]

Attachment: 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.

Comment 14

[Jorge Villalobos [:jorgev] (he/him)]

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

Comment 15

[Dave Townsend [:mossop]]

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("[e-mail]", 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

Comment 16

[Matthew N. [:MattN]]

Attachment: 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.

Comment 17

[Dave Townsend [:mossop]]

Comment on attachment 686873 [details] [diff] [review]
v.2 address review comments

Ok, that's fine to leave as is then

Comment 18

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/firefox-hotfixes/rev/412dde7968ff

Comment 19

[tabmix.onemen]

try Tabmix 0.4.0.3.1
https://addons.mozilla.org/en-US/firefox/addon/tab-mix-plus/versions/

Comment 20

[Dão Gottwald [:dao]]

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.

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

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