Closed Bug 737833 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox :: Installer, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 18
Tracking Status
firefox15 --- unaffected
firefox16 --- verified
firefox17 --- verified
firefox18 --- verified

People

(Reporter: jimm, Assigned: bbondy)

References

Details

(Keywords: qawanted, Whiteboard: metro-preview completed-elm)

Attachments

(8 files, 11 obsolete files)

892 bytes, patch
jimm
: review+
Details | Diff | Splinter Review
1.07 KB, patch
jimm
: review+
Details | Diff | Splinter Review
1.12 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
7.97 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
1.21 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
2.60 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
18.94 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
1.72 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
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.
Install should also support setting fennec as the default metro browser via Windows Default Programs control panel.
Blocks: 740424
Summary: Implement install/uninstall/helper for fennec metro → Implement install/uninstall/helper changes in Fx desktop installer for Fx metro
Component: General → Installer
Product: Fennec → Firefox
QA Contact: general → installer
Blocks: elm-merge
Depends on: metro-build
Attached patch changes patch (obsolete) — Splinter Review
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/
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
Blocks: 756580
No longer blocks: 756580
Depends on: 760023
No longer depends on: 760023
Depends on: 762344
No longer depends on: 762344
No longer depends on: metro-build
Assignee: nobody → netzen
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.
Whiteboard: metro-preview
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.
Attachment #625172 - Attachment is obsolete: true
Attachment #659948 - Flags: review?(jmathies)
Attached patch Patch v1 - Win8 logicLib checks (obsolete) — Splinter Review
Attachment #659949 - Flags: review?(jmathies)
Attached patch Patch v1 - Zip install changes (obsolete) — Splinter Review
Attachment #659950 - Flags: review?(jmathies)
Tomorrow I'll try with a fresh win8 install, and also I'll try on win7.
Attachment #659951 - Flags: review?(jmathies) → review+
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?
Attachment #659949 - Flags: review?(jmathies) → review+
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'
Attachment #659948 - Flags: review?(jmathies) → review+
Can we update unregisterbrowser.reg as well?
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.
Fixed missing E typo on DELEGATE_EX(E)CUTE_ID causing uninstaller to not remove the wow64 registration.
Attachment #659948 - Attachment is obsolete: true
Attachment #660181 - Flags: review+
Attached patch Patch v2 - Zip install changes (obsolete) — Splinter Review
- 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
Attachment #659951 - Attachment is obsolete: true
Attachment #660182 - Flags: review?(jmathies)
Attachment #660182 - Attachment description: Patch v1 - Zip install changes → Patch v2 - Zip install changes
Attachment #659950 - Attachment is obsolete: true
Attachment #659950 - Flags: review?(jmathies)
Attachment #659951 - Attachment is obsolete: false
Whiteboard: metro-preview → metro-preview completed-elm(except for the zip patch)
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.
Attachment #660181 - Flags: superreview?(robert.bugzilla)
Attachment #659949 - Flags: superreview?(robert.bugzilla)
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.
Blocks: 790422
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.
Attachment #660182 - Flags: review?(jmathies) → review-
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.
Attachment #660213 - Flags: review?(jmathies) → review+
Depends on: 789461
Whiteboard: metro-preview completed-elm(except for the zip patch) → metro-preview completed-elm
Attachment #660182 - Attachment is obsolete: true
Blocks: 790474
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.????
Attachment #659949 - Flags: superreview?(robert.bugzilla) → superreview-
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?
Attachment #660181 - Flags: superreview?(robert.bugzilla) → superreview-
(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.
(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.
Attached patch Patch v2 - Win8 logicLib checks (obsolete) — Splinter Review
Attachment #659949 - Attachment is obsolete: true
Attachment #661092 - Flags: review?(robert.bugzilla)
Attachment #660181 - Attachment is obsolete: true
Attachment #661124 - Flags: review?(robert.bugzilla)
Split out CEH from installer patch since it'll be landing on m-c as part of the bigger elm merge.
Attachment #661125 - Flags: review+
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.
Attachment #661092 - Flags: review?(robert.bugzilla) → review-
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.
Attachment #661124 - Flags: review?(robert.bugzilla) → review-
ah! I was completely confused why some of the existing code only had 2 parameters, that makes sense :)
Attached patch Patch v3 - Win8 logicLib checks (obsolete) — Splinter Review
Fixed nsis 2.33 support
Attachment #661092 - Attachment is obsolete: true
Attachment #661208 - Flags: review?(robert.bugzilla)
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.
Attachment #661124 - Attachment is obsolete: true
Attachment #661276 - Flags: review?(robert.bugzilla)
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?
Attachment #661208 - Flags: review?(robert.bugzilla) → review+
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?
Attachment #661276 - Flags: review?(robert.bugzilla) → review+
(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.
Attachment #661417 - Flags: review?(robert.bugzilla)
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.
Attachment #661420 - Flags: review?(robert.bugzilla)
Attachment #661125 - Attachment description: Patch v1 - CEH → Patch v1 - CEH (elm only)
(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?
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"
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.
Attachment #661714 - Flags: review?(robert.bugzilla)
http://hg.mozilla.org/projects/elm/rev/1b19a8b61ffe
(will sync review comments to elm)
Depends on: 791687
(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.
As a side note, it is fine to use LogicLib in common.nsh if you prefer.
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?
Attachment #661420 - Flags: review?(robert.bugzilla) → review+
(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.
Rebased win8 check patch since removing 2.33 support will land first.
Carrying forward r+.
Attachment #661208 - Attachment is obsolete: true
Attachment #661866 - Flags: review+
Attachment #660213 - Attachment description: Patch v1 - Fix helper.exe path after resource splitting bug → Patch v1 - Fix helper.exe path after resource splitting bug (Elm only)
Removed an extra DEH entry that can cause problems when uninstalling a metro build followed by installing a non metro build.  Carrying forward r+.
Attachment #661276 - Attachment is obsolete: true
Attachment #662780 - Flags: review+
rebased
Attachment #661417 - Attachment is obsolete: true
Attachment #662781 - Flags: review+
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.
Attachment #659951 - Flags: approval-mozilla-beta?
Attachment #659951 - Flags: approval-mozilla-aurora?
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.
Are there any specific tests QA can do to verify this feature to aid in our risk assessment for branch uplift?
Keywords: qawanted, verifyme
Depends on: 791019
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.
Attachment #659951 - Flags: approval-mozilla-beta?
Attachment #659951 - Flags: approval-mozilla-aurora?
Blocks: 794071
No longer blocks: 794071
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.
Thanks for the testing Simona, I think Comment 58 is normal handling.
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.
QA Contact: simona.marcu
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.
You can repin it by typing "firefox" on the start screen, then right click the shortcut that comes up and select pin to start.
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.
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.
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.