737833 - Implement install/uninstall/helper changes in Fx desktop installer for Fx metro

Status: RESOLVED FIXED

(Firefox :: Installer, defect)

Description

[Jim Mathies [:jimm]]

Currently we only do zip builds. We need an nsis based installer/uninstall. We will also need a helper.exe that can elevate and register / set the default browser, and create the needed start menu link with the proper properties.

Comment 1

[Jim Mathies [:jimm]]

Install should also support setting fennec as the default metro browser via Windows Default Programs control panel.

Comment 2

[Jim Mathies [:jimm]]

http://hg.mozilla.org/projects/elm/rev/3ede5c480150
http://hg.mozilla.org/projects/elm/rev/718f2e44e580
http://hg.mozilla.org/projects/elm/rev/ae09726fd6be

Comment 3

[Jim Mathies [:jimm]]

Attachment: changes patch

This is a diff between the firefox install files I copied up into mobile to get the install working. All in all the changes aren't that major. Essentially we stepped away from the use of the FirefoHTML and FirefoxHTTP keys, which still get used by desktop, and created a new set of keys that match what we needed for the metro registration. Our launcher currently uses the path from FirefoxHTML for finding the currently registered browser when launching things like links. More on this below.

One nice thing about this setup, everything in the registry starts with Mozilla, so the keys are all grouped together, which makes debugging easier.

For an example of how this all gets populated there are registry scripts currently generated during the build for zip installs - 

http://mxr.mozilla.org/projects-central/source/elm/mobile/xul/metro/

and for a look at how the launcher works - 

http://mxr.mozilla.org/projects-central/source/elm/mobile/xul/metro/commandexecutehandler/

Comment 4

[Jim Mathies [:jimm]]

Also, the launcher relies on installer generated keys. I've added some comments about this in the code - 

http://mxr.mozilla.org/projects-central/source/elm/mobile/xul/metro/commandexecutehandler/CommandExecuteHandler.cpp#256

Comment 5

[Jim Mathies [:jimm]]

The big difference here is that with the fennec install I created two fresh handler entries under Mozilla.Firefox.HTML and Mozilla.Firefox.URL. Firefox uses FirefoxHTML and FirefoxURL. The idea was to steer clear of fx on the desktop so they didn't conflict. The entries under each are slightly different. The goal would be to blend metro handler data in with the desktop entries without breaking either browser. 

The comments in the posted patch explain the various steps, and you can pretty much follow those along with the registry scripts which do the same thing.

Also, the profile directory for each browser needs to be separate. So there may be a naming conflict in the variables passed from build to the nsis builder.

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Core metro browser registration

I'll get an sr from rstrong after you do the initial review.  I decided not to change from the existing progids to avoid regressions.

Comment 7

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Win8 logicLib checks

Comment 8

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Zip install changes

Comment 9

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Disallow appusermodelid registration from widget code

Comment 10

[Brian R. Bondy [:bbondy]]

Tomorrow I'll try with a fresh win8 install, and also I'll try on win7.

Comment 11

[Jim Mathies [:jimm]]

Comment on attachment 659950 [details] [diff] [review]
Patch v1 - Zip install changes

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

::: browser/metro/shell/installscripts/registerdefaultbrowser.reg.in
@@ +86,5 @@
>  "FriendlyTypeName"="Firefox URL"
>  "URL Protocol"=""
>  "EditFlags"=dword:00000002
>  
> +[HKEY_CLASSES_ROOT\sFirefoxURL\Application]

Is the 's' a typo?

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 659948 [details] [diff] [review]
Patch v1 - Core metro browser registration

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

Looks good to me.

::: browser/installer/windows/nsis/shared.nsh
@@ +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/.
>  
> +!define DELEGATE_EXCUTE_HANDLER_ID {5100FEC1-212B-4BF5-9BF8-3E650FD794A3}

nit - this needs a comment

::: toolkit/mozapps/installer/windows/nsis/common.nsh
@@ +1575,5 @@
> +!macro AddMetroBrowserHandlerValues DELEGATE_EXECUTE_HANDLER_ID \
> +                                    DELEGATE_EXECUTE_HANDLER_PATH \
> +                                    APP_USER_MODEL_ID \
> +                                    PROTOCOL_ACTIVATION_ID \
> +                                    FILE_ACTIVATION_ID 

nit - splinter shows white space after 'FILE_ACTIVATION_ID'

Comment 13

[Jim Mathies [:jimm]]

Can we update unregisterbrowser.reg as well?

Comment 14

[Brian R. Bondy [:bbondy]]

Thanks, yup the s is a typo, I hadn't tested the zip setup yet. 
I'll update the zip patch to include unregisterbrowser.reg.

Comment 15

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Core metro browser registration

Fixed missing E typo on DELEGATE_EX(E)CUTE_ID causing uninstaller to not remove the wow64 registration.

Comment 16

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Zip install changes

- Explicitly say which of HKLM or HKCU will be written to so it is more deterministic
- Synced up several things to what the installer does
- Added uninstall bits

Comment 17

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/projects/elm/rev/437d324102e7
http://hg.mozilla.org/projects/elm/rev/94121416115e
http://hg.mozilla.org/projects/elm/rev/6f2389dd45b8
Awaiting landing is the zip patch

Comment 18

[Brian R. Bondy [:bbondy]]

Comment on attachment 660181 [details] [diff] [review]
Patch v2 - Core metro browser registration

I'm not sure if we'll land this on m-c right away or not, depending on if it has no known side effects on win8 with a non metro build. But this review request is for correctness, and for landing on m-c as long as I don't find any show stoppers from testing with non elm builds.

Comment 19

[Jim Mathies [:jimm]]

Comment on attachment 660182 [details] [diff] [review]
Patch v2 - Zip install changes

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

::: browser/metro/shell/installscripts/registerdefaultbrowser.reg.in
@@ +16,5 @@
> +[-HKEY_CURRENT_USER\SOFTWARE\Classes\AppID\{5100FEC1-212B-4BF5-9BF8-3E650FD794A3}]
> +[-HKEY_CURRENT_USER\SOFTWARE\CLSID\{5100FEC1-212B-4BF5-9BF8-3E650FD794A3}]
> +[-HKEY_CURRENT_USER\SOFTWARE\Classes\Mozilla.MetroFirefox.Default]
> +[-HKEY_CURRENT_USER\SOFTWARE\Classes\FirefoxURL]
> +[-HKEY_CURRENT_USER\SOFTWARE\Classes\FirefoxHTML]

I'm confused about this, everything below is being added to HKEY_LOCAL_MACHINE.

Comment 20

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Fix helper.exe path after resource splitting bug (Elm only)

Comment 21

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/projects/elm/rev/f3570dfef0ed

Comment 22

[Jim Mathies [:jimm]]

Comment on attachment 660182 [details] [diff] [review]
Patch v2 - Zip install changes

Lets just get rid of the manual registration scripts and rely on the desktop browser for setup. I filed bug 790422 on this.

Comment 23

[Jim Mathies [:jimm]]

Comment on attachment 660213 [details] [diff] [review]
Patch v1 - Fix helper.exe path after resource splitting bug (Elm only)

Approval for elm only. I think we should get bug 789461 fixed and use the new constant on mc.

Comment 24

[Robert Strong (they/them - no direct email)]

Comment on attachment 659949 [details] [diff] [review]
Patch v1 - Win8 logicLib checks

It looks like there is duplicate code in overrides.nsh for WINVER_7. Could you add this to overrides.nsh instead and remove the WINVER_7 code from common.nsh? It should also be simple to add WINVER_8 support to the !ifndef _WINVER_VERXBIT block in overrides.nsh for NSIS 2.33 support and then a small !ifndef WINVER_8 block for NSIS 2.46 support which should use the following since that is the format used in NSIS 2.46.
!define WINVER_8         0x06020000 ;6.02.????

Comment 25

[Robert Strong (they/them - no direct email)]

Comment on attachment 660181 [details] [diff] [review]
Patch v2 - Core metro browser registration

So, if I have two installs, one is set as default, and I uninstall the other this will remove the metro values for the one that is set as default.

Does setting dual mode for the shortcuts on Win7 have any side affects?

Comment 26

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #25)
> Comment on attachment 660181 [details] [diff] [review]
> Patch v2 - Core metro browser registration
> 
> So, if I have two installs, one is set as default, and I uninstall the other
> this will remove the metro values for the one that is set as default.

Ya I knew of that one but I was going to do that in a follow up since that's a non issue for the metro preview, but I'll do it here.

> Does setting dual mode for the shortcuts on Win7 have any side affects?

None seen yet but I'm not done testing on win7 yet.

> ...overrides.nsh... 

Sounds good, I did it that way because the win8 check is needed independent of _WINVER_VERXBIT, but the workaround you mentioned sounds like it'll work nicely.

Comment 27

[Robert Strong (they/them - no direct email)]

(In reply to Brian R. Bondy [:bbondy] from comment #26)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #25)
> > Comment on attachment 660181 [details] [diff] [review]
> > Patch v2 - Core metro browser registration
> > 
> > So, if I have two installs, one is set as default, and I uninstall the other
> > this will remove the metro values for the one that is set as default.
> 
> Ya I knew of that one but I was going to do that in a follow up since that's
> a non issue for the metro preview, but I'll do it here.
I'm fine with that if you would prefer to do it in a followup.

Comment 28

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Win8 logicLib checks

Comment 29

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3 - Core metro browser registration

Comment 30

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - CEH (elm only)

Split out CEH from installer patch since it'll be landing on m-c as part of the bigger elm merge.

Comment 31

[Robert Strong (they/them - no direct email)]

Comment on attachment 661092 [details] [diff] [review]
Patch v2 - Win8 logicLib checks

I'd go with putting the missing WINVER_7, WINVER_2008R2, and WINVER_8 which are missing from NSIS 2.33 WinVer.nsh inside the _WINVER_VERXBIT block using the following since NSIS 2.33's WinVer.nsh only takes 2 parameters whereas NSIS 2.46's WinVer.nsh takes 3 parameters:
!define WINVER_7 0x601
!define WINVER_2008R2 0x601
!define WINVER_8 0x602
etc.
!insertmacro __WinVer_DefineOSTest AtLeast WINVER_8
!insertmacro __WinVer_DefineOSTest Is 8
!insertmacro __WinVer_DefineOSTest AtMost 8
etc.
...
+!endif # _WINVER_VERXBIT

Then outside the _WINVER_VERXBIT block go with something like
!ifndef WINVER_8
!define WINVER_8 0x06020000 ;6.02.????
!insertmacro __WinVer_DefineOSTest AtLeast 8 ""
!insertmacro __WinVer_DefineOSTest Is 8 ""
!insertmacro __WinVer_DefineOSTest AtMost 8 ""
!endif

I'm fine if you want to simplify it with a helper macro.

r- just to take one quick look with these changes.

Comment 32

[Robert Strong (they/them - no direct email)]

Comment on attachment 661124 [details] [diff] [review]
Patch v3 - Core metro browser registration

The RegCleanAppHandler and RegCleanProtocolHandler macros shows how to cleanup orphaned entries which would be nice to include here as well.

If that isn't possible for these registry entries please let me know how and just request r? for this patch.

Comment 33

[Brian R. Bondy [:bbondy]]

ah! I was completely confused why some of the existing code only had 2 parameters, that makes sense :)

Comment 34

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3 - Win8 logicLib checks

Fixed nsis 2.33 support

Comment 35

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4 - Core metro browser registration

OK I think this is correct now.  I check the delegate execute handler COM server location, if it's set to the current channel (in my case Nightly), then it removes it.  Otherwise it doesn't.  I tried with 2 installs and it is working correctly.

The last patch did something similar but was dependent on FirefoxURL for the delegate execute handler.

Comment 36

[Robert Strong (they/them - no direct email)]

Comment on attachment 661208 [details] [diff] [review]
Patch v3 - Win8 logicLib checks

Looks good

Could you file a bug to verify that all builders have NSIS 2.46 and remove the code for NSIS 2.33 support?

Comment 37

[Robert Strong (they/them - no direct email)]

Comment on attachment 661276 [details] [diff] [review]
Patch v4 - Core metro browser registration

Looks ok. I wonder why you aren't doing FileExists checks and removing the registry entries when they don't as I suggested in comment #32?

Comment 38

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #37)
> Comment on attachment 661276 [details] [diff] [review]
> Patch v4 - Core metro browser registration
> 
> Looks ok. I wonder why you aren't doing FileExists checks and removing the
> registry entries when they don't as I suggested in comment #32?

Sorry I wasn't exactly sure what you meant by orphaned entries, I had only looked at one of your 2 examples (RegCleanProtocolHandler) and assumed both examples were following siilar formats.  I see what you mean now and I think that checking if the file doesn't exist and clearing the entries in that case is a good idea.  I'll do an update shortly.

Comment 39

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Add support for removing orphaned DEH entries

Comment 40

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Only do metro registration for metro builds

This patch just makes sure that people that don't use metro builds won't try to rpregister as a metro browser. Leaving the properties on the shortcuts the same since that doesn't matter.

With this patch, if r+ed, then I plan to land all of this on m-c after final testing on win7 and win8 m-c builds.

Comment 41

[Brian R. Bondy [:bbondy]]

(In reply to Brian R. Bondy [:bbondy] from comment #20)
> Created attachment 660213 [details] [diff] [review]
> Patch v1 - Fix helper.exe path after resource splitting bug

Jim you don't want this one on m-c right? If so, should I file a follow up bug blocking landing metro bits on m-c to fix this?

Comment 42

[Robert Strong (they/them - no direct email)]

Could you also add the following to the StartInstallLog macro in common.nsh?
       ${ElseIf} ${IsWinVista}
         ${LogMsg} "OS Name    : Windows Vista"
-      ${ElseIf} ${AtLeastWinVista} ; Workaround for NSIS 2.33 WinVer.nsh not knowing Win7
-        ${LogMsg} "OS Name    : Windows 7 or above"
+      ${ElseIf} ${IsWin7}
+        ${LogMsg} "OS Name    : Windows 7"
+      ${ElseIf} ${AtLeastWin8}
+        ${LogMsg} "OS Name    : Windows 8 or above"
       ${Else}
         ${LogMsg} "OS Name    : Unable to detect"

Comment 43

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - NSIS log OS detection

Slight change, it would be good to know for when the next version of windows pops up, the difference between win8, a bug in the nsis code for a version in between, and post win8.

Comment 44

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/projects/elm/rev/1b19a8b61ffe
(will sync review comments to elm)

Comment 45

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #36)
> Comment on attachment 661208 [details] [diff] [review]
> Patch v3 - Win8 logicLib checks
> 
> Looks good
> 
> Could you file a bug to verify that all builders have NSIS 2.46 and remove
> the code for NSIS 2.33 support?

Yup, that work is being tracked in bug 791687.  It should land after this task is reviewed&landed, and after catlee or aki confirms we don't use NSIS 2.33 at all.

Comment 46

[Robert Strong (they/them - no direct email)]

As a side note, it is fine to use LogicLib in common.nsh if you prefer.

Comment 47

[Robert Strong (they/them - no direct email)]

Comment on attachment 661420 [details] [diff] [review]
Patch v1 - Only do metro registration for metro builds

Add a new line to the end of common.nsh.

After Metro support is complete will this be removed?

Comment 48

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #47)
> Comment on attachment 661420 [details] [diff] [review]
> Patch v1 - Only do metro registration for metro builds
> 
> Add a new line to the end of common.nsh.
> 
> After Metro support is complete will this be removed?

I think we'll keep the flag, it'll be useful to be able to compile with no metro support at all.  Also the flag is used in other places in the tree that is shared for different products.  For the actual Firefox installer code, if someone really wants to use an open source tool to compile, and build an installer, and make a patch, they still can.

Comment 49

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4 - Win8 logicLib checks

Rebased win8 check patch since removing 2.33 support will land first.
Carrying forward r+.

Comment 50

[Brian R. Bondy [:bbondy]]

Attachment: Patch v5 - Core metro browser registration

Removed an extra DEH entry that can cause problems when uninstalling a metro build followed by installing a non metro build.  Carrying forward r+.

Comment 51

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1' - Add support for removing orphaned DEH entries

rebased

Comment 52

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/mozilla-central/rev/e5198b6afda0
http://hg.mozilla.org/mozilla-central/rev/451c73598cc7
http://hg.mozilla.org/mozilla-central/rev/d212c1a845e2
http://hg.mozilla.org/mozilla-central/rev/b7b2e59c6237
http://hg.mozilla.org/mozilla-central/rev/4b9af52437cf
http://hg.mozilla.org/mozilla-central/rev/abbac9696665

Comment 53

[Brian R. Bondy [:bbondy]]

Comment on attachment 659951 [details] [diff] [review]
Patch v1 - Disallow appusermodelid registration from widget code

[Approval Request Comment]
(snip)
The patches for defaults handling in win8 are dependent on this bug. For reasoning please see Bug 791019 Comment 35

This request is for all patches in this bug.

Comment 54

[Lukas Blakk [:lsblakk] use ?needinfo]

Triage drive-by:  let's give this more time and testing on trunk, also please nominate all the patches or submit a roll-up patch for approval, it's too obscure to mention in a comment that the approval carries on to all the patches. Thanks for helping keep our approval process consistent.

Comment 55

[Lukas Blakk [:lsblakk] use ?needinfo]

Are there any specific tests QA can do to verify this feature to aid in our risk assessment for branch uplift?

Comment 56

[Brian R. Bondy [:bbondy]]

Comment on attachment 659951 [details] [diff] [review]
Patch v1 - Disallow appusermodelid registration from widget code

I'm going to do a rollup patch for the involved bugs for defaults handling on Windows 8 and ask for approval all at once in bug 791019.  If that lands on aurora and beta I'll update the tracking fields in this bug and post the changesets in this bug as well.

Comment 57

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/releases/mozilla-beta/rev/4130a141a36b
http://hg.mozilla.org/releases/mozilla-aurora/rev/030540c0d243

Comment 58

[Simona Badau, Desktop QA]

I can see the Firefox tile on the metro start screen (if when installing Firefox 16 beta 5 in classic view, the box to create a shortcut in the Start menu programs folder remains checked).

Also, Firefox can be uninstalled by right clicking on the Firefox tile on the metro screen and selecting the Uninstall option (after selecting the uninstall option I was redirected to the 'Uninstall or change a program' window in classic view).

Brian, please let me know if there is anything else that I can test here.
Thanks.

Comment 59

[Brian R. Bondy [:bbondy]]

Thanks for the testing Simona, I think Comment 58 is normal handling.

Comment 60

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:16.0) Gecko/20100101 Firefox/16.0

I can confirm that Firefox 16 beta 5 tile is present and can be uninstalled from metro start screen.

I did find a scenario in which the Firefox tile is not displayed in the metro start screen (I could also reproduce it on Chrome). 
Steps to reproduce:
1. Install Firefox in a standard location (the option to create a shortcut in the start menu is checked by default)
2. Go to metro start screen - right click on the Firefox tile and select Unpin from Start
3. Install Firefox again in the same location as in step 1.
Actual results: The Firefox tile will not be displayed in the metro start screen. After restarting Win 8, the tile is displayed again if Firefox is reinstalled.
Please advice if I should log a new bug for this.

Based on Comment 59 and Comment 58 (and considering the previous scenario is reproducible also for other browsers), setting status for Firefox 16 to verified.

Comment 61

[Brian R. Bondy [:bbondy]]

For the unpin from start screen, I've seen that as well. You can post another bug for that, but it may be protected by a cryptographic hash.  But we should look into it.

Comment 62

[Jim Mathies [:jimm]]

You can repin it by typing "firefox" on the start screen, then right click the shortcut that comes up and select pin to start.

Comment 63

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0

Verified that Firefox 17 beta 5 tile is present and can be uninstalled from metro start screen.

Based on comment 61 -> filed Bug 810349 (for the scenario from Comment 60).

Setting status for Firefox 17 to Verified.

Comment 64

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0

Verified as fixed - Firefox 18 beta 1 tile is present and can be uninstalled from metro start screen.

Comment 65

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old