Closed Bug 1386176 Opened 7 years ago Closed 7 years ago

Provide value in update url to indicate whether a Firefox 32 bit install hasn't been migrated to Firefox 64 bit previously

Categories

(Toolkit :: Application Update, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 + fixed
firefox57 --- fixed

People

(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)

References

Details

Attachments

(2 files, 6 obsolete files)

This will allow us to be able to tell if a system has been migrated as well as allow clients to add a registry entry to prevent migration.
Ben, do you have a preference of where this value exists in the update url. Also, I would like this value to be optional since it isn't necessary for an Windows install that has always been Firefox 64 bit and it isn't needed for other platforms.

The value will only be present on Firefox 32 bit and it will be along the lines of
mig64=0 the system hasn't been migrated previously
mig64=1 the system has already been migrated previously and shouldn't be migrated to Firefox 64 bit
Flags: needinfo?(bhearsum)
Attached patch 1 - installer patch (obsolete) — Splinter Review
Matt, this is what I had in mind for the installer. I'm still working on the patch for app update.
Attachment #8892365 - Flags: review?(mhowell)
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #1)
> Ben, do you have a preference of where this value exists in the update url.
> Also, I would like this value to be optional since it isn't necessary for an
> Windows install that has always been Firefox 64 bit and it isn't needed for
> other platforms.
> 
> The value will only be present on Firefox 32 bit and it will be along the
> lines of
> mig64=0 the system hasn't been migrated previously
> mig64=1 the system has already been migrated previously and shouldn't be
> migrated to Firefox 64 bit

Ben, it would be much simpler if this could be a param on the url. Is that an option for this?
Comment on attachment 8892365 [details] [diff] [review]
1 - installer patch

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

I'm a bit unsure about the name Migrate32BitTo64Bit for the registry value; it seems like the idea is that the value is set to 1 to show that a migration has occurred, or, I'm guessing, this is the value a user would set to disable the migration. Especially in the latter case, this name seems exactly backwards and kind of confusing. I might also be misunderstanding what's going on, in which case the name isn't helping me get it right.
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #3)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #1)
> > Ben, do you have a preference of where this value exists in the update url.
> > Also, I would like this value to be optional since it isn't necessary for an
> > Windows install that has always been Firefox 64 bit and it isn't needed for
> > other platforms.
> > 
> > The value will only be present on Firefox 32 bit and it will be along the
> > lines of
> > mig64=0 the system hasn't been migrated previously
> > mig64=1 the system has already been migrated previously and shouldn't be
> > migrated to Firefox 64 bit
> 
> Ben, it would be much simpler if this could be a param on the url. Is that
> an option for this?

Putting this in %OS_VERSION% would be simplest on the Balrog side, but if it's easier or cleaner for you if it's a query param, we can do that - no big deal.
Flags: needinfo?(bhearsum)
Attached patch 1 - installer patch (obsolete) — Splinter Review
Changed the key name to 32to64DidMigrate per our irc convo.
Attachment #8892823 - Flags: review?(mhowell)
Attached patch 1 - installer patch (obsolete) — Splinter Review
This handles a couple of other cases
Attachment #8892365 - Attachment is obsolete: true
Attachment #8892823 - Attachment is obsolete: true
Attachment #8892365 - Flags: review?(mhowell)
Attachment #8892823 - Flags: review?(mhowell)
Attachment #8892825 - Flags: review?(mhowell)
[Tracking Requested - why for this release]:
This is in support of bug 1274659.

This will make it possible to differentiate between a Windows Firefox 32 bit installation that has been migrated to Firefox 64 bit and then the client chose to reinstall Firefox 32 bit again so we don't migrate them a second time. This also allows configuring a system so it never migrates from Firefox 32 bit to Firefox 64 bit which might be handy for some enterprise clients.
Attached patch 2 - app update patch (obsolete) — Splinter Review
Forgot about the no unused var lint rule
Attachment #8892864 - Attachment is obsolete: true
Attachment #8892864 - Flags: review?(mhowell)
Attachment #8892877 - Flags: review?(mhowell)
Attached patch 2 - app update patch (obsolete) — Splinter Review
Also forgot to qrefresh
Attachment #8892877 - Attachment is obsolete: true
Attachment #8892877 - Flags: review?(mhowell)
Attachment #8892878 - Flags: review?(mhowell)
Pushed a win32 only try since the first push should cover other platforms
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5ae95eb6b5cc499267b5fe77b5614f72c2bd91c
Comment on attachment 8892825 [details] [diff] [review]
1 - installer patch

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

Thanks!
Attachment #8892825 - Flags: review?(mhowell) → review+
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #0)
> This will allow us to be able to tell if a system has been migrated as well
> as allow clients to add a registry entry to prevent migration.

What will mig64 be set to for systems that have been migrated, and then paved over with a 32-bit install? I'm assuming "1", but I just want to be certain.
Flags: needinfo?(robert.strong.bugs)
Comment on attachment 8892878 [details] [diff] [review]
2 - app update patch

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

We're going to need to have some user documentation, in a SUMO article or something, for how to use these registry values and how to reinstall to avoid getting migrated again. Not sure if that's being tracked in another bug already, but it needs to be done somewhere.

::: toolkit/mozapps/update/nsUpdateService.js
@@ +2821,5 @@
> +      return false;
> +    }
> +
> +    let aryABI = UpdateUtils.ABI.split("-");
> +    if (aryABI[0] != "x86" || aryABI[2] != "x64") {

Took me a minute to figure out that aryABI[0] is the application architecture and aryABI[2] is the OS architecture, so this is checking for 32-bit Firefox running on 64-bit Windows and returning early otherwise. Would be good to explain that better, either with a comment or by giving names to those two values.
Attachment #8892878 - Flags: review?(mhowell) → review+
(In reply to Ben Hearsum (:bhearsum) from comment #14)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #0)
> > This will allow us to be able to tell if a system has been migrated as well
> > as allow clients to add a registry entry to prevent migration.
> 
> What will mig64 be set to for systems that have been migrated, and then
> paved over with a 32-bit install? I'm assuming "1", but I just want to be
> certain.

Doing the 32-bit paveover is a supported way to opt out of further migrations, so that install won't send the mig64 parameter.
Flags: needinfo?(robert.strong.bugs)
(In reply to Matt Howell [:mhowell] from comment #16)
> (In reply to Ben Hearsum (:bhearsum) from comment #14)
> > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> > comment #0)
> > > This will allow us to be able to tell if a system has been migrated as well
> > > as allow clients to add a registry entry to prevent migration.
> > 
> > What will mig64 be set to for systems that have been migrated, and then
> > paved over with a 32-bit install? I'm assuming "1", but I just want to be
> > certain.
> 
> Doing the 32-bit paveover is a supported way to opt out of further
> migrations, so that install won't send the mig64 parameter.

Paveovers won't send mig64 at all? This means that we need to treat the absence of mig64 the same as mig64=1, if I've understood correctly?
Flags: needinfo?(mhowell)
(In reply to Ben Hearsum (:bhearsum) from comment #17)
> Paveovers won't send mig64 at all? This means that we need to treat the
> absence of mig64 the same as mig64=1, if I've understood correctly?

No, no, sorry; paving over a migrated 64-bit install with a 32-bit install signals that you don't want to be migrated. So mig64 gets left off in that case, so that we avoid migrating that user again.
Flags: needinfo?(mhowell)
(In reply to Matt Howell [:mhowell] from comment #15)
> Comment on attachment 8892878 [details] [diff] [review]
> 2 - app update patch
> 
> Review of attachment 8892878 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We're going to need to have some user documentation, in a SUMO article or
> something, for how to use these registry values and how to reinstall to
> avoid getting migrated again. Not sure if that's being tracked in another
> bug already, but it needs to be done somewhere.
cpeterson will be handling this.

> ::: toolkit/mozapps/update/nsUpdateService.js
> @@ +2821,5 @@
> > +      return false;
> > +    }
> > +
> > +    let aryABI = UpdateUtils.ABI.split("-");
> > +    if (aryABI[0] != "x86" || aryABI[2] != "x64") {
> 
> Took me a minute to figure out that aryABI[0] is the application
> architecture and aryABI[2] is the OS architecture, so this is checking for
> 32-bit Firefox running on 64-bit Windows and returning early otherwise.
> Would be good to explain that better, either with a comment or by giving
> names to those two values.
Will add a comment for that
I changed ${GetLongPath} "$INSTDIR\${FileMainEXE}" $1 to ${GetLongPath} "$INSTDIR" $1 in the installer patch and got mhowell's r+ for this change over irc.
Attachment #8892825 - Attachment is obsolete: true
Attachment #8893105 - Flags: review+
Added requested comment
Attachment #8892878 - Attachment is obsolete: true
Attachment #8893106 - Flags: review+
It looks like adding mig64=1 at this moment makes it so no update xml file is returned so I am going to hold off on landing this bug until bug 1386756 makes it so the server advertises updates when it is present.
cpeterson, the way this works is as follows:

To disable migration for all installations on a client system for all users the following registry entry should be added
Path: HKEY_LOCAL_MACHINE\SOFTWARE\Mozilla\Firefox\32to64DidMigrate
Value Type: DWORD (32 bit) Value
Value Name: Never
Value: 1

To disable migration for all installations on a client system only for the current user the following registry entry should be added
Path: HKEY_CURRENT_USER\SOFTWARE\Mozilla\Firefox\32to64DidMigrate
Value Type: DWORD (32 bit) Value
Value Name: Never
Value: 1

To disable migration for a single installation on a client system for all users the following registry entry should be added
Path: HKEY_LOCAL_MACHINE\SOFTWARE\Mozilla\Firefox\32to64DidMigrate
Value Type: DWORD (32 bit) Value
Value Name: <Windows path to the installation without a trailing backslash>
example: C:\Program Files (x86)\Mozilla Firefox
Value: 1

To disable migration for a single installation on a client system only for the current user the following registry entry should be added
Path: HKEY_CURRENT_USER\SOFTWARE\Mozilla\Firefox\32to64DidMigrate
Value Type: DWORD (32 bit) Value
Value Name: <Windows path to the installation without a trailing backslash>
example: C:\Program Files (x86)\Mozilla Firefox
Value: 1
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #22)
> It looks like adding mig64=1 at this moment makes it so no update xml file
> is returned so I am going to hold off on landing this bug until bug 1386756
> makes it so the server advertises updates when it is present.

The fact that an unknown query arg causes this seems bad - I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1325377 on it, which might be fixed sooner than bug 1386756.
Summary: Provide value in update url to indicate whether a Firefox 32 bit install can be migrated to Firefox 64 bit → Provide value in update url to indicate whether a Firefox 32 bit install hasn't been migrated to Firefox 64 bit previously
From talking with rstrong today, it should be OK to wait for beta 4 to uplift.  Should we ask for testing here on Nightly (once the patches land?)
Flags: needinfo?(cpeterson)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #26)
> From talking with rstrong today, it should be OK to wait for beta 4 to
> uplift.  Should we ask for testing here on Nightly (once the patches land?)

I don't think so. We don't plan to migrate 32-bit Nightly users to 64-bit so there isn't much in these migration patches we can test on Nightly.
Flags: needinfo?(cpeterson)
https://hg.mozilla.org/mozilla-central/rev/09f9d9c51836
https://hg.mozilla.org/mozilla-central/rev/c14c05b4baa0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8893105 [details] [diff] [review]
1 - installer patch

Approval Request Comment
[Feature/Bug causing the regression]: Not a regression
[User impact if declined]: When Windows Firefox clients are migrated from 32 bit to 64 bit Firefox as part of bug 1274659 they could get migrated a second time if they choose to reinstall Firefox 32 bit
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: This has landed in Nightly and I manually verified that it works correctly.
[Needs manual test from QE? If yes, steps to reproduce]: This will be done as part of the verification of bug 1274659 in beta 7
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: This has been manually verified and only adds a query param to the update url based on a registry key value.
[String changes made/needed]: None
Attachment #8893105 - Flags: approval-mozilla-beta?
Comment on attachment 8893106 [details] [diff] [review]
2 - app update patch

Approval Request Comment
[Feature/Bug causing the regression]: Not a regression
[User impact if declined]: When Windows Firefox clients are migrated from 32 bit to 64 bit Firefox as part of bug 1274659 they could get migrated a second time if they choose to reinstall Firefox 32 bit
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: This has landed in Nightly and I manually verified that it works correctly.
[Needs manual test from QE? If yes, steps to reproduce]: This will be done as part of the verification of bug 1274659 in beta 7
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: This has been manually verified and only adds a query param to the update url based on a registry key value.
[String changes made/needed]: None
Attachment #8893106 - Flags: approval-mozilla-beta?
Comment on attachment 8893105 [details] [diff] [review]
1 - installer patch

We need this and other installer patches for the 64-bit watershed/migration from beta 3 to higher.
Attachment #8893105 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8893106 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.