968916 - Pave over upgrades w/metrofx set as the last browser launched, installer launches desktop

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Install/Update, defect, P2)

Description

[Jim Mathies [:jimm]]

STR:

1) install an older nightly 
2) flip from desktop to metro
3) download latest stub installer in metrofx and run

result: stub switches to desktop

4) flip back to metrofx and close it
5) flip to Windows desktop and complete upgrade

result: after complete, the stub launches the desktop browser

expected: stub should launch into metrofx

Comment 1

[Jim Mathies [:jimm]]

Attachment: wip

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 3

[Jim Mathies [:jimm]]

Attachment: part 1 - installer patch v.1

This switches between launch firefox.exe directly and using a new helper I've added to the ceh for launching the metro browser. Metro is only launched if the install is the default browser, and if the metro front end was loaded last.

One question I had herer, there's comment here about $INSTDIR - 

http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/installer.nsi#747

That statement doesn't appear to be true. The macro IsMetroBrowserDefaultOnWin8 is using $INSTDIR and it's valid in the elevated process. I dumped it via a MessageBox to be sure. I'm wondering if I'm missing something here or if that comment can be removed.

Note this patch isn't complete, I need to add this to the stub as well.. but I'm not sure how I can go about testing my changes there, maybe a try built stub installer would work?

Comment 4

[Jim Mathies [:jimm]]

Attachment: part 1 - ceh entry point

Comment 5

[Jim Mathies [:jimm]]

Attachment: part 2 - installer patch

Cleaned up version without the message boxes.

Comment 6

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

(In reply to Jim Mathies [:jimm] from comment #3)
> Created attachment 8378537 [details] [diff] [review]
> part 1 - installer patch v.1
> 
> This switches between launch firefox.exe directly and using a new helper
> I've added to the ceh for launching the metro browser. Metro is only
> launched if the install is the default browser, and if the metro front end
> was loaded last.
> 
> One question I had herer, there's comment here about $INSTDIR - 
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/
> installer.nsi#747
> 
> That statement doesn't appear to be true. The macro
> IsMetroBrowserDefaultOnWin8 is using $INSTDIR and it's valid in the elevated
> process. I dumped it via a MessageBox to be sure. I'm wondering if I'm
> missing something here or if that comment can be removed.
; Find the installation directory when launching using GetFunctionAddress
; from an elevated installer since $INSTDIR will not be set in this installer
The elevated process is making the unelevated process launch the application and when this was implemented the plugin used for elevation didn't sync the main variables so $INSTDIR in the unelevated process could definitely be set to a different value than $INSTDIR in the elevated process. I believe the plugin now syncs the main variables but I haven't had a need to verify this since what we have works.

> Note this patch isn't complete, I need to add this to the stub as well.. but
> I'm not sure how I can go about testing my changes there, maybe a try built
> stub installer would work?
The stub just launches the full installer so as long as your new code in the full installer handles the silent run case properly as it does currently when installing from the stub it should be fine. As for testing, you should be able to use a locally built stub and the try server doesn't build stub installers.

Comment 7

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

Comment on attachment 8378544 [details] [diff] [review]
part 2 - installer patch

Looks fine overall though this will need a more thorough review.

Use the AppAssocReg plugin for finding out whether Firefox is set as default
http://nsis.sourceforge.net/Application_Association_Registration_plug-in

Comment 8

[Jim Mathies [:jimm]]

> Use the AppAssocReg plugin for finding out whether Firefox is set as default
> http://nsis.sourceforge.net/Application_Association_Registration_plug-in

That is a huge help!

Comment 9

[Jim Mathies [:jimm]]

Attachment: part 2 - installer patch

Comment 10

[Jim Mathies [:jimm]]

Attachment: part 2 - installer patch

Comment 11

[Jim Mathies [:jimm]]

Comment on attachment 8383256 [details] [diff] [review]
part 2 - installer patch

this time with the right patch.

Comment 12

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

Comment on attachment 8383256 [details] [diff] [review]
part 2 - installer patch

>diff -r 53cfed52f7f2 browser/installer/windows/nsis/installer.nsi
>--- a/browser/installer/windows/nsis/installer.nsi	Thu Feb 27 14:19:07 2014 -0600
>+++ b/browser/installer/windows/nsis/installer.nsi	Thu Feb 27 15:05:39 2014 -0600
>...
>@@ -734,34 +731,66 @@
> 
> Function LaunchApp
>   ${ManualCloseAppPrompt} "${WindowClass}" "$(WARN_MANUALLY_CLOSE_APP_LAUNCH)"
> 
>   ClearErrors
>   ${GetParameters} $0
>   ${GetOptions} "$0" "/UAC:" $1
>   ${If} ${Errors}
>-    Exec "$\"$INSTDIR\${FileMainEXE}$\""
>+    StrCpy $1 "0"
Add
StrCpy $2 "0"

>+!ifdef MOZ_METRO
>+    ; Check to see if this install location is currently set as the
>+    ; default browser.
>+    AppAssocReg::QueryAppIsDefaultAll "${AppRegName}" "effective"
>+    Pop $1
>+    ; Check for a last run type to see if metro was the last browser
>+    ; front end in use.
>+    ReadRegDWORD $2 HKCU "Software\Mozilla\Firefox" "MetroLastAHE"
>+!endif
>+    ${If} $1 == "1"
>+    ${AndIf} $2 == "1" ; 1 equals AHE_IMMERSIVE
>+      ; Relaunch into metro
>+      Exec '$\"$INSTDIR\CommandExecuteHandler.exe$\" --launchmetro'
Change to
Exec "$\"$INSTDIR\CommandExecuteHandler.exe$\" --launchmetro"
since that is the prevailing style

>+    ${Else}
>+      ; Relaunch into desktop
>+      Exec "$\"$INSTDIR\${FileMainEXE}$\""
>+    ${EndIf}
>   ${Else}
>     GetFunctionAddress $0 LaunchAppFromElevatedProcess
>     UAC::ExecCodeSegment $0
>   ${EndIf}
> FunctionEnd
> 
> Function LaunchAppFromElevatedProcess
>   ; Find the installation directory when launching using GetFunctionAddress
>   ; from an elevated installer since $INSTDIR will not be set in this installer
>   ${StrFilter} "${FileMainEXE}" "+" "" "" $R9
>   ReadRegStr $0 HKLM "Software\Clients\StartMenuInternet\$R9\DefaultIcon" ""
>   ${GetPathFromString} "$0" $0
>   ${GetParent} "$0" $1
>   ; Set our current working directory to the application's install directory
>   ; otherwise the 7-Zip temp directory will be in use and won't be deleted.
>   SetOutPath "$1"
>-  Exec "$\"$0$\""
>+  StrCpy $2 "0"
Add
StrCpy $3 "0"

>+!ifdef MOZ_METRO
>+  ; Check to see if this install location is currently set as the
>+  ; default browser.
>+  AppAssocReg::QueryAppIsDefaultAll "${AppRegName}" "effective"
>+  Pop $2
>+  ; Check for a last run type to see if metro was the last browser
>+  ; front end in use.
>+  ReadRegDWORD $3 HKCU "Software\Mozilla\Firefox" "MetroLastAHE"
>+!endif
>+  ${If} $2 == "1"
>+  ${AndIf} $3 == "1" ; 1 equals AHE_IMMERSIVE
>+    Exec '$\"$1\CommandExecuteHandler.exe$\" --launchmetro'
Change to
Exec "$\"$1\CommandExecuteHandler.exe$\" --launchmetro"
since that is the prevailing style

>+  ${Else}
>+    Exec "$\"$0$\""
>+  ${EndIf}
> FunctionEnd

Comment 13

[Jim Mathies [:jimm]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/343f8b01d180
remote:   https://hg.mozilla.org/integration/fx-team/rev/d8033ff0e71a

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/343f8b01d180
https://hg.mozilla.org/mozilla-central/rev/d8033ff0e71a

Comment 15

[Marco Mucci [:MarcoM]]

For testing and verification.  Reopen if any defects found.