741004 - Create a hotfix add-on to communicate the FF13 de-support

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Alex Keybl [:akeybl]]

We need to create a hotfix add-on to communicate the FF13 de-support. Here's what we've nailed down so far:

* The add-on should only be available for Firefox versions 10 through 12 running on Windows
* The add-on will uninstall itself immediately if it isn't running on Windows 2000, Windows XP RTM, or Windows XP SP 1
* Our goal is to have no strings needing localization within the add-on itself - we'll try to reuse strings already available in Firefox
* We'll want to use a different URL for the billboard depending on whether Windows 2000 or Windows XP is being used
* We should shoot to have this available for release on 4/24, but it would be great to test on FF12 while it's still on beta prior to 4/24
* We should use the assets in bug 719893 for the hotfix icon

TBD:

* Final copy for the add-on description
* Frequency of prompting after first being installed
* Finalizing the URLs we're hitting for the billboard
* Finalizing the UI - for now, let's assume the UI is a titlebar, a billboard frame (same size as the major update prompt), and an OK button

Comment 1

[Alex Keybl [:akeybl]]

Also TBD:

* The title of the add-on (do we want to go with "Mozilla Firefox hotfix" or something more specific to this purpose)

Comment 2

[Matthew N. [:MattN]]

Attachment: WIP unsigned add-on with billboard on startup

I used this table from MSDN to determine the Windows version numbers affected. http://msdn.microsoft.com/en-us/library/windows/desktop/ms724833%28v=vs.85%29.aspx
I'm going to get clarification on which service pack versions matter for the various 5.2 versions of Windows.

The main problem I've come across is that add-on sync will sync the disabled/uninstalled state from a supported computer to an unsupported one.  ie. A user has two computers (Windows 7 and Windows 2000) and we auto-uninstall on Windows 7 then it will also be uninstalled from Windows 2000. 
It would be ideal if add-on sync ignored the add-on specified by extensions.hotfix.id but since add-on sync already shipped in FF11, we need an alternative.

Comment 3

[Matthew N. [:MattN]]

I filed bug 741670 to have add-on sync ignore hotfixes.

Some other ideas from gps and Unfocused:
* Don't uninstall the hotfix if it doesn't apply (downside is that users see it in the AOM)
* Gave the hotfix install an add-on in the system profile directory which is excluded from add-on sync.
* Paraphrasing gps: Sync also maintains a list of add-on changes which get updated when add-ons do certain things (like install or uninstall). You could always disable the tracker or update the source of truth although that is Hacky (with a giant H).

Comment 4

[Alex Keybl [:akeybl]]

Dave notes in email that we should set strictCompatibility=true in the install.rdf so that we can vend a separate add-on hotfix for future versions of Firefox.

> I've verified that AMO behaves as expected, a hotfix for 10-12 
> won't be offered to later versions (as long as you include 
> strictCompatibility=true in the install.rdf).

Comment 5

[Matthew N. [:MattN]]

Yep, I added strictCompability along with <em:targetPlatform>WINNT</em:targetPlatform> after I uploaded my WIP XPI.

Comment 6

[Matthew N. [:MattN]]

There is some ambiguity about versions of Windows identified as 5.2 [1].  MSDN talks about required SP versions[2] but doesn't mention XP x64 or home server.  The marketshare is probably small and khuey mentioned that there was a decision in an email somewhere not to worry about XP x64 but I want to get confirmation about how to proceed.

Some options:
1) Ignore 5.2 versions of Windows for this messaging
2) Target all affected versions where we know the required SP from [2]
3) Test XP x64 by installing a copy from MSDN and target all affected 5.0 - 5.2 versions.

[1] Remarks table on http://msdn.microsoft.com/en-us/library/windows/desktop/ms724833%28v=vs.85%29.aspx
[2] http://msdn.microsoft.com/en-us/library/ws0swas0.aspx

Comment 7

[Alex Keybl [:akeybl]]

(In reply to Matthew N. [:MattN] from comment #6)
> There is some ambiguity about versions of Windows identified as 5.2 [1]. 
> MSDN talks about required SP versions[2] but doesn't mention XP x64 or home
> server.  The marketshare is probably small and khuey mentioned that there
> was a decision in an email somewhere not to worry about XP x64 but I want to
> get confirmation about how to proceed.

Are we sure 5.2 is affected? Bug 668574 only seems to block 5.0, 5.1.0, and 5.1.1. Here's the conversation about XP x64 and Server from dev-planning: https://groups.google.com/group/mozilla.dev.planning/msg/e752febf21d65c49.

Comment 8

[Matthew N. [:MattN]]

(In reply to Alex Keybl [:akeybl] from comment #7)
> (In reply to Matthew N. [:MattN] from comment #6)
> > There is some ambiguity about versions of Windows identified as 5.2 [1]. 
> > MSDN talks about required SP versions[2] but doesn't mention XP x64 or home
> > server.  The marketshare is probably small and khuey mentioned that there
> > was a decision in an email somewhere not to worry about XP x64 but I want to
> > get confirmation about how to proceed.
> 
> Are we sure 5.2 is affected? Bug 668574 only seems to block 5.0, 5.1.0, and
> 5.1.1. Here's the conversation about XP x64 and Server from dev-planning:
> https://groups.google.com/group/mozilla.dev.planning/msg/e752febf21d65c49.

http://msdn.microsoft.com/en-us/library/ws0swas0.aspx implies that VC2010 requires SP1 for Windows Server 2003 whereas VC2008 doesn't require a service pack for Server 2003 [1].

[1] http://msdn.microsoft.com/en-us/library/ws0swas0%28v=vs.90%29.aspx

Comment 10

[Matthew N. [:MattN]]

I've put TODOs and known issues in an etherpad[1] (TODOs copied below).  The main part of development remaining depends on the finalized UI and the interval to prompt the user.  Feel free to update the etherpad and/or comment here with finalized details so we can get the extension development finished this week.

[1] https://etherpad.mozilla.org/arC2CZqj6y

===== TODO =====
* Final copy for the add-on description
* The title of the add-on (do we want to go with "Mozilla Firefox hotfix" or something more specific to this purpose)
* Finalize UI - Currently a billboard dialog.
* Decide on reminding interval – Use idle timer? Show immediately on first install or wait until idle?
* Should the hotfix check the application pref after install in case the user disables application update after the hotfix is installed?
* Finalizing the URLs we're hitting for the billboard - Current parameters are ?majorVersion=5&minorVersion=1&servicePackMajor=1&servicePackMinor=999
* Decide how to handle XP x64 and Windows Server versions.  Test on these OSs
* Version # – can't re-use them because last hotfix number is stored in a pref - How about a timestamp? ie. 20120424.01
* Get the extension signed

Comment 11

[Dave Townsend [:mossop]]

For reference I put the hotfixes I made for testing in a hg repository at http://hg.mozilla.org/users/dtownsend_mozilla.com/hotfixes/. Not sure whether that is the sort of thing we want to use all the time or not though.

Comment 12

[Laura Mesa [:lmesa] [:lvmesa]]

Just to be clear, I've been working on getting the copy for what the billboard actually says with Pascal and James Long in a separate bug. How can we best coordinate?

You can see the final copy in this etherpad (https://pmm.etherpad.mozilla.org/33 - scroll to the bottom), but James and Pascal have implemented it into billboards and asked the l10n communities to localize those already.  

[James and Pascal, we're only using the hotfix for the WinXP and Win2K billboards, not the others.]

Comment 13

[Laura Mesa [:lmesa] [:lvmesa]]

(In reply to Matthew N. [:MattN] from comment #10)
> I've put TODOs and known issues in an etherpad[1] (TODOs copied below).  The
> main part of development remaining depends on the finalized UI and the
> interval to prompt the user.  Feel free to update the etherpad and/or
> comment here with finalized details so we can get the extension development
> finished this week.
> 
> [1] https://etherpad.mozilla.org/arC2CZqj6y
> 
> ===== TODO =====
> * Final copy for the add-on description
> * The title of the add-on (do we want to go with "Mozilla Firefox hotfix" or
> something more specific to this purpose)
> * Finalize UI - Currently a billboard dialog.
> * Decide on reminding interval – Use idle timer? Show immediately on first
> install or wait until idle?
> * Should the hotfix check the application pref after install in case the
> user disables application update after the hotfix is installed?
> * Finalizing the URLs we're hitting for the billboard - Current parameters
> are ?majorVersion=5&minorVersion=1&servicePackMajor=1&servicePackMinor=999
> * Decide how to handle XP x64 and Windows Server versions.  Test on these OSs
> * Version # – can't re-use them because last hotfix number is stored in a
> pref - How about a timestamp? ie. 20120424.01
> * Get the extension signed

Who is the owner of the coordination? I've added a few comments in the etherpad, but I think it might be worth meeting up quickly to make sure we're all on the same page, specifically around the billboard copy we're already localizing.

Comment 14

[Alex Keybl [:akeybl]]

(In reply to Laura Mesa [:lmesa] [:lvmesa] from comment #13)
> Who is the owner of the coordination? I've added a few comments in the
> etherpad, but I think it might be worth meeting up quickly to make sure
> we're all on the same page, specifically around the billboard copy we're
> already localizing.

I've just added answers to most of the open questions in https://etherpad.mozilla.org/arC2CZqj6y

I'll set up a meeting for tomorrow morning to finalize the remaining TODO items, testing (with QA present), and scheduling.

Comment 15

[Alex Keybl [:akeybl]]

(In reply to Matthew N. [:MattN] from comment #6)
> Some options:
> 1) Ignore 5.2 versions of Windows for this messaging
> 2) Target all affected versions where we know the required SP from [2]
> 3) Test XP x64 by installing a copy from MSDN and target all affected 5.0 -
> 5.2 versions.

Since we're not blocking updates for 5.2 versions of Windows to FF13, I don't think there's any value in sending them a de-support notification. Let's go with option #1.

Also, for sake of completeness, here is the current QA testplan that Anthony has put together: https://wiki.mozilla.org/QA/Desupport_Hotfix_Testing

Comment 16

[Matthew N. [:MattN]]

Attachment: Screenshot of dialog and AOM details page

Here is a screenshot for those who can't test this.

(In reply to Alex Keybl [:akeybl] from comment #15)
> (In reply to Matthew N. [:MattN] from comment #6)
> > Some options:
> > 1) Ignore 5.2 versions of Windows for this messaging
> > 2) Target all affected versions where we know the required SP from [2]
> > 3) Test XP x64 by installing a copy from MSDN and target all affected 5.0 -
> > 5.2 versions.
> 
> Since we're not blocking updates for 5.2 versions of Windows to FF13, I
> don't think there's any value in sending them a de-support notification.
> Let's go with option #1.

Was there a decision made to not block updates for 5.2? It seems to me like it's not blocked by omission.  Why would we want to break Firefox for users who are updating when we can easily avoid it? I think it's safer to require SP1 for all 5.2 users since we know from [2] that it's the right thing for Server 2003.

Comment 17

[Matthew N. [:MattN]]

Attachment: WIP unsigned add-on with billboard on startup every 2 weeks

An issue I ran into is that I need the links from the billboard to open in the main browser window, not the small billboard <browser>. Have we had billboards with links before?  It seems like I'll have to attach a listener to the browser element and look for link clicks and send them to most recent browser window instead.

Comment 18

[Matthew N. [:MattN]]

Attachment: WIP patch of attachment 615252

I'm not sure what the review policy is for a hotfix but I wouldn't mind a quick review pass before the QA starts. Note that right now it's using a single page on my people account for XP and 2000.  You can use compatibility mode in XP to pretend to be XP RTM or 2000 if you want to test.

Comment 19

[Axel Hecht [:Pike]]

Comment on attachment 615255 [details] [diff] [review]
WIP patch of attachment 615252 [details]

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

::: content/unsupportedOS.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const billboardURL = "https://people.mozilla.com/~mnoorenberghe/winxp.htm"; //"https://www.mozilla.org/en-US/firefox/unsupported/details/"; // TODO:change

The URL shouldn't hardcode the locale, i.e., https://www.mozilla.org/firefox/unsupported/details/

Comment 20

[Dave Townsend [:mossop]]

Comment on attachment 615255 [details] [diff] [review]
WIP patch of attachment 615252 [details]

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

Some questions I'd like to see answered here

::: bootstrap.js
@@ +9,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +const REPROMPT_INTERVAL_SECONDS = 14 * 24 * 60 * 60; // 2 weeks
> +const LAST_PROMPT_TIME_PREF = "extensions.firefox-hotfix.unsupportedOSLastPrompt";
> +const BOOTSTRAP_REASONS = {

These should already be defined in bootstrap.js's scope, no need to re-declare them.

@@ +25,5 @@
> +
> +function install(data, reason) {
> +  checkApplicationVersion(data);
> +
> +  AddonManager.getAddonByID(data.id, function(addon) {

Nasty things might happen if checkApplicationVersion already uninstalled the hotfix. At the least I hope addon is null here so this will throw.

@@ +27,5 @@
> +  checkApplicationVersion(data);
> +
> +  AddonManager.getAddonByID(data.id, function(addon) {
> +    // Auto-enable on install
> +    addon.userDisabled = false;

Why is this necessary?

@@ +38,5 @@
> +  Services.prefs.clearUserPref(LAST_PROMPT_TIME_PREF);
> +}
> +
> +function startup(data, reason) {
> +  checkApplicationVersion(data);

As above

@@ +63,5 @@
> +function shutdown(data, reason) {
> +  LOG(reason);
> +  LOG(BOOTSTRAP_REASONS.ADDON_DISABLE);
> +  if (reason == BOOTSTRAP_REASONS.ADDON_DISABLE
> +      || reason == BOOTSTRAP_REASONS.ADDON_UNINSTALL) {

Why would you ever want to leave the window open, even if upgrading the hotfix it might be getting upgraded to a version that doesn't do the billboard or something.

@@ +103,5 @@
> + * Based on getServicePack from toolkit/mozapps/update/test/unit/test_0040_general.js
> + */
> +function getWindowsVersion() {
> +  let xulRuntime = Components.classes["@mozilla.org/xre/app-info;1"].
> +                   getService(Components.interfaces.nsIXULRuntime);

Use Cc and Ci

@@ +197,5 @@
> +
> +/**
> + * Open a dialog to notify the user that their operating system is unsupported.
> + */
> +function notifyUnsupportedWindows(versionInfo) {

So this only seems to get called when you first install the add-on or when you start Firefox. Is that enough? What about users that leave Firefox running for long periods?

If this happens during startup have you verified that the window appears on top of the browser window and not behind it?

::: content/unsupportedOS.css
@@ +5,5 @@
> +/* Remote content displayed in the billboard. */
> + remotecontent {
> +   -moz-binding: url("chrome://mozapps/content/update/updates.xml#remotecontent");
> +   display: -moz-deck;
> + }

Nit: Bad indentation

::: content/unsupportedOS.xul
@@ +31,5 @@
> +    <remotecontent id="updateMoreInfoContent" flex="1" remotetype="billboard"/><!-- TODO: test links -->
> +  </wizardpage>
> +
> +  <!-- HACK: button to get the localized label from -->
> +  <button id="okButton" label="&okButton.label;" hidden="true"/>

If you use a <dialog> then you'd get this for free

::: install.rdf
@@ +4,5 @@
> +     xmlns:em="http://www.mozilla.org/2004/em-rdf#">
> +
> +  <Description about="urn:mozilla:install-manifest">
> +    <em:id>[e-mail]</em:id>
> +    <em:version>20120413.01</em:version>

Is this the versioning scheme we plan to use for hotfixes? Seems reasonable to me

Comment 21

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] from comment #17)
> An issue I ran into is that I need the links from the billboard to open in
> the main browser window, not the small billboard <browser>. Have we had
> billboards with links before?  It seems like I'll have to attach a listener
> to the browser element and look for link clicks and send them to most recent
> browser window instead.

I'm going to use a click event listener checking if the target is a HTMLAnchorElement which seems to work well.

(In reply to Axel Hecht [:Pike] from comment #19)
> The URL shouldn't hardcode the locale, i.e.,
> https://www.mozilla.org/firefox/unsupported/details/

Yep, the URL will have the locale substituted in when it's finalized.  That URL is for a 3.6 billboard.

Comment 22

[Axel Hecht [:Pike]]

(In reply to Matthew N. [:MattN] from comment #21)
> 
> (In reply to Axel Hecht [:Pike] from comment #19)
> > The URL shouldn't hardcode the locale, i.e.,
> > https://www.mozilla.org/firefox/unsupported/details/
> 
> Yep, the URL will have the locale substituted in when it's finalized.  That
> URL is for a 3.6 billboard.

My point is, rely on the language negotiation on the webserver, just don't specify a locale at all.

On a different note, should the dialog size be flexible size-to-content stuff instead of sizing the dialog? Some localizatinos might very well need more space than en-US to get the message across.

Comment 23

[Matthew N. [:MattN]]

(In reply to Axel Hecht [:Pike] from comment #22)
> (In reply to Matthew N. [:MattN] from comment #21)
> > 
> > (In reply to Axel Hecht [:Pike] from comment #19)
> > > The URL shouldn't hardcode the locale, i.e.,
> > > https://www.mozilla.org/firefox/unsupported/details/
> > 
> > Yep, the URL will have the locale substituted in when it's finalized.  That
> > URL is for a 3.6 billboard.
> 
> My point is, rely on the language negotiation on the webserver, just don't
> specify a locale at all.

I didn't think that was the preferred way since most of our current in-product URLS have /%LOCALE%/.  

> On a different note, should the dialog size be flexible size-to-content
> stuff instead of sizing the dialog? Some localizatinos might very well need
> more space than en-US to get the message across.

I was thinking about that as well but I didn't find a bug for the creation of the billboards.  We can use the billboard feature to zoom the content out from the server side if we need as well.

Comment 24

[Alex Keybl [:akeybl]]

I just wanted to jump in here and give everybody an update on scheduling.

* A signed hotfix targeting only FF12 will be pushed to our beta audience no later than Thursday 4/19 so that we can collect feedback on any fallout
** To meet that goal, we need to have a signed hotfix available that morning so that QA can run through https://wiki.mozilla.org/QA/Desupport_Hotfix_Testing on the staged server prior to the push, and on the production server after
** We're currently waiting on l10n work to be completed, but plan to go out with all available localizations no later than 4/19
* On 4/23, the above hotfix will be disabled 
* A signed hotfix targeting FF10-FF12 will be pushed out soon after 4/24

Comment 25

[Axel Hecht [:Pike]]

In particular in the case where we may not have all locales ready on the billboard, doing server-side language negotiation is far superior to a 404 that redirects to the home page.

Also, as the remaining l10n work is the billboard content on mozilla.org, new locales will appear as we push them to production on the web, independently on when we ship the hotfix addon.

Would be a good QA task to verify that locales without a ready billboard do get a billboard page in a available language instead of garbage.

Comment 26

[Matthew N. [:MattN]]

Attachment: v.2 patch using www-dev

(In reply to Dave Townsend (:Mossop) from comment #20)
> Review of attachment 615255 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some questions I'd like to see answered here
>
> @@ +25,5 @@
> > +
> > +function install(data, reason) {
> > +  checkApplicationVersion(data);
> > +
> > +  AddonManager.getAddonByID(data.id, function(addon) {
> 
> Nasty things might happen if checkApplicationVersion already uninstalled the
> hotfix. At the least I hope addon is null here so this will throw.

I changed checkApplicationVersion to return whether the application version was valid and return early from startup if it's false. I changed the order of the two function calls in install to avoid this. Does this address your concern?

> @@ +27,5 @@
> > +  checkApplicationVersion(data);
> > +
> > +  AddonManager.getAddonByID(data.id, function(addon) {
> > +    // Auto-enable on install
> > +    addon.userDisabled = false;
> 
> Why is this necessary?

I'm handling the case where a user disabled a previous hotfix. I think that disabling one hotfix shouldn't imply that future hotfixes are unwanted as they may be unrelated.
 
> @@ +63,5 @@
> > +function shutdown(data, reason) {
> > +  LOG(reason);
> > +  LOG(BOOTSTRAP_REASONS.ADDON_DISABLE);
> > +  if (reason == BOOTSTRAP_REASONS.ADDON_DISABLE
> > +      || reason == BOOTSTRAP_REASONS.ADDON_UNINSTALL) {
> 
> Why would you ever want to leave the window open, even if upgrading the
> hotfix it might be getting upgraded to a version that doesn't do the
> billboard or something.

That's a good point.  I was thinking of the case where we iterate on this same unsupported OS hotfix (ie. a bugfix for the hotfix) and the user left the billboard open as a reminder or because they hadn't seen it yet.  I was thinking that I didn't want the user to have to wait another 2 weeks before they see the message again since either they wanted to follow-up with it or they didn't see it yet.

> @@ +197,5 @@
> > +
> > +/**
> > + * Open a dialog to notify the user that their operating system is unsupported.
> > + */
> > +function notifyUnsupportedWindows(versionInfo) {
> 
> So this only seems to get called when you first install the add-on or when
> you start Firefox. Is that enough? What about users that leave Firefox
> running for long periods?

Yeah, I wondered the same thing but at a meeting on Friday it didn't seem like that was a big issue.  I confirmed with akeybl that a startup notification is fine if we lower the threshold to 10 days.

> If this happens during startup have you verified that the window appears on
> top of the browser window and not behind it?

I've now postponed showing the dialog until 5 seconds after sessionstore-windows-restored if it's run on APP_STARTUP. (5 seconds is arbitrary)

> ::: content/unsupportedOS.xul
> @@ +31,5 @@
> > +    <remotecontent id="updateMoreInfoContent" flex="1" remotetype="billboard"/><!-- TODO: test links -->
> > +  </wizardpage>
> > +
> > +  <!-- HACK: button to get the localized label from -->
> > +  <button id="okButton" label="&okButton.label;" hidden="true"/>
> 
> If you use a <dialog> then you'd get this for free

I tried switching to a dialog but had some problems with sizing which are no longer an issue so I fixed this.

> ::: install.rdf
> @@ +4,5 @@
> > +     xmlns:em="http://www.mozilla.org/2004/em-rdf#">
> > +
> > +  <Description about="urn:mozilla:install-manifest">
> > +    <em:id>[e-mail]</em:id>
> > +    <em:version>20120413.01</em:version>
> 
> Is this the versioning scheme we plan to use for hotfixes? Seems reasonable
> to me

Yes, I proposed it in comment 10 and akeybl agreed[1].

[1] https://etherpad.mozilla.org/arC2CZqj6y

Comment 27

[Matthew N. [:MattN]]

Attachment: v.2 unsigned add-on with billboard (www-dev) on startup every 10 days

The only thing left to change here is the server hostname from www-dev when the billboards are deployed into production plus any review comments.

Comment 28

[Dave Townsend [:mossop]]

(In reply to Matthew N. [:MattN] from comment #26)
> > @@ +27,5 @@
> > > +  checkApplicationVersion(data);
> > > +
> > > +  AddonManager.getAddonByID(data.id, function(addon) {
> > > +    // Auto-enable on install
> > > +    addon.userDisabled = false;
> > 
> > Why is this necessary?
> 
> I'm handling the case where a user disabled a previous hotfix. I think that
> disabling one hotfix shouldn't imply that future hotfixes are unwanted as
> they may be unrelated.

We should probably bake that into the add-ons manager code, could you file a bug for it? Fine to leave it here though.

Comment 29

[Dave Townsend [:mossop]]

Comment on attachment 615547 [details] [diff] [review]
v.2 patch using www-dev

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

::: bootstrap.js
@@ +19,5 @@
> +    // Enable on install in case the user disabled a prior hotfix
> +    addon.userDisabled = false;
> +  });
> +
> +  checkApplicationVersion(data);

There's a race going on here. checkApplicationVersion may call uninstallHotfix which calls getAddonByID which will race with the previous call to getAddonByID here. I think it's highly likely that they'll happen in the order you expect but I'm not sure it's guaranteed so I'd prefer you just did as below, check the result of checkApplicationVersion and only try to enable the add-on if it is not going to be uninstalled. There is no point enabling it if it is getting uninstalled anyway.

@@ +66,5 @@
> +    let billboardWin = getBillboardWindow();
> +    if (billboardWin) {
> +      billboardWin.close();
> +    }
> +  }

If we're going to put out another version of this it can just clear the timer pref in its install method and show its new notification immediately. I don't like the idea of leaving the window open when the code under it has gone away (and been flushed from the cache). There may be unforeseeable problems.

@@ +77,5 @@
> +  },
> +
> +  observe: function(subject, topic, data) {
> +    if (topic == "sessionstore-windows-restored")
> +      setTimeout(showStartupNotification, 5000);

Probably should remove the observer for best practices sake.

I'll leave the 5s question to UX guys, once you've received sessionstore-windows-restored all the main browser windows are open so any window you open should be on top. I wonder if 5s is long enough for a user to actually start doing something and get annoyed by the interruption. Maybe that's what we're going for though!

::: content/unsupportedOS.js
@@ +18,5 @@
> +  evt.preventDefault();
> +  let mainWindow = Services.wm.getMostRecentWindow("navigator:browser");
> +  mainWindow.gBrowser.selectedTab = mainWindow.gBrowser.addTab(evt.target.href);
> +  mainWindow.gBrowser.ownerDocument.defaultView.focus();
> +}

Not for the first time I wish there were an easy way to intercept page loads inside a browser and redirect them elsewhere :(

Comment 30

[:Gavin Sharp [email: [e-mail]]]

Doesn't mainWindow.gBrowser.ownerDocument.defaultView == mainWindow? :)

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 615547 [details] [diff] [review]
v.2 patch using www-dev

>+  let mainWindow = Services.wm.getMostRecentWindow("navigator:browser");
>+  mainWindow.gBrowser.selectedTab = mainWindow.gBrowser.addTab(evt.target.href);
>+  mainWindow.gBrowser.ownerDocument.defaultView.focus();

mainWindow can be a popup. You should just include utilityOverlay.js in unsupportedOS.xul and call openUILinkIn here.

Comment 32

[Matthew N. [:MattN]]

Attachment: v.3 patch using www-dev

UX: Is it fine to show the billboard dialog (attachment 615188 [details]) 3 seconds after the windows restore?

(In reply to Dave Townsend (:Mossop) from comment #28)
> (In reply to Matthew N. [:MattN] from comment #26)
> > > @@ +27,5 @@
> > > > +  checkApplicationVersion(data);
> > > > +
> > > > +  AddonManager.getAddonByID(data.id, function(addon) {
> > > > +    // Auto-enable on install
> > > > +    addon.userDisabled = false;
> > > 
> > > Why is this necessary?
> > 
> > I'm handling the case where a user disabled a previous hotfix. I think that
> > disabling one hotfix shouldn't imply that future hotfixes are unwanted as
> > they may be unrelated.
> 
> We should probably bake that into the add-ons manager code, could you file a
> bug for it? Fine to leave it here though.

Filed bug 746174

(In reply to Dave Townsend (:Mossop) from comment #29)
> Comment on attachment 615547 [details] [diff] [review]
> v.2 patch using www-dev
> @@ +66,5 @@
> > +    let billboardWin = getBillboardWindow();
> > +    if (billboardWin) {
> > +      billboardWin.close();
> > +    }
> > +  }
> 
> If we're going to put out another version of this it can just clear the
> timer pref in its install method and show its new notification immediately.
> I don't like the idea of leaving the window open when the code under it has
> gone away (and been flushed from the cache). There may be unforeseeable
> problems.

Yes, but it doesn't have the same effect because the dialog opened in the new version would be focused.
 
> ::: content/unsupportedOS.js
> @@ +18,5 @@
> > +  evt.preventDefault();
> > +  let mainWindow = Services.wm.getMostRecentWindow("navigator:browser");
> > +  mainWindow.gBrowser.selectedTab = mainWindow.gBrowser.addTab(evt.target.href);
> > +  mainWindow.gBrowser.ownerDocument.defaultView.focus();
> > +}
> 
> Not for the first time I wish there were an easy way to intercept page loads
> inside a browser and redirect them elsewhere :(

Yeah, maybe an |target| attribute on <browser> which specifies the default link target would do the job?

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #30)
> Doesn't mainWindow.gBrowser.ownerDocument.defaultView == mainWindow? :)

I thought when I test mainWindow.focus() that it didn't work but you're right.

(In reply to Dão Gottwald [:dao] from comment #31)
> Comment on attachment 615547 [details] [diff] [review]
> v.2 patch using www-dev
> 
> >+  let mainWindow = Services.wm.getMostRecentWindow("navigator:browser");
> >+  mainWindow.gBrowser.selectedTab = mainWindow.gBrowser.addTab(evt.target.href);
> >+  mainWindow.gBrowser.ownerDocument.defaultView.focus();
> 
> mainWindow can be a popup. You should just include utilityOverlay.js in
> unsupportedOS.xul and call openUILinkIn here.

Done

Comment 33

[Matthew N. [:MattN]]

Attachment: v.3 unsigned add-on with billboard (www-dev) on startup every 10 days

Comment 34

[Axel Hecht [:Pike]]

Comment on attachment 615819 [details] [diff] [review]
v.3 patch using www-dev

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

::: content/unsupportedOS.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +const billboardURL = "https://www-dev.allizom.org/b/firefox/unsupported/win"; // TODO:change to prod server

The comment isn't too specific, thus adding: the /b/ part is only showing up on dev, on the production server that shouldn't be there afaict.

Comment 35

[Justin Dolske [:Dolske]]

Comment on attachment 615819 [details] [diff] [review]
v.3 patch using www-dev

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

Ok, I dialed my nitpick up to 11 but didn't find any major issues, only nitpicks and just-in-case stuff that would be good to fix. Solid work, r+ with that.

BTW, what icon is this using? I didn't apply to get the binary bits.

::: bootstrap.js
@@ +37,5 @@
> +    return;
> +
> +  try {
> +    versionInfo = getWindowsVersion();
> +    // Uninstall if this is not Windows

Hmm, slightly inaccurate comment. We won't get past checkAppVersion() if not windows.

In fact getWindowsVersion() looks to either succeed-or-throw, so you could just eliminate this check (perhaps add a comment to getWindowsVersion() to document that fact explicitly).

@@ +47,5 @@
> +    Cu.reportError(ex);
> +    uninstallHotfix(data);
> +    return;
> +  }
> +  if (isUnsupportedWindows(versionInfo)) {

Nit: insert blank line above.

@@ +51,5 @@
> +  if (isUnsupportedWindows(versionInfo)) {
> +    if (reason == APP_STARTUP) {
> +      showOnStartup = true;
> +      // Delay the billboard on app. startup so that it's shown in the foreground.
> +      Services.obs.addObserver(hotfixObserver, "sessionstore-windows-restored", false);

Did we consider having another long-delay, repeating timer fire to handle people who run their browser for extended periods (days or weeks)? I guess I'm not too worried about that case, though.

@@ +79,5 @@
> +      notifyUnsupportedWindows(versionInfo);
> +  },
> +
> +  observe: function(subject, topic, data) {
> +    if (topic == "sessionstore-windows-restored") {

I am 95% sure this always fires, even when not restoring a session, right?

@@ +92,5 @@
> + * If the hotfix doesn't apply to this application version, uninstall it.
> + *
> + * @return boolean whether the hotfix applies to the application. The hotfix gets uninstalled if false is returned.
> + */
> +function checkApplicationVersion(data) {

Bikeshed: boolean funcs are move obvious when named isFoo() or shouldFoo(). shouldHotfixApp()? It's not just version after all...

Seems like this would also be a bit cleaner if the caller was responsible for calling uninstallHotfix(). (ie, make this idempotent)

@@ +94,5 @@
> + * @return boolean whether the hotfix applies to the application. The hotfix gets uninstalled if false is returned.
> + */
> +function checkApplicationVersion(data) {
> +  let appInfo = Cc["@mozilla.org/xre/app-info;1"].
> +                getService(Ci.nsIXULAppInfo);

Services.appinfo

@@ +96,5 @@
> +function checkApplicationVersion(data) {
> +  let appInfo = Cc["@mozilla.org/xre/app-info;1"].
> +                getService(Ci.nsIXULAppInfo);
> +  let versionChecker = Cc["@mozilla.org/xpcom/version-comparator;1"].
> +                       getService(Ci.nsIVersionComparator);

Services.vc

@@ +99,5 @@
> +  let versionChecker = Cc["@mozilla.org/xpcom/version-comparator;1"].
> +                       getService(Ci.nsIVersionComparator);
> +  // Ensure that this is the correct version in case compatability checking is overridden.
> +  if (versionChecker.compare(appInfo.version, "10.0") < 0
> +      || versionChecker.compare(appInfo.version, "12.*") > 0) {

Operator at end of preceding line please.

@@ +111,5 @@
> +    return false;
> +  }
> +
> +  let xulRuntime = Cc["@mozilla.org/xre/app-info;1"].
> +                   getService(Ci.nsIXULRuntime);

Also Services.appinfo (it QIs to both)

@@ +123,5 @@
> +
> +/**
> + * Based on getServicePack from toolkit/mozapps/update/test/unit/test_0040_general.js
> + */
> +function getWindowsVersion() {

Please file a followup to get this info in nsIXULRuntime. Inevitably something else will be wanting this info in the future.

@@ +129,5 @@
> +  const WORD = ctypes.uint16_t;
> +  const DWORD = ctypes.uint32_t;
> +  const WCHAR = ctypes.jschar;
> +  const BOOL = ctypes.int;
> +

Addan initial sanity check here to bail out if Services.appinfo.OS !== "WINNT".

Even though it's duplicating the explicit "don't install hotfix on not-Windows" elsewhere, we want to be sure if this ever gets called on not-Windows that there is no way for it to succeed. (eg, one _could_ make a Linux kernel32.so that works. Or, more terrifying, a kernel32.so that _crashes_)

@@ +157,5 @@
> +                                        OSVERSIONINFOEXW.ptr);
> +    let winVer = OSVERSIONINFOEXW();
> +    winVer.dwOSVersionInfoSize = OSVERSIONINFOEXW.size;
> +
> +    if(0 === GetVersionEx(winVer.address())) {

Space after "if". === is slight overkill here, but ok.

@@ +177,5 @@
> +
> +/**
> + * Returns whether the passed versionInfo represents an unsupported Windows version.
> + */
> +function isUnsupportedWindows(versionInfo) {

Was going to suggest having this call getWindowsVersion() itself (instead of caller), but as-is would be better for testing.

@@ +207,5 @@
> +
> +  return false;
> +}
> +
> +function getBillboardWindow() Services.wm.getMostRecentWindow("UnsupportedOS:Billboard");

Braces and multiline, please.

Could consider calling it getExistingBillboard() so it's clearer that it doesn't create one.

@@ +233,5 @@
> +  }
> +
> +  let uri = "chrome://firefox-hotfix/content/unsupportedOS.xul";
> +  let openFeatures = "chrome,centerscreen,dialog=no,resizable=no,titlebar,toolbar=no";
> +  versionInfo.wrappedJSObject = versionInfo;

Huh?

::: content/unsupportedOS.js
@@ +9,5 @@
> +
> +/**
> + * Open clicked links for a billboard in the most recent browser window as the selected tab.
> + */
> +function onBillboardClick(evt) {

Do we care about right-clicks? Hmm, probably not.

::: install.rdf
@@ +3,5 @@
> +<RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
> +     xmlns:em="http://www.mozilla.org/2004/em-rdf#">
> +
> +  <Description about="urn:mozilla:install-manifest">
> +    <em:id>[e-mail]</em:id>

Add <em:creator>Mozilla</em:creator>? Want to make sure this looks as official / non-sketchy as possible.

We should consider having a page describing changes in (modest) detail, either via <em:homepage/> or on the normal AMO page. I'd kinda prefer the former (something clearly on mozilla.org/mumble/foo). Again to help anyone concerned make absolutely sure this is a legit addon and not some kind of 3rd party malware.

@@ +11,5 @@
> +    <em:strictCompatibility>true</em:strictCompatibility>
> +
> +    <!-- Front End MetaData -->
> +    <em:name>Firefox Notification Hotfix</em:name>
> +    <em:description>Allows Firefox to push necessary support-related notifications and/or updates to users without having to download a separate update.</em:description>

Two small quibbles to consider:

1) Seems like this could speak more directly to the user reading this, eg "you" vs "users".
2) It's a bit off technically, eg disabling the addon does not disallow Firefox from "pushing necessary etc etc".

"Firefox automatically installed this add-on to give you fixes and support notifications without having to restart your browser." ?

Comment 36

[Justin Dolske [:Dolske]]

Comment on attachment 615819 [details] [diff] [review]
v.3 patch using www-dev

Oh, right, the flag... ahem.

Comment 37

[Matthew N. [:MattN]]

Attachment: v.4 patch using prod. server without locale for Firefox 12

(In reply to Justin Dolske [:Dolske] from comment #35)
> Comment on attachment 615819 [details] [diff] [review]
> v.3 patch using www-dev
>
> BTW, what icon is this using? I didn't apply to get the binary bits.

Icons are from bug 719893 as suggested by comment 0.

> ::: bootstrap.js
> @@ +37,5 @@
> > +    return;
> > +
> > +  try {
> > +    versionInfo = getWindowsVersion();
> > +    // Uninstall if this is not Windows
> 
> Hmm, slightly inaccurate comment. We won't get past checkAppVersion() if not
> windows.

Yeah, I had a Windows check in there originally but decided to move it out to checkAppVersion.

> In fact getWindowsVersion() looks to either succeed-or-throw, so you could
> just eliminate this check (perhaps add a comment to getWindowsVersion() to
> document that fact explicitly).
> 
> 
> @@ +51,5 @@
> > +  if (isUnsupportedWindows(versionInfo)) {
> > +    if (reason == APP_STARTUP) {
> > +      showOnStartup = true;
> > +      // Delay the billboard on app. startup so that it's shown in the foreground.
> > +      Services.obs.addObserver(hotfixObserver, "sessionstore-windows-restored", false);
> 
> Did we consider having another long-delay, repeating timer fire to handle
> people who run their browser for extended periods (days or weeks)? I guess
> I'm not too worried about that case, though.

Yeah, I confirmed in comment 26 that we're not too worried about this.

> @@ +79,5 @@
> > +      notifyUnsupportedWindows(versionInfo);
> > +  },
> > +
> > +  observe: function(subject, topic, data) {
> > +    if (topic == "sessionstore-windows-restored") {
> 
> I am 95% sure this always fires, even when not restoring a session, right?

Yes, it does because IIRC we just create a new session object to restore from on a new session and then restore it.

> @@ +92,5 @@
> > + * If the hotfix doesn't apply to this application version, uninstall it.
> > + *
> > + * @return boolean whether the hotfix applies to the application. The hotfix gets uninstalled if false is returned.
> > + */
> > +function checkApplicationVersion(data) {
> 
> Bikeshed: boolean funcs are move obvious when named isFoo() or shouldFoo().
> shouldHotfixApp()? It's not just version after all...

Informally I consider builds for different channels and platforms as different versions of the application.
 
> @@ +123,5 @@
> > +
> > +/**
> > + * Based on getServicePack from toolkit/mozapps/update/test/unit/test_0040_general.js
> > + */
> > +function getWindowsVersion() {
> 
> Please file a followup to get this info in nsIXULRuntime. Inevitably
> something else will be wanting this info in the future.



> @@ +129,5 @@
> > +  const WORD = ctypes.uint16_t;
> > +  const DWORD = ctypes.uint32_t;
> > +  const WCHAR = ctypes.jschar;
> > +  const BOOL = ctypes.int;
> > +
> 
> Addan initial sanity check here to bail out if Services.appinfo.OS !==
> "WINNT".

OK, I guess I'll put it back :)

> @@ +157,5 @@
> > +                                        OSVERSIONINFOEXW.ptr);
> > +    let winVer = OSVERSIONINFOEXW();
> > +    winVer.dwOSVersionInfoSize = OSVERSIONINFOEXW.size;
> > +
> > +    if(0 === GetVersionEx(winVer.address())) {
> 
> Space after "if". === is slight overkill here, but ok.

Per the comment, I didn't write most of this function.
 
> @@ +233,5 @@
> > +  }
> > +
> > +  let uri = "chrome://firefox-hotfix/content/unsupportedOS.xul";
> > +  let openFeatures = "chrome,centerscreen,dialog=no,resizable=no,titlebar,toolbar=no";
> > +  versionInfo.wrappedJSObject = versionInfo;
> 
> Huh?

https://developer.mozilla.org/en/Working_with_windows_in_chrome_code#Example_5:_Using_nsIWindowWatcher_for_passing_an_arbritrary_JavaScript_object

> ::: install.rdf
> @@ +3,5 @@
> > +<RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
> > +     xmlns:em="http://www.mozilla.org/2004/em-rdf#">
> > +
> > +  <Description about="urn:mozilla:install-manifest">
> > +    <em:id>[e-mail]</em:id>
> 
> Add <em:creator>Mozilla</em:creator>? Want to make sure this looks as
> official / non-sketchy as possible.

I thought that AMO adds metadata to the AOM details but it won't hurt here.

> We should consider having a page describing changes in (modest) detail,
> either via <em:homepage/> or on the normal AMO page. I'd kinda prefer the
> former (something clearly on mozilla.org/mumble/foo). Again to help anyone
> concerned make absolutely sure this is a legit addon and not some kind of
> 3rd party malware.

That makes sense. Maybe file a bug on mozilla.org/firefox?

> @@ +11,5 @@
> > +    <em:strictCompatibility>true</em:strictCompatibility>
> > +
> > +    <!-- Front End MetaData -->
> > +    <em:name>Firefox Notification Hotfix</em:name>
> > +    <em:description>Allows Firefox to push necessary support-related notifications and/or updates to users without having to download a separate update.</em:description>
> 
> Two small quibbles to consider:
> 
> 1) Seems like this could speak more directly to the user reading this, eg
> "you" vs "users".
> 2) It's a bit off technically, eg disabling the addon does not disallow
> Firefox from "pushing necessary etc etc".
> 
> "Firefox automatically installed this add-on to give you fixes and support
> notifications without having to restart your browser." ?

This was something put together quickly in https://etherpad.mozilla.org/arC2CZqj6y

How about: "Delivers necessary support-related notifications and/or updates for Firefox without requiring a software update."?

Comment 38

[Matthew N. [:MattN]]

Attachment: Screenshot of dialog and AOM details page

Comment 39

[Justin Dolske [:Dolske]]

(In reply to Matthew N. [:MattN] from comment #37)

> > > +  let uri = "chrome://firefox-hotfix/content/unsupportedOS.xul";
> > > +  let openFeatures = "chrome,centerscreen,dialog=no,resizable=no,titlebar,toolbar=no";
> > > +  versionInfo.wrappedJSObject = versionInfo;
> > 
> > Huh?
> 
> https://developer.mozilla.org/en/
> Working_with_windows_in_chrome_code#Example_5:
> _Using_nsIWindowWatcher_for_passing_an_arbritrary_JavaScript_object

Whaaaa... Mossop!!!

That feels like xpconnect abuse, wonder if that's guaranteed to stay working. Will ponder more tomorrow.

Comment 40

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 616455 [details] [diff] [review]
v.4 patch using prod. server without locale for Firefox 12

>diff --git a/bootstrap.js b/bootstrap.js

>+function shouldHotfixApp() {
>+  // Ensure that this is the correct version in case compatability checking is overridden.
>+  if (Services.vc.compare(Services.appinfo.version, "12.0") < 0 ||
>+      Services.vc.compare(Services.appinfo.version, "12.*") > 0) {
>+    // Version is not between 10 and 12.* so uninstall.

Is this a typo? Or the comment is just out of date?

>+  if (Services.prefs.getCharPref("app.update.channel") == "esr") {
>+    return false;

You should probably check the default pref branch, since that's what the update service uses.

Comment 41

[Matthew N. [:MattN]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #40)
> Comment on attachment 616455 [details] [diff] [review]
> v.4 patch using prod. server without locale for Firefox 12
> 
> >diff --git a/bootstrap.js b/bootstrap.js
> 
> >+function shouldHotfixApp() {
> >+  // Ensure that this is the correct version in case compatability checking is overridden.
> >+  if (Services.vc.compare(Services.appinfo.version, "12.0") < 0 ||
> >+      Services.vc.compare(Services.appinfo.version, "12.*") > 0) {
> >+    // Version is not between 10 and 12.* so uninstall.
> 
> Is this a typo? Or the comment is just out of date?

Just out-of-date because we're making two versions and I'm switching between them.  10.0 - 12.*  and 12.0 - 12.*

> >+  if (Services.prefs.getCharPref("app.update.channel") == "esr") {
> >+    return false;
> 
> You should probably check the default pref branch, since that's what the
> update service uses.

Changed to he following, ok?

diff --git a/bootstrap.js b/bootstrap.js
--- a/bootstrap.js
+++ b/bootstrap.js
@@ -95,21 +95,22 @@ let hotfixObserver = {
 
 /**
  * @return boolean whether the hotfix applies to the application.
  */
 function shouldHotfixApp() {
   // Ensure that this is the correct version in case compatability checking is overridden.
   if (Services.vc.compare(Services.appinfo.version, "12.0") < 0 ||
       Services.vc.compare(Services.appinfo.version, "12.*") > 0) {
-    // Version is not between 10 and 12.* so uninstall.
+    // Version is not between 12 and 12.* so uninstall.
     return false;
   }
 
-  if (Services.prefs.getCharPref("app.update.channel") == "esr") {
+  let defaultPrefs = Services.prefs.getDefaultBranch("");
+  if (defaultPrefs.getCharPref("app.update.channel") == "esr") {
     return false;
   }
 
   if (Services.appinfo.OS !== "WINNT") {
     return false;
   }
 
   return true;

Comment 42

[Alex Keybl [:akeybl]]

https://bugzilla.mozilla.org/attachment.cgi?id=616695&action=edit (the XPI targeting only FF12) is now live on production! Beta users will start receiving notification shortly.

Comment 43

[Laura Mesa [:lmesa] [:lvmesa]]

I'm going to go ahead and close this as it seems like everything is fixed. Feel free to reopen if I missed something.

Comment 44

[u279076]

I've verified that the new hotfix, which fixes the reprompting issues reported earlier, is now working. I tested Windows 2000 and Windows XP SP2. Windows 2000 gets the hotfix, Windows XP SP2 does not.

Hotfix: 20120430.01

Comment 45

[Matthew N. [:MattN]]

Committed: https://hg.mozilla.org/users/dtownsend_mozilla.com/hotfixes/rev/1e8236d9d338