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

RESOLVED FIXED in Firefox 30

Status

defect
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
Firefox 30
x86_64
Windows 8.1

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: p=3 s=it-30c-29a-28b.3 r=ff30 [qa-])

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

5 years ago
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
Blocks: metrobacklog
Whiteboard: p=0
(Assignee)

Updated

5 years ago
Whiteboard: p=0 → p=3
Assignee: nobody → jmathies
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: kamiljoz
Whiteboard: p=3 → p=3 s=it-30c-29a-28b.2 r=ff30
(Assignee)

Comment 1

5 years ago
Posted patch wip (obsolete) — Splinter Review
(Assignee)

Updated

5 years ago
Attachment #8378495 - Attachment is patch: true
(Assignee)

Comment 2

5 years ago
Posted patch patch v.1 (obsolete) — Splinter Review
Attachment #8378495 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
Posted patch part 1 - installer patch v.1 (obsolete) — Splinter Review
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?
Attachment #8378524 - Attachment is obsolete: true
Attachment #8378537 - Flags: feedback?(robert.strong.bugs)
(Assignee)

Comment 4

5 years ago
(Assignee)

Updated

5 years ago
Attachment #8378539 - Attachment description: part 2 - ceh entry point → part 1 - ceh entry point
(Assignee)

Comment 5

5 years ago
Posted patch part 2 - installer patch (obsolete) — Splinter Review
Cleaned up version without the message boxes.
Attachment #8378537 - Attachment is obsolete: true
Attachment #8378537 - Flags: feedback?(robert.strong.bugs)
Attachment #8378544 - Flags: feedback?(robert.strong.bugs)
(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 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
Attachment #8378544 - Flags: feedback?(robert.strong.bugs) → feedback+
(Assignee)

Comment 8

5 years ago
> 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!
(Assignee)

Updated

5 years ago
Attachment #8378539 - Flags: review?(netzen)
(Assignee)

Comment 9

5 years ago
Posted patch part 2 - installer patch (obsolete) — Splinter Review
Attachment #8378544 - Attachment is obsolete: true
Attachment #8383252 - Flags: review?(robert.strong.bugs)
(Assignee)

Updated

5 years ago
Attachment #8383252 - Attachment is obsolete: true
Attachment #8383252 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 10

5 years ago
(Assignee)

Comment 11

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

this time with the right patch.
Attachment #8383256 - Flags: review?(robert.strong.bugs)
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
Attachment #8383256 - Flags: review?(robert.strong.bugs) → review+
Whiteboard: p=3 s=it-30c-29a-28b.2 r=ff30 → p=3 s=it-30c-29a-28b.3 r=ff30
Attachment #8378539 - Flags: review?(netzen) → review+
https://hg.mozilla.org/mozilla-central/rev/343f8b01d180
https://hg.mozilla.org/mozilla-central/rev/d8033ff0e71a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
For testing and verification.  Reopen if any defects found.
Flags: needinfo?(kamiljoz)
Whiteboard: p=3 s=it-30c-29a-28b.3 r=ff30 → p=3 s=it-30c-29a-28b.3 r=ff30 [qa-]
Flags: needinfo?(kamiljoz)
You need to log in before you can comment on or make changes to this bug.