Closed Bug 981166 Opened 6 years ago Closed 6 years ago

Disable Metro builds

Categories

(Firefox Build System :: General, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

(firefox28+ fixed, firefox29+ verified, firefox30+ verified, firefox31 verified)

VERIFIED FIXED
mozilla31
Tracking Status
firefox28 + fixed
firefox29 + verified
firefox30 + verified
firefox31 --- verified

People

(Reporter: jimm, Assigned: bbondy)

References

Details

Attachments

(6 files, 10 obsolete files)

101.35 KB, image/png
Details
1.35 KB, patch
Details | Diff | Splinter Review
1.47 KB, text/x-ms-regedit
Details
15.44 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
853 bytes, patch
jimm
: review+
lsblakk
: feedback+
Details | Diff | Splinter Review
16.13 KB, patch
Details | Diff | Splinter Review
Cleanup work on install/uninstall and update.
Group: mozilla-employee-confidential
Attached patch rip metrofx (obsolete) — Splinter Review
Assignee: nobody → jmathies
Attachment #8387945 - Flags: review?(robert.strong.bugs)
Attached patch rip metrofx (obsolete) — Splinter Review
without the message box. :)
Attachment #8387945 - Attachment is obsolete: true
Attachment #8387945 - Flags: review?(robert.strong.bugs)
Attachment #8387949 - Flags: review?(robert.strong.bugs)
Comment on attachment 8387949 [details] [diff] [review]
rip metrofx

Just nits

>diff -r 02506bdd5bd8 browser/installer/windows/nsis/installer.nsi
>--- a/browser/installer/windows/nsis/installer.nsi	Thu Mar 06 08:26:09 2014 -0800
>+++ b/browser/installer/windows/nsis/installer.nsi	Fri Mar 07 17:53:12 2014 -0600
>@@ -378,16 +378,24 @@
>     ${CleanupMetroBrowserHandlerValues} ${DELEGATE_EXECUTE_HANDLER_ID} \
>                                         "FirefoxURL" \
>                                         "FirefoxHTML"
>     ${AddMetroBrowserHandlerValues} ${DELEGATE_EXECUTE_HANDLER_ID} \
>                                     "$INSTDIR\CommandExecuteHandler.exe" \
>                                     $AppUserModelID \
>                                     "FirefoxURL" \
>                                     "FirefoxHTML"
>+!else
>+  ; The metro browser has been deprecated.
nit: The metro browser is not enabled by the mozconfig

>+  ${If} ${AtLeastWin8}
>+    ${RemoveDEHRegistration} ${DELEGATE_EXECUTE_HANDLER_ID} \
>+                             $AppUserModelID \
>+                             "FirefoxURL" \
>+                             "FirefoxHTML"
>+  ${EndIf}
> !endif
>   ${EndIf}
> 
> !ifdef MOZ_MAINTENANCE_SERVICE
>   ; If the maintenance service page was displayed then a value was already 
>   ; explicitly selected for installing the maintenance service and 
>   ; and so InstallMaintenanceService will already be 0 or 1.
>   ; If the maintenance service page was not displayed then 
>diff -r 02506bdd5bd8 browser/installer/windows/nsis/shared.nsh
>--- a/browser/installer/windows/nsis/shared.nsh	Thu Mar 06 08:26:09 2014 -0800
>+++ b/browser/installer/windows/nsis/shared.nsh	Fri Mar 07 17:53:12 2014 -0600
>@@ -211,16 +211,24 @@
>     ; the tile image on the start screen.  So wait before calling RegisterCEH.
>     ; We only need to do this when the DEH doesn't already exist.
>     ReadRegStr $0 HKCU "Software\Classes\FirefoxURL\shell\open\command" "DelegateExecute"
>     ${If} $0 != ${DELEGATE_EXECUTE_HANDLER_ID}
>       Sleep 3000
>     ${EndIf}
>     Call RegisterCEH
>   ${EndIf}
>+!else
>+  ; The metro browser has been deprecated.
nit: The metro browser is not enabled by the mozconfig

>+  ${If} ${AtLeastWin8}
>+    ${RemoveDEHRegistration} ${DELEGATE_EXECUTE_HANDLER_ID} \
>+                             $AppUserModelID \
>+                             "FirefoxURL" \
>+                             "FirefoxHTML"
>+  ${EndIf}
> !endif
> !macroend
> !define PostUpdate "!insertmacro PostUpdate"
> 
> !macro SetAsDefaultAppGlobal
>   ${RemoveDeprecatedKeys} ; Does not use SHCTX
> 
>   SetShellVarContext all      ; Set SHCTX to all users (e.g. HKLM)
>diff -r 02506bdd5bd8 browser/installer/windows/nsis/uninstaller.nsi
>--- a/browser/installer/windows/nsis/uninstaller.nsi	Thu Mar 06 08:26:09 2014 -0800
>+++ b/browser/installer/windows/nsis/uninstaller.nsi	Fri Mar 07 17:53:12 2014 -0600
>@@ -295,16 +295,24 @@
> !ifdef MOZ_METRO
>   ${If} ${AtLeastWin8}
>     ${un.CleanupMetroBrowserHandlerValues} ${DELEGATE_EXECUTE_HANDLER_ID} \
>                                            "FirefoxURL" \
>                                            "FirefoxHTML"
>   ${EndIf}
>   ${ResetWin8PromptKeys}
>   ${ResetWin8MetroSplash}
>+!else
>+  ; The metro browser has been deprecated.
nit: The metro browser is not enabled by the mozconfig

>+  ${If} ${AtLeastWin8}
>+    ${RemoveDEHRegistration} ${DELEGATE_EXECUTE_HANDLER_ID} \
>+                             $AppUserModelID \
>+                             "FirefoxURL" \
>+                             "FirefoxHTML"
>+  ${EndIf}
> !endif
> 
>   ${un.RegCleanAppHandler} "FirefoxURL"
>   ${un.RegCleanAppHandler} "FirefoxHTML"
>   ${un.RegCleanProtocolHandler} "ftp"
>   ${un.RegCleanProtocolHandler} "http"
>   ${un.RegCleanProtocolHandler} "https"
> 
>diff -r 02506bdd5bd8 toolkit/mozapps/installer/windows/nsis/common.nsh
>--- a/toolkit/mozapps/installer/windows/nsis/common.nsh	Thu Mar 06 08:26:09 2014 -0800
>+++ b/toolkit/mozapps/installer/windows/nsis/common.nsh	Fri Mar 07 17:53:12 2014 -0600
>@@ -7688,17 +7688,17 @@
>   System::Int64Op $0 / 1000
>   Pop $0
> 
>   Pop $1
>   Exch $0 ; return elapsed seconds
> !macroend
> 
> !ifdef MOZ_METRO
>-; Removes the CEH registration if it's set to our installation directory.
>+; Removes the DEH registration if it's set to our installation directory.
> ; If it's set to some other installation directory, then it should be removed
> ; by that installation.
> !macro RemoveDEHRegistrationIfMatchingCall un
> 
>   Function ${un}RemoveDEHRegistrationIfMatchingCall
>     ; Retrieve DEH ID from the stack into $R9
>     Exch $R9
>     Exch 1
>@@ -7812,8 +7812,72 @@
>   ; Win8 Metro delegate execute handler registration
>   WriteRegStr SHCTX "Software\Classes\CLSID\${DELEGATE_EXECUTE_HANDLER_ID}" "" "$BrandShortName CommandExecuteHandler"
>   WriteRegStr SHCTX "Software\Classes\CLSID\${DELEGATE_EXECUTE_HANDLER_ID}" "AppId" "${DELEGATE_EXECUTE_HANDLER_ID}"
>   WriteRegStr SHCTX "Software\Classes\CLSID\${DELEGATE_EXECUTE_HANDLER_ID}\LocalServer32" "" "${DELEGATE_EXECUTE_HANDLER_PATH}"
> !macroend
> !define AddMetroBrowserHandlerValues "!insertmacro AddMetroBrowserHandlerValues"
> !endif ;end MOZ_METRO
> 
>+; Unconditionally removes delegate execute handler registration used in
>+; launching the metro browser, plus misc. metro related registry values.
nit:
; Unconditionally removes the delegate execute handler registration used to
; launch the metro browser and misc. metro related registry values.

>+!macro RemoveDEHRegistration DELEGATE_EXECUTE_HANDLER_ID \
>+                             APP_USER_MODEL_ID \
>+                             PROTOCOL_ACTIVATION_ID \
>+                             FILE_ACTIVATION_ID
>+  ; remove the app user model id root registration. We don't need this
>+  ; here anymore, we just use it for tray registrationdown in widget,
>+  ; which we read out of the mozilla keys.
nit:
; Remove the app user model id root registration used by widget for tray redistration.

>+  DeleteRegKey HKCU "Software\Classes\${APP_USER_MODEL_ID}"
>+  DeleteRegKey HKLM "Software\Classes\${APP_USER_MODEL_ID}"
>+
>+  ; remove metro browser splash image data
>+  DeleteRegKey HKCU "Software\Classes\Local Settings\Software\Microsoft\Windows\CurrentVersion\AppModel\SystemAppData\DefaultBrowser_NOPUBLISHERID\SplashScreen\DefaultBrowser_NOPUBLISHERID!${APP_USER_MODEL_ID}"
>+
>+  ; misc. mozilla keys no longer in use
; Misc. metro keys

>+  DeleteRegKey HKCU "Software\Mozilla\Firefox\Metro"
>+  DeleteRegValue HKCU "Software\Mozilla\Firefox" "CEHDump"
>+  DeleteRegValue HKCU "Software\Mozilla\Firefox" "MetroD3DAvailable"
>+  DeleteRegValue HKCU "Software\Mozilla\Firefox" "MetroLastAEH"
>+
>+  ; toast gunk
; Remove Application Accociation Toasts

>+  DeleteRegValue HKCU "Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxHTML_.htm"
>+  DeleteRegValue HKCU "Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxHTML_.html"
>+  DeleteRegValue HKCU "Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxHTML_.xht"
>+  DeleteRegValue HKCU "Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxHTML_.xhtml"
>+  DeleteRegValue HKCU "Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxHTML_.shtml"
>+  DeleteRegValue HKCU "Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxURL_ftp"
>+  DeleteRegValue HKCU "Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxURL_http"
>+  DeleteRegValue HKCU "Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxURL_https"
>+
>+  ; Remove delegate execute handler clsid registration
>+  DeleteRegKey HKCU "Software\Classes\CLSID\${DELEGATE_EXECUTE_HANDLER_ID}"
>+  DeleteRegKey HKLM "Software\Classes\CLSID\${DELEGATE_EXECUTE_HANDLER_ID}"
>+
>+  DeleteRegValue HKCU "Software\Classes\${PROTOCOL_ACTIVATION_ID}\shell\open\command" "DelegateExecute"
>+  DeleteRegValue HKLM "Software\Classes\${PROTOCOL_ACTIVATION_ID}\shell\open\command" "DelegateExecute"
>+  DeleteRegValue HKCU "Software\Classes\${FILE_ACTIVATION_ID}\shell\open\command" "DelegateExecute"
>+  DeleteRegValue HKLM "Software\Classes\${FILE_ACTIVATION_ID}\shell\open\command" "DelegateExecute"
>+
>+  ; Remove protocol and file delegate execute handler id assoc
>+  DeleteRegValue HKCU "Software\Classes\${PROTOCOL_ACTIVATION_ID}" "AppUserModelID"
>+  DeleteRegValue HKLM "Software\Classes\${PROTOCOL_ACTIVATION_ID}" "AppUserModelID"
>+  DeleteRegValue HKCU "Software\Classes\${FILE_ACTIVATION_ID}" "AppUserModelID"
>+  DeleteRegValue HKLM "Software\Classes\${FILE_ACTIVATION_ID}" "AppUserModelID"
>+
>+  ; Purge delegate execute application data windows 8 required to work
nit:
Not sure why you went with "delegate execute application data windows 8"
This seems better
; Remove delegate execute application registry keys


>+  DeleteRegKey HKCU "Software\Classes\${PROTOCOL_ACTIVATION_ID}\Application"
>+  DeleteRegKey HKLM "Software\Classes\${PROTOCOL_ACTIVATION_ID}\Application"
>+  DeleteRegKey HKCU "Software\Classes\${FILE_ACTIVATION_ID}\Application"
>+  DeleteRegKey HKLM "Software\Classes\${FILE_ACTIVATION_ID}\Application"
>+
>+  ; Remove misc. shell open info
>+  DeleteRegValue HKCU "Software\Classes\${PROTOCOL_ACTIVATION_ID}\shell\open" "CommandId"
>+  DeleteRegValue HKLM "Software\Classes\${PROTOCOL_ACTIVATION_ID}\shell\open" "CommandId"
>+  DeleteRegValue HKCU "Software\Classes\${FILE_ACTIVATION_ID}\shell\open" "CommandId"
>+  DeleteRegValue HKLM "Software\Classes\${FILE_ACTIVATION_ID}\shell\open" "CommandId"
>+  DeleteRegValue HKCU "Software\Classes\${PROTOCOL_ACTIVATION_ID}\shell\open\command" "DelegateExecute"
>+  DeleteRegValue HKLM "Software\Classes\${PROTOCOL_ACTIVATION_ID}\shell\open\command" "DelegateExecute"
>+  DeleteRegValue HKCU "Software\Classes\${FILE_ACTIVATION_ID}\shell\open\command" "DelegateExecute"
>+  DeleteRegValue HKLM "Software\Classes\${FILE_ACTIVATION_ID}\shell\open\command" "DelegateExecute"
>+!macroend
>+!define RemoveDEHRegistration "!insertmacro RemoveDEHRegistration"
>+!define un.RemoveDEHRegistration "!insertmacro RemoveDEHRegistration"

r=me with nits fixed and assuming you have verified the registry keys are removed correctly
Attachment #8387949 - Flags: review?(robert.strong.bugs) → review+
Comment on attachment 8387949 [details] [diff] [review]
rip metrofx

Jim, there is a lot of code duplication with this patch between common.nsh and shared.nsh. I'm going to resubmit without the code duplication and request review from you.
Attachment #8387949 - Flags: review+ → review-
Comment on attachment 8387949 [details] [diff] [review]
rip metrofx

bah, let's just fix the comments, etc. in a followup bug.
Attachment #8387949 - Flags: review- → review+
For testing purposes: I'm assuming we're going to be testing migrating from a beta build that currently supports the metro browser, to a build that does not (a build that has the --enable-metro flag removed from build config). In which case the front end for metro will not be included in the new distribution, and the code posted here which landed on beta today will be triggered.

What this means is that the os integration configuration in the registry, as well as the specialized CommandExecuteHandler.exe launcher we use to launch one or the other front end will not be in use after the update completes. 

Specific areas that should be tested:

1) clicking on the main application tile in the metro tile interface
2) clicking on existing shortcuts
3) clicking on pinned shortcuts on the desktop taskbar
4) clicking on desktop shortcuts
5) searching for firefox in the tile interface and executing it from search results

Default browser configurations we should test the above with:

1) a beta build that has been set as the default browser before the update
2) a beta build that is not the default before the update

Default browser settings:

1) fresh install, setting the browser as default - making sure shortcuts, tiles, and pinned shortcuts still launch the browser correctly.

That's a good starting list. Generally we want to make sure that os integration for a fresh install and an updated install work as expected.
I am CCing Ioana Budnar, our Softvision QA lead, since she and her team will be required to know this information for their testing. 

Ioana, I will set up a private document to track the testing of this since we don't want it to be public yet. Any issues should be reported to that document and not as public bugs in Bugzilla.
If no one minds, let's use this bug to make sure we remove cleanly (and mozconfig --enable-metro too) from Aurora & Nightly.
Summary: Install cleanup → Disable Metro builds
Let's put it in the correct component for the disabling as well
Component: Installer → Build Config
Depends on: 982448
Depends on: 982462
Depends on: 982478
I think we could fix both bug 982462 and bug 982478 by disabling the updates from the metro browser.  These users would have to update from Desktop to get a new Beta build.  Something I think is acceptable.  

That is if they both only happen from updating via Metro for a Metro enabled build to a non Metro enabled build, which I think they do.
I don't think we can effectively do that without a code change though, which people would have to update through.  I'm not sure if there's a way remotely to detect that the update is happening through Metro. I think the update URL is the same.
We have the update server set up to respond to requests with %PRODUCT% Firefox and MetroFirefox. AIUI the latter is used when using Metro. We could off offer no update to MetroFirefox requests, or offer some build prior to the problem (then nothing from that build).
That's what I was thinking originally but I think that as of shared profiles we have the same product name. We have different app ids, but the app ID doesn't show up in that URL.
It sounds to me like the Post Update is not being executed from the Metro updates. The 1-time splash screen after updates bug I think we can live with, the class not registered one is another story and I think is related to Post update not running for some reason.
nthomas, is forcing an upgrade path through a particular must have build, a pain?
We'd have to do an additional beta before disabling Metro if we went that route.
Flags: needinfo?(nthomas)
It looks like the post update code is not running on my machine that has it reproduced right now. The registry entries that should be deleted from Jim's patch are still there.  Here's a screenshot.
So perhaps this check isn't passing:
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/common.nsh#5310

Or helper.exe isn't running at all for some reason from metro updates.
I know it used to but maybe that's broken.
(In reply to Brian R. Bondy [:bbondy] from comment #16)
> nthomas, is forcing an upgrade path through a particular must have build, a
> pain?
> We'd have to do an additional beta before disabling Metro if we went that
> route.

Depends which branches you're interested in. On beta we're planning a watershed for the recent changes rstrong has landed (in support of release builds on the beta channel). For Nightly/Aurora it's not easy unless something else changes in the update query, but around the merges might be possible because of the version change.
Flags: needinfo?(nthomas)
Where watershed means update paths like:  old builds ->  watershed build --> latest
(In reply to Kamil Jozwiak [:kjozwiak] from comment #17)
> Created attachment 8389615 [details]
> metro registries not being removed
> 
> It looks like the post update code is not running on my machine that has it
> reproduced right now. The registry entries that should be deleted from Jim's
> patch are still there.  Here's a screenshot.

Is this from doing updates via metro land or from desktop, or both? We should concentrate on testing desktop integration post the update. Some weirdness in launch behavior when the metro browser is the default and last used front end is expected. Although we do want to make sure desktop gets launched.
(In reply to Jim Mathies [:jimm] (away 4/1-4/21) from comment #21)
> (In reply to Kamil Jozwiak [:kjozwiak] from comment #17)
> > Created attachment 8389615 [details]
> > metro registries not being removed
> > 
> > It looks like the post update code is not running on my machine that has it
> > reproduced right now. The registry entries that should be deleted from Jim's
> > patch are still there.  Here's a screenshot.
> 
> Is this from doing updates via metro land or from desktop, or both? We
> should concentrate on testing desktop integration post the update. Some
> weirdness in launch behavior when the metro browser is the default and last
> used front end is expected. Although we do want to make sure desktop gets
> launched.

I did some desktop testing using the steps in bug 982478 and found that desktop appears to be working properly post update. I did notice that the MetroLastAEH key was left behind. I think during the update process we're cleaning things up, but still running the last launch from the old install.

I've also confirmed the new install purged old files and settings, the ceh is gone and registry registration for the ceh has been removed.

So I think our remaining issue here is with updating via metro. (QA should confirm desktop users are in good shape post update.)
(In reply to Brian R. Bondy [:bbondy] from comment #18)
> So perhaps this check isn't passing:
> http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/
> windows/nsis/common.nsh#5310
> 
> Or helper.exe isn't running at all for some reason from metro updates.
> I know it used to but maybe that's broken.

From what I remember, helper.exe gets launched from browser shell code after the browser starts?
Flags: needinfo?(robert.strong.bugs)
Yep, Jim that sounds consistent with what Kamil found.  Bug 982478 is particularly bad long term though if someone happens to update via the metro browser.

(In reply to Nick Thomas [:nthomas] from comment #19)
> (In reply to Brian R. Bondy [:bbondy] from comment #16)
> > nthomas, is forcing an upgrade path through a particular must have build, a
> > pain?
> > We'd have to do an additional beta before disabling Metro if we went that
> > route.
> 
> Depends which branches you're interested in. On beta we're planning a
> watershed for the recent changes rstrong has landed (in support of release
> builds on the beta channel). For Nightly/Aurora it's not easy unless
> something else changes in the update query, but around the merges might be
> possible because of the version change.

If we did this, I think we'd want it ideally for Nightly, Aurora and Beta channels unfortunately.
I'm not sure if that amount of work is worth it for the amount of people that will update via Metro though.
Given low usage, and even lower people updating from it, maybe it's not worth it.
helper.exe gets launched for some shell cleanup in the browser, and also when setting defaults. 

But in this context, for post update, it gets executed a couple of different ways, from updater code.  Depending on if you're using the service, whether you're using background updates, or whether you're using neither.

It gets run twice for service updates, one from the current user for some shell stuff and one from the local system account.

http://dxr.mozilla.org/mozilla-central/search?q=LaunchWinPostProcess&redirect=true


---

We do have some front end code that runs after updates though too. It might be a good opportunity to run some one off code to do similar cleanup, or just launch post update via helper.exe again for the metro case.  I believe running helper.exe /PostUpdate to be idempotent.

http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#2057
The restart step are here, up to the point where we start updating. 

1) browser front end triggers the download and staging of a new rev.
2) user shuts down
3) MetroAppShell catches the update shutdown and launches the ceh here - 
http://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroAppShell.cpp#130
4) The ceh launches desktop in the background with a command line flag indicating the metro browser should be relaunched after the update - 
http://mxr.mozilla.org/mozilla-central/source/browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp#676
5) nsAppRunner launches the update process which ends up here -
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsUpdateDriver.cpp#974

From here things split off depending on which call is made and whether or not the service is in use.
(In reply to Brian R. Bondy [:bbondy] from comment #25)
> helper.exe gets launched for some shell cleanup in the browser, and also
> when setting defaults. 
> 
> But in this context, for post update, it gets executed a couple of different
> ways, from updater code.  Depending on if you're using the service, whether
> you're using background updates, or whether you're using neither.
> 
> It gets run twice for service updates, one from the current user for some
> shell stuff and one from the local system account.
> 
> http://dxr.mozilla.org/mozilla-central/
> search?q=LaunchWinPostProcess&redirect=true

Hmm, so if most users are using the service to update from metro shouldn't this helper.exe call get made for the user before we try to relaunch? I'm confused as to why the registry isn't getting cleaned up.
(In reply to Jim Mathies [:jimm] (away 4/1-4/21) from comment #27)
> Hmm, so if most users are using the service to update from metro shouldn't
> this helper.exe call get made for the user before we try to relaunch?

Yes whether the service and background updates are used or not, it should be called.

> I'm confused as to why the registry isn't getting cleaned up.

ya that's the bug, unrelated to the service it's not getting run for some reason, see Comment 18 for a couple thoughts on it.
Here's the last of the log output, no errors, and I see the metro launch params there.

Executing service command software-update, ID: a475d7ec-960d-4362-b26f-f35db3834b61
Passed in path: 'C:\Users\Jim\AppData\Local\Temp\MozUpdater\bgupdate\updater.exe'; Using this path for updating: 'C:\Program Files (x86)\Mozilla Maintenance Service\update\updater.exe'.
updater.exe was compared successfully to the installation directory updater.exe.
The updater.exe application contains the Mozilla updater identity.
The file "C:\Program Files (x86)\Mozilla Maintenance Service\update\updater.exe" is signed and the signature was verified.
Starting update process as the service in session 0.
Starting service with cmdline: "C:\Program Files (x86)\Mozilla Maintenance Service\update\updater.exe" C:\Users\Jim\AppData\Local\Mozilla\updates\E7CF176E110C211B\updates\0 "C:\Program Files (x86)\Mozilla Firefox\updated" 7108/replace C:\WINDOWS\system32 "C:\Program Files (x86)\Mozilla Firefox\firefox.exe" --metro-update -ServerName:DefaultBrowserServer
Process was started... waiting on result.
Process finished with return code 0.
updater.exe returned status: succeeded

Launching post update process as the service in session 0.
updater.exe was launched and run successfully!
Service command software-update complete.
service command MozillaMaintenance complete with result: Success.
> Launching post update process as the service in session 0.
> updater.exe was launched and run successfully!

Does this mean the post update process wasn't launched as the user?
Not necessarily the maintenance service program itself only runs it elevated. The updater runs it unelevated. Does anything show up in the last update log?
It just tells us that for sure it ran as elevated as the system account, it doesn't tell us anything about if it ran as unelevated. (Runs twice when using the service).  The important one here is the unelevated one because of the HKCU entries.
I think this is the case that's happening:
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.cpp#2796

Not sure why though, and it only seems to be happening if you update from the Metro front end.  Possibly only when going to a disabled metro build.
It wouldn't be in the last update log btw because we only keep a log for the elevated updater and not the unelevated updater which is a shim
(In reply to Brian R. Bondy [:bbondy] from comment #34)
> I think this is the case that's happening:
> http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/
> updater.cpp#2796
> 
> Not sure why though, and it only seems to be happening if you update from
> the Metro front end.  Possibly only when going to a disabled metro build.

Hmm, I don't see anything here that would cause that launch call to fail, but I can also confirm from looking at the registry that the helper.exe nsis call RemoveDEHRegistration definitely wasn't invoked.
So our options here seem to be:

1) take the time to do some in-depth update testing on oak. Even if we thought we knew what was wrong, guessing and landing something only to find it doesn't fix the issue will just make things worse.

2) disable updates from metro and update our messaging such that users know they must update via desktop.

Also, I think we still have some in-depth testing of desktop updates to do.

I can get started on #1.
#2 will only work if we combine it with an extra beta (the first having Metro enabled) and treating it like a watershed as nthomas described:

> Where watershed means update paths like:  old builds ->  watershed build --> latest

I'll chat with you on IRC Jim and help with #1.
Depends on: 982655
Updating using fxdesktop will not produce the class error message as the registries are being removed. However I can confirm that "MetroLastAHE" is not being removed as per comment #22. I created Bug #982655 to address this issue but it's a lower priority issue. This doesn't prevent fxdesktop from working.

I'm still testing this and will post anything else I find!
(In reply to Kamil Jozwiak [:kjozwiak] from comment #39)
> Updating using fxdesktop will not produce the class error message as the
> registries are being removed. However I can confirm that "MetroLastAHE" is
> not being removed as per comment #22. I created Bug #982655 to address this
> issue but it's a lower priority issue. This doesn't prevent fxdesktop from
> working.
> 
> I'm still testing this and will post anything else I find!

That will go away on the next update since the same helper code will run again, and none of the metro components will be involved in updating.
Some interesting findings: 
I caught this with procmon while reproducing.

It is launching helper.exe twice, once elevated and the second time unelevated.
We were expecting it to only be launched once elevated.

So for some reason, even know we're running helper.exe with /PostUpdate as the current user, we aren't hitting the reg removal code.
It launches so both HKLM and HKCU can be updated in the case of the service.
Flags: needinfo?(robert.strong.bugs)
Attached patch oak debug patch (obsolete) — Splinter Review
Attachment #8389866 - Flags: feedback?(netzen)
Comment on attachment 8389866 [details] [diff] [review]
oak debug patch

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

I think the extra logging may make the problem less reproducible. I'd opt for only the common.nsh change for Oak to see what happens.
Attachment #8389866 - Flags: feedback?(netzen)
Depends on: 982731
I've requested follow up on the dependencies reported last night. I need to know if A) these will be fixed before we release and B) whether they should block pushing RC to beta testers.
Someone else should confirm, but in my opinion bug 982478 should block the beta, aurora, and m-c releases where this lands.  For the other bugs currently reported, I do not think they should block.
(In reply to Brian R. Bondy [:bbondy] from comment #46)
> Someone else should confirm, but in my opinion bug 982478 should block the
> beta, aurora, and m-c releases where this lands.  For the other bugs
> currently reported, I do not think they should block.

I agree, but to add bug 982462 to block, assuming it's not a dupe of bug 982478.
Has anyone given any thought to how we'll handle this on the main repos? From my understanding we'll need to - 

1) land the changes here plus any updates from our current testing with the RC on mc, aurora, and beta.
2) merge those changes into every repo that has metro enabled.
3) remove --enable-metro from all repos.
4) disable metro-mochitest test runs where ever they are running.
6) hide mc test runs on tbpl.
5) do nightly builds and test updates on each of the main release repos.

Since we haven't turned metro off yet on any repo (just in the RC builds) we should probably test all this on a project repo like oak first. Then coordinate rolling all this out from there.

Am I missing any important steps here?
See Bug 982478 Comment 11 for one way to fix this and some other thoughts. Further discussion happening over there. 

----

> Am I missing any important steps here?

That sounds like a good starting list and we'll catch anything missing by trying it on oak.
Depends on: 983793
Group: mozilla-employee-confidential
CC list accessible: false
Not accessible to reporter
Depends on: 983854
Depends on: 983855
No longer depends on: 983855
Depends on: 983269
Depends on: 982034
Tommorow is merge day from mozilla-aurora -> mozilla-beta. Will we be landing patches in aurora (or in beta) that disable metro prior to 29.0b1 ?
(In reply to Nick Thomas [:nthomas] from comment #50)
> Tommorow is merge day from mozilla-aurora -> mozilla-beta. Will we be
> landing patches in aurora (or in beta) that disable metro prior to 29.0b1 ?

I think we'll be landing on mc, ma, and mb all at the same time. We need to fix bug 983793 for mr as well since those changes weren't necessary.

bbondy and I will discuss what we want to land and when today. We have a working solution on oak currently.
Attachment #8387949 - Attachment is obsolete: true
Attachment #8389866 - Attachment is obsolete: true
Please keep metro enabled for another day on mozilla-central, mozilla-aurora, and mozilla-beta. 
This is obvious, but don't enable metro on mozilla-release :)

We do have a fix we can use today if there is a need, but we'd rather land a different fix late today or tomorrow. The alt fix will be ready in 2-3 hours, but we'd like to test it through oak to be sure too and that takes a few additional hours.
Can you take a look at this Jim?
This works locally but I want to test it on Oak still. I'd like you to take a look before I push it to Oak for actual update testing.

I didn't bother doing a session 0 check because I think it's safer to do it this way. For non service updates we'll be ensured proper cleanup too.
Attachment #8392228 - Flags: review?(jmathies)
Comment on attachment 8392228 [details] [diff] [review]
Cleanup for all users via reg enumeration from high integrity helper.exe

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

Can you roll the base patch into this a repost? (rstrong should review the final too.)

::: toolkit/mozapps/installer/windows/nsis/common.nsh
@@ +7834,5 @@
> +  DeleteRegValue HKLM "Software\Classes\${FILE_ACTIVATION_ID}" "AppUserModelID"
> +  DeleteRegValue HKLM "Software\Classes\${PROTOCOL_ACTIVATION_ID}\shell\open" "CommandId"
> +  DeleteRegValue HKLM "Software\Classes\${FILE_ACTIVATION_ID}\shell\open" "CommandId"
> +  DeleteRegValue HKLM "Software\Classes\${PROTOCOL_ACTIVATION_ID}\shell\open\command" "DelegateExecute"
> +  DeleteRegValue HKLM "Software\Classes\${FILE_ACTIVATION_ID}\shell\open\command" "DelegateExecute"

these last two are dupes.

@@ +7860,2 @@
>  
>    ; toast gunk

nit - down in comment 3 rob requested some comment updates for this routine. Those comments are updated in my latest option 1 patch as well.

@@ +7885,5 @@
>    ; Remove misc. shell open info
> +  DeleteRegValue HKU "$1\Software\Classes\${PROTOCOL_ACTIVATION_ID}\shell\open" "CommandId"
> +  DeleteRegValue HKU "$1\Software\Classes\${FILE_ACTIVATION_ID}\shell\open" "CommandId"
> +  DeleteRegValue HKU "$1\Software\Classes\${PROTOCOL_ACTIVATION_ID}\shell\open\command" "DelegateExecute"
> +  DeleteRegValue HKU "$1\Software\Classes\${FILE_ACTIVATION_ID}\shell\open\command" "DelegateExecute"

these last two are dupes.
Attachment #8392228 - Flags: review?(jmathies) → feedback+
f? for oak landing
Attachment #8392228 - Attachment is obsolete: true
Attachment #8392261 - Flags: review?(robert.strong.bugs)
Attachment #8392261 - Flags: feedback?(jmathies)
Pushed and started a nightly for testing.
Comment on attachment 8392261 [details] [diff] [review]
Solution 2 - Cleanup for all users via reg enumeration from high integrity helper.exe

Please move the Firefox specific code into shared.nsh.

Can you reuse the existing code in shared.nsh for some of this?
Attachment #8392261 - Flags: review?(robert.strong.bugs)
We can probably combine some cleanup with other cleanup that only happens when we're the default browser in common.nsh but this is a good step in the right direction and we need something sooner than later.
Attachment #8392261 - Attachment is obsolete: true
Attachment #8392261 - Flags: feedback?(jmathies)
Attachment #8392350 - Flags: review?(robert.strong.bugs)
Comment on attachment 8392350 [details] [diff] [review]
Solution 2 - Cleanup for all users via reg enumeration from high integrity helper.exe   rev2

I discussed a couple of issues with this approach over irc with Brian so clearing review.
Attachment #8392350 - Flags: review?(robert.strong.bugs)
Attachment #8392207 - Attachment is obsolete: true
Remaining to do is the cleanup of user level registry keys for users that are not logged in while the update happens.
I suspect that this will need to be done for both Post Update and install.
iirc the profiles in the registry are located under Windows NT\CurrentVersionProfileList\
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #63)
> iirc the profiles in the registry are located under Windows
> NT\CurrentVersionProfileList\

Yep I found that list as well which matches the names of the HKU entries when the user is logged in.
I also found you can mount in user registries to an arbitrary path into HKU if you have the .dat file path. 
I'm still trying to find a way without having to use the .dat path though.
A successful LogonUser call doesn't seem to mount the registry by the way.

The RegLoadKey API that I mentioned above with the filepath can be found here will work, but I think there was some concern about the filepath being on a network path and it itself needing to be loaded:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms724889%28v=vs.85%29.aspx
Best I have so far in Win32 code (Not NSIS yet), works on my machine but not sure about others:

1. Manually acquire SE_BACKUP_NAME and SE_RESTORE_NAME privs (Required for mounting HKEY_USER keys)by using LookupPrivilegeValue, AdjustTokenPrivileges
2. Get the user users profile directory (accessible via NSIS or via GetProfilesDirectory)
3. Enumerate each directory
4. For each directory, call:  
   RegLoadKeyW(HKEY_USERS, L"ProfileNumber_$0", L"PathFromStep2\\NTUSER.DAT");
5. Run the current code we have in the above patch (Solution 2)
6. Call RegUnloadKey for each key

This doesn't use the reg key SIDs that we have.
I'm going to keep looking for solutions that make use of the SID list that you mentioned rstrong, but if you want, I can stop looking for more solutions and just implement this solution as documented in this comment.

Other ideas welcome too if you can think of some in the meantime jimm or others.
I'm fine with that.
K I'm going ahead with Comment 66 but I'll use the profiledirs listed in the registry here:
HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion\ProfileList

The LoadProfile win32 api that I found only succeeds when the user is actually logged in already. Otherwise it returns that there is no valid token error.
This new rev mounts all ntuser.dat files it can find to subkeys of HKEY_USERS and then umounts them after the cleanup.

I verified that I could reproduce the problem with the old Solution 1 and Solution 2 with multiple users.

I verified that with this new patch, it successfully cleans up multi users even if they are not logged in and from a fresh reboot.

I haven't tried this via updates yet, but I'm pushing to oak to test that.
Assignee: jmathies → netzen
Attachment #8392350 - Attachment is obsolete: true
Attachment #8393555 - Flags: review?(robert.strong.bugs)
Comment on attachment 8393555 [details] [diff] [review]
Solution 2 - Cleanup for all users via reg enumeration from high integrity helper.exe rev3

>diff --git a/browser/installer/windows/nsis/shared.nsh b/browser/installer/windows/nsis/shared.nsh
>--- a/browser/installer/windows/nsis/shared.nsh
>+++ b/browser/installer/windows/nsis/shared.nsh
>@@ -1,15 +1,32 @@
> # 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/.
> 
> ; The registration ID of the COM server which is used for choosing wether
> ; to launch the Win8 metro browser or desktop browser.
> !define DELEGATE_EXECUTE_HANDLER_ID {5100FEC1-212B-4BF5-9BF8-3E650FD794A3}
>+;
>+; Defines for adjust token privs and for enumerating keys
>+!ifndef TOKEN_QUERY
>+!define TOKEN_QUERY             0x0008
>+!endif
>+!ifndef TOKEN_ADJUST_PRIVILEGES
>+!define TOKEN_ADJUST_PRIVILEGES 0x0020
>+!endif
>+!ifndef SE_RESTORE_NAME
>+!define SE_RESTORE_NAME         SeRestorePrivilege
>+!endif
>+!ifndef SE_PRIVILEGE_ENABLED
>+!define SE_PRIVILEGE_ENABLED    0x00000002
>+!endif
>+!ifndef HKEY_USERS
>+!define HKEY_USERS              0x80000003
>+!endif
nit: indent the !define in between the !ifndef

>@@ -744,44 +769,179 @@ FunctionEnd
>   StrCpy $0 "Software\Microsoft\Windows\Shell\Associations\UrlAssociations\gopher"
>   ReadRegStr $2 HKCU "$0\UserChoice" "Progid"
>   ${If} "$2" == "FirefoxURL"
>     DeleteRegKey HKCU "$0"
>   ${EndIf}
> !macroend
> !define RemoveDeprecatedKeys "!insertmacro RemoveDeprecatedKeys"
> 
>-!ifdef MOZ_METRO
> ; Resets Win8+ specific toast keys Windows sets. We call this on a
> ; fresh install and on uninstall.
>-!macro ResetWin8PromptKeys
>+!macro ResetWin8PromptKeys KEY PREFIX
>   ${If} ${AtLeastWin8}
>-    DeleteRegValue HKCU "Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxHTML_.htm"
>-    DeleteRegValue HKCU "Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxHTML_.html"
>-    DeleteRegValue HKCU "Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxHTML_.xht"
>-    DeleteRegValue HKCU "Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxHTML_.xhtml"
>-    DeleteRegValue HKCU "Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxHTML_.shtml"
>-    DeleteRegValue HKCU "Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxURL_ftp"
>-    DeleteRegValue HKCU "Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxURL_http"
>-    DeleteRegValue HKCU "Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxURL_https"
>+    DeleteRegValue ${KEY} "${PREFIX}Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxHTML_.htm"
>+    DeleteRegValue ${KEY} "${PREFIX}Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxHTML_.html"
>+    DeleteRegValue ${KEY} "${PREFIX}Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxHTML_.xht"
>+    DeleteRegValue ${KEY} "${PREFIX}Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxHTML_.xhtml"
>+    DeleteRegValue ${KEY} "${PREFIX}Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxHTML_.shtml"
>+    DeleteRegValue ${KEY} "${PREFIX}Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxURL_ftp"
>+    DeleteRegValue ${KEY} "${PREFIX}Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxURL_http"
>+    DeleteRegValue ${KEY} "${PREFIX}Software\Microsoft\Windows\CurrentVersion\ApplicationAssociationToasts" "FirefoxURL_https"
>   ${EndIf}
> !macroend
>+
nit: no empty line

> !define ResetWin8PromptKeys "!insertmacro ResetWin8PromptKeys"
>-
nit: leave empty line

>+!ifdef MOZ_METRO
> ; Resets Win8+ Metro specific splash screen info. Relies
> ; on AppUserModelID.
> !macro ResetWin8MetroSplash
>   ${If} ${AtLeastWin8}
>   ${AndIf} "$AppUserModelID" != ""
>     DeleteRegKey HKCR "Local Settings\Software\Microsoft\Windows\CurrentVersion\AppModel\SystemAppData\DefaultBrowser_NOPUBLISHERID\SplashScreen\DefaultBrowser_NOPUBLISHERID!$AppUserModelID"
>   ${EndIf}
> !macroend
> !define ResetWin8MetroSplash "!insertmacro ResetWin8MetroSplash"
> !endif
> 
>+; Adds SE_RESTORE_NAME privs
>+!macro AcquireSERestoreName
>+  StrCpy $R1 0
>+
>+  System::Call "kernel32::GetCurrentProcess() i .R0"
>+  System::Call "advapi32::OpenProcessToken(i R0, i ${TOKEN_QUERY}|${TOKEN_ADJUST_PRIVILEGES}, \
>+                                          *i R1R1) i .R0"
>+  ${If} $R0 != 0
>+    System::Call "advapi32::LookupPrivilegeValue(t n, t '${SE_RESTORE_NAME}', *l .R2) i .R0"
>+    ${If} $R0 != 0
>+      System::Call "*(i 1, l R2, i ${SE_PRIVILEGE_ENABLED}) i .R0"
>+      System::Call "advapi32::AdjustTokenPrivileges(i R1, i 0, i R0, i 0, i 0, i 0)"
>+      System::Free $R0
>+    ${EndIf}
>+    System::Call "kernel32::CloseHandle(i R1)"
>+  ${EndIf}
>+!macroend
>+!define AcquireSERestoreName "!insertmacro AcquireSERestoreName"
>+!define un.AcquireSERestoreName "!insertmacro AcquireSERestoreName"
>+
>+; Mounts all user ntuser.dat files into the registry as a subkey of HKU
>+!macro MountRegistryIntoHKU
>+  ; $0 is used as an index for HKEY_USERS enumeration
>+  StrCpy $0 0
>+
>+loopMountRegistryIntoHKU:
Per irc, please use a LogicLib Loop if possible

>+  EnumRegKey $1 HKLM "SOFTWARE\Microsoft\Windows NT\CurrentVersion\ProfileList" $0
>+  StrCmp $1 "" doneMountRegistryIntoHKU
>+
>+  ReadRegStr $2 HKLM "SOFTWARE\Microsoft\Windows NT\CurrentVersion\ProfileList\$1" "ProfileImagePath"
>+
>+  System::Call "advapi32::RegLoadKey(i ${HKEY_USERS}, t 'User_$0', t '$2\ntuser.dat')"
>+
>+  IntOp $0 $0 + 1
>+  Goto loopMountRegistryIntoHKU
>+doneMountRegistryIntoHKU:
>+
>+!macroend
>+!define MountRegistryIntoHKU "!insertmacro MountRegistryIntoHKU"
>+!define un.MountRegistryIntoHKU "!insertmacro MountRegistryIntoHKU"
>+;
>+; Unmounts all user ntuser.dat files into the registry as a subkey of HKU
>+!macro UnmountRegistryIntoHKU
>+  ; $0 is used as an index for HKEY_USERS enumeration
>+  StrCpy $0 0
>+
>+loopUnmountRegistryIntoHKU:
Per irc, please use a LogicLib Loop if possible

>+  EnumRegKey $1 HKLM "SOFTWARE\Microsoft\Windows NT\CurrentVersion\ProfileList" $0
>+  StrCmp $1 "" doneUnmountRegistryIntoHKU
>+
>+  ReadRegStr $2 HKLM "SOFTWARE\Microsoft\Windows NT\CurrentVersion\ProfileList\$1" "ProfileImagePath"
>+
>+  System::Call "advapi32::RegUnLoadKey(i ${HKEY_USERS}, t 'User_$0')"
>+
>+  IntOp $0 $0 + 1
>+  Goto loopUnmountRegistryIntoHKU
>+doneUnmountRegistryIntoHKU:
>+
>+!macroend
>+!define UnmountRegistryIntoHKU "!insertmacro UnmountRegistryIntoHKU"
>+!define un.UnmountRegistryIntoHKU "!insertmacro UnmountRegistryIntoHKU"
>+
>+; Unconditionally removes the delegate execute handler registration used to
>+; launch the metro browser and misc. metro related registry values.
>+!macro RemoveDEHRegistration DELEGATE_EXECUTE_HANDLER_ID \
>+                             APP_USER_MODEL_ID \
>+                             PROTOCOL_ACTIVATION_ID \
>+                             FILE_ACTIVATION_ID
>+  ${AcquireSERestoreName}
>+  ${MountRegistryIntoHKU}
>+
>+  ; Remove HKLM entries
>+  DeleteRegKey HKLM "Software\Classes\${APP_USER_MODEL_ID}"
>+  DeleteRegKey HKLM "Software\Classes\CLSID\${DELEGATE_EXECUTE_HANDLER_ID}"
>+  DeleteRegKey HKLM "Software\Classes\${PROTOCOL_ACTIVATION_ID}\Application"
>+  DeleteRegKey HKLM "Software\Classes\${FILE_ACTIVATION_ID}\Application"
>+  DeleteRegValue HKLM "Software\Classes\${PROTOCOL_ACTIVATION_ID}\shell\open\command" "DelegateExecute"
>+  DeleteRegValue HKLM "Software\Classes\${FILE_ACTIVATION_ID}\shell\open\command" "DelegateExecute"
>+  DeleteRegValue HKLM "Software\Classes\${PROTOCOL_ACTIVATION_ID}" "AppUserModelID"
>+  DeleteRegValue HKLM "Software\Classes\${FILE_ACTIVATION_ID}" "AppUserModelID"
>+  DeleteRegValue HKLM "Software\Classes\${PROTOCOL_ACTIVATION_ID}\shell\open" "CommandId"
>+  DeleteRegValue HKLM "Software\Classes\${FILE_ACTIVATION_ID}\shell\open" "CommandId"
>+
>+  ; $0 is used as an index for HKEY_USERS enumeration
>+  StrCpy $0 0
>+
>+loopRemoveDEHRegistration:
Per irc, please use a LogicLib Loop if possible

>+  EnumRegKey $1 HKU "" $0
>+  StrCmp $1 "" doneRemoveDEHRegistration
>+
>+  ; remove the app user model id root registration. We don't need this
>+  ; here anymore, we just use it for tray registrationdown in widget,
>+  ; which we read out of the mozilla keys.
>+  DeleteRegKey HKU "$1\Software\Classes\${APP_USER_MODEL_ID}"
>+
>+  ; remove metro browser splash image data
>+  DeleteRegKey HKU "$1\Software\Classes\Local Settings\Software\Microsoft\Windows\CurrentVersion\AppModel\SystemAppData\DefaultBrowser_NOPUBLISHERID\SplashScreen\DefaultBrowser_NOPUBLISHERID!${APP_USER_MODEL_ID}"
>+
>+  ; misc. Metro keys
>+  DeleteRegKey HKU "$1\Software\Mozilla\Firefox\Metro"
>+  DeleteRegValue HKU "$1\Software\Mozilla\Firefox" "CEHDump"
>+  DeleteRegValue HKU "$1\Software\Mozilla\Firefox" "MetroD3DAvailable"
>+  DeleteRegValue HKU "$1\Software\Mozilla\Firefox" "MetroLastAHE"
>+
>+  ${ResetWin8PromptKeys} "HKU" "$1\"
>+
>+  ; Remove delegate execute handler clsid registration
>+  DeleteRegKey HKU "$1\Software\Classes\CLSID\${DELEGATE_EXECUTE_HANDLER_ID}"
>+
>+  ; Remove protocol and file delegate execute handler id assoc
>+  DeleteRegValue HKU "$1\Software\Classes\${PROTOCOL_ACTIVATION_ID}" "AppUserModelID"
>+  DeleteRegValue HKU "$1\Software\Classes\${FILE_ACTIVATION_ID}" "AppUserModelID"
>+
>+  ; Remove delegate execute application registry keys
>+  DeleteRegKey HKU "$1\Software\Classes\${PROTOCOL_ACTIVATION_ID}\Application"
>+  DeleteRegKey HKU "$1\Software\Classes\${FILE_ACTIVATION_ID}\Application"
>+
>+  ; Remove misc. shell open info
>+  DeleteRegValue HKU "$1\Software\Classes\${PROTOCOL_ACTIVATION_ID}\shell\open" "CommandId"
>+  DeleteRegValue HKU "$1\Software\Classes\${FILE_ACTIVATION_ID}\shell\open" "CommandId"
>+  DeleteRegValue HKU "$1\Software\Classes\${PROTOCOL_ACTIVATION_ID}\shell\open\command" "DelegateExecute"
>+  DeleteRegValue HKU "$1\Software\Classes\${FILE_ACTIVATION_ID}\shell\open\command" "DelegateExecute"
>+
>+  IntOp $0 $0 + 1
>+  Goto loopRemoveDEHRegistration
>+doneRemoveDEHRegistration:
>+
>+  ${UnmountRegistryIntoHKU}
>+
>+  ClearErrors
>+!macroend
>+
>+!define RemoveDEHRegistration "!insertmacro RemoveDEHRegistration"
>+!define un.RemoveDEHRegistration "!insertmacro RemoveDEHRegistration"
>+
> ; Removes various directories and files for reasons noted below.
> !macro RemoveDeprecatedFiles
>   ; Remove talkback if it is present (remove after bug 386760 is fixed)
>   ${If} ${FileExists} "$INSTDIR\extensions\talkback@mozilla.org"
>     RmDir /r /REBOOTOK "$INSTDIR\extensions\talkback@mozilla.org"
>   ${EndIf}
> 
>   ; Remove the Java Console extension (bug 597235)
Attachment #8393555 - Flags: review?(robert.strong.bugs)
Review comments implemented.
Attachment #8393555 - Attachment is obsolete: true
Attachment #8393872 - Flags: review?(robert.strong.bugs)
Comment on attachment 8393872 [details] [diff] [review]
Solution 2 - Cleanup for all users via reg enumeration from high integrity helper.exe rev4

Thanks!
Attachment #8393872 - Flags: review?(robert.strong.bugs) → review+
btw: I am assuming that you verified that the var that is checked to be an empty string always returns an empty string in the loops.
It returns an empty string when there are no more enumerated values that match ya.
Heads up, if this passes final testing on Oak, I'll land it on fx-team tomorrow.
Then I'll request approval for aurora and beta.
Went through the following verification using the following builds:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-03-17-09-28-29-oak/firefox-30.0a1.en-US.win32.installer.exe

- Ensured that the "Windows 8 Touch" button under the Australis settings menu
- Ensured that metrofx cannot be added via the Australis configuration menu
- Ensured that the "Relaunch in Nightly for Windows 8 Touch" is removed from the Menu Bar
- Ensured that the "MetroD3DAvailable" key under HKCU/Software/Mozilla/Firefox is removed after the update
- Ensured that the "MetroLastAHE" key under HKCU/Software/Mozilla/Firefox is removed after the update
- Ensured that the "Metro" folder under HKCU/Software/Mozilla/Firefox is removed
- Ensured that all the available shortcuts are being launched in desktopfx without any issues
- Ensured that launching exterior links from Metro correctly launches desktopfx
- Went through two different users to ensure that both users will not start in metrofx once updated

Issues:

Trivial: (not important)
- The splash screen is left behind under the "App List" but never appears in front of the user like it did before. This is very minor and not even worth fixing in opinion. Once the user either restarts the machine or closes the splash screen from the "App List", it will never return and everything will correctly be launched in desktopfx.

Major Issues:
=============

I'm still getting the "Class not registered" on the second user once I updated from the first user, here are the STR: (reproduced twice without issues)

- Install the above oak build on the main user (this is a MS account)
- Install the above oak once again on the second user using the same location (this is a local account and NOT a MS account)
- Ensured that both users can switch into metrofx and have all the metrofx reg keys
- Logged into the first user (MS account) and updated via metrofx (correctly removed all the keys and launches in desktopfx without any issues)
- Logged into the second user (local account) and received the "Class not registered" error message when using any of the shortcuts (Desktop, Taskbar, Metro Start)
Thanks Kamil,

It's working for both primary and secondary user for me when I use a Microsoft account as the secondary. 
I just tried right now with another admin user which is a non microsoft account, and I reproduced the class not registered on the secondary user. 

I'm not sure if it's related to microsoft vs non microsoft user, or if it's related to a brand new user without having had a reboot yet.  I'm looking into it.
I also ran into the above issue when updating via the desktopfx rather than metrofx.. So quick recap:

- First user (MS account), all the reg keys are cleared and everything is working correctly (launching without issues)
- Second user (local account), all the reg keys are cleared but receiving "Class not registered" error. Updated via desktopfx and metrofx (same problem)
Looks like we aren't removing:
HKEY_LOCAL_MACHINE\SOFTWARE\Classes\EEFEA8717BC47F65\.exe\shell\open\command
DelegateExecute

I'm not sure why this effects only the secondary user though.  And only if they are a non microsoft win8 created account.
I'm just going to flatten this into the other patch but wanted to get an r+ for the changes.  This key is already being removed in HKU for each user.
Attachment #8394231 - Flags: review?(jmathies)
Comment on attachment 8394231 [details] [diff] [review]
appusermodelid.diff

err we already list this, checking why the existing one isn't working
Attachment #8394231 - Flags: review?(jmathies)
While reproducing the issue, I noticed that the following key isn't being removed:
- HKEY_USERS\S-1-5-21-1791338240-857805526-2257772433-1002\Software\Classes\EEFEA8717BC47F65\.exe\shell\open\command

The one under HKLM is being removed.
So from Kamil's findings, it seems like the problem can happen with either HKLM or HKCU.

This new rev tries for best case deletion instead:

If only this succeeds, then  Firefox will startup without a class error, but it will ask to open the exe.
> DeleteRegValue HKLM "Software\Classes\${APP_USER_MODEL_ID}\.exe\shell\open\command" "DelegateExecute"

If any of these succeed, then firefox will start fine without errors:

> DeleteRegKey HKLM "Software\Classes\${APP_USER_MODEL_ID}\.exe\shell\open"
> DeleteRegKey HKLM "Software\Classes\${APP_USER_MODEL_ID}\.exe\shell"
> DeleteRegKey HKLM "Software\Classes\${APP_USER_MODEL_ID}\.exe"
> DeleteRegKey HKLM "Software\Classes\${APP_USER_MODEL_ID}"
Attachment #8393872 - Attachment is obsolete: true
Attachment #8394327 - Flags: review?(robert.strong.bugs)
Comment on attachment 8394327 [details] [diff] [review]
Solution 2 - Cleanup for all users via reg enumeration from high integrity helper.exe rev5

r=me based on the interdiff. Thanks!
Attachment #8394327 - Flags: review?(robert.strong.bugs) → review+
Attached file EEFEA8717BC47F65.reg
Still getting the same issue with the latest push. Added an export of the key that's not being removed.
So I talked to Kamil about the issue in Comment 86.

The reg key belongs to the secondary user which is not logged in during the update.
Other reg keys that exist from that user are properly removed, so it doesn't seem like a mounting problem.
It doesn't seem like a permission deleting key problem, because all of the key and subkeys are still there and the user isn't even logged in for that key.
Update OK I know what's happening...

There was an intermittent problem with HKLM removal which seems to be fixed with the newest build.

Independent of that....

We add these keys to SHCTX, at install time those will go into HKLM (what I was testing)
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/common.nsh#

But if you explicitly set the default, it runs under helper.exe and sets them under HKCU.

When we do this:
> DeleteRegKey HKU "$1\Software\Classes\${APP_USER_MODEL_ID}"

That is basically a no-op because the Classes root is not stored inside ntuser.dat. It's stored in a different .dat file which we have to mount independent of the ntuser.dat. 

The location with the per user class registration is: 
> C:\Users\BrianR.Bondy\AppData\Local\Microsoft\Windows\UsrClass.dat
As per discussion on irc, mounts HKU classes separately since it's in a diff .dat file and those parts were not being cleaned up on secondary users that actually have the keys (Those who set their default explicitly)
Attachment #8394327 - Attachment is obsolete: true
Attachment #8394956 - Flags: review?(robert.strong.bugs)
Comment on attachment 8394956 [details] [diff] [review]
Solution 2 - Cleanup for all users via reg enumeration from high integrity helper.exe rev6

Interdiff looks good.
Attachment #8394956 - Flags: review?(robert.strong.bugs) → review+
I went through the issue once again with the latest oak push and everything worked this time around! Once I switched over to the local user, all three shortcuts (Desktop, Task Bar and Metro) worked without any issues. I also made sure that the registry entries have been removed from the following location:

- HKEY_CURRENT_USER\Software\Mozilla (made sure Metro folders and keys have been removed)
- HKEY_LOCAL_MACHINE\SOFTWARE\Classes\EEFEA8717BC47F65\ (made sure it has been removed)
- HKEY_USERS\S-1-5-21-1791338240-857805526-2257772433-1002\Software\Classes\EEFEA8717BC47F65\ (made sure it has been removed)

I also went through some of the test cases from comment #77 and made sure that everything was working correctly. Launched the browser several times and made sure that all the "Firefox Metro" shortcuts have been removed within the browser. Also made sure that clicking on links from third party applications would launch desktopfx without any issues.
feedback? to lsblakk for approval to turn off Metro Firefox on m-c. 
I'll request uplift after it lands on m-c.

This patch will land with the other patch r+ed by rstrong and tested on Oak by Kamil.
Attachment #8395444 - Flags: review?(jmathies)
Attachment #8395444 - Flags: feedback?(lsblakk)
Attachment #8395444 - Flags: review?(jmathies) → review+
Comment on attachment 8395444 [details] [diff] [review]
Disable Metro Firefox. rev1

I approve of this plan, then uplift nom for Aurora/Beta branches when vetted.
Attachment #8395444 - Flags: feedback?(lsblakk) → feedback+
https://hg.mozilla.org/mozilla-central/rev/e2e942406501
https://hg.mozilla.org/mozilla-central/rev/132db9edab95
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Metro Firefox being enabled
User impact if declined: Metro Firefox will live
Testing completed (on m-c, etc.): Kamil tested on Oak and it is working
Risk to taking this patch (and alternatives if risky): lowish
String or IDL/UUID changes made by this patch: none
Attachment #8396391 - Flags: approval-mozilla-beta?
Attachment #8396391 - Flags: approval-mozilla-aurora?
Attachment #8396391 - Flags: approval-mozilla-beta?
Attachment #8396391 - Flags: approval-mozilla-beta+
Attachment #8396391 - Flags: approval-mozilla-aurora?
Attachment #8396391 - Flags: approval-mozilla-aurora+
I went through the Nightly verification using the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-03-24-03-02-03-mozilla-central/

Once the above build was installed, I updated via metrofx and pulled today's build without any issues.

- Went through the test cases outlined in comment #77
- Went through the test cases outlined in comment #91 and ensured that all the registries have been removed from both users
- Ensured that desktopfx was launched via all the shortcuts (Desktop, Task Bar and Metro)
- Ensured that selecting third party links from Metro launches desktopfx
- Ensured that Firefox Nightly was still selected as the default browser when updating
- Updating using both desktopfx and metrofx and made sure metrofx was removed correctly (went through each update route twice)
Duplicate of this bug: 982655
Depends on: 987939
Let me know if you want me to back this out of beta and aurora, there seems to be some reports of problems in bug 982478. I don't think this is a general problem though and I think these users that are affected should probably just fix via a manual work around.
Flags: needinfo?(sledru)
Flags: needinfo?(lsblakk)
To buy us a bit of extra time I canceled the nightly Aurora build:
https://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=849084164ca7
I'm not sure if there's something special about the peoples install in bug 982478 or if it's a more widespread problem.
If I don't hear back, just so everyone is on the same page I'll leave the Aurora nightly build tomorrow and the beta build whenever that builds.
Backed out of beta as a precaution:
https://hg.mozilla.org/releases/mozilla-beta/rev/ffd1dcf75a6f

lsblakk if you disagree for any reason, or if you also want it backed out of aurora, please let me know before 6PM PST.  The current plan is to let it ride on an Aurora Nightly and to see if there is any fallout from that. We believe it will be minor if there is.
Flags: needinfo?(sledru)
Flags: needinfo?(lsblakk)
FWIW, the revert on beta also made the Win8 mochitest-bc perma-fail below go away.
https://tbpl.mozilla.org/php/getParsedLog.php?id=36696159&tree=Mozilla-Beta
Does debug build and win64 still keep --enable-metro?
Yep there are a few cases that still enable it, I'll fix that in  bug
Depends on: 988853
Went through the verification process using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-03-25-00-40-02-mozilla-aurora/

- Went through all of the test cases from comment #77 without any issues
- Went through all of the test cases from comment #97 without any issues

Went through single user and multi-user upgrades and everything worked without any issues. However, there may be some edge cases that have not been verified in this ticket that can be found in bug #982478
Depends on: 990261
We've tested Metro disabling today on 29.0 Beta 4, and found some issues, if someone can take a look:
https://etherpad.mozilla.org/australis-b4.
Depends on: 990711
Updated with comments in the etherpad.
Additional comments were added by the QA team members in: https://etherpad.mozilla.org/australis-b4.
Let us know if more info is needed.
So the etherpad says you're using a zip build.
Testing should not be done with zip builds.  It is not supported.

Also this exact issue is expected and known for zip builds.
See bug 982478 comment 49 

Please re-test without a zip build and let us know if there is still a problem for class not registerd or with opening HTML files. 

---

In general, I think that testing should not be done with zip builds moving forward.
It's better to test the things that are supported.
Note: Use the full installer when testing specific builds and not the stub installer, because the stub installer always downloads the latest build behind the scenes.
Tested using full installers of Nightly and Firefox 29 beta 4.
For the following scenario I've encountered the "Class not registered" error pop-up:
- Using Nightly as the default browser, save a webpage as simple HTML (eg. google.com)
- Right click the saved page and select it to open using Firefox Beta.

Results:
Instead of the webpage being displayed in FF 29 beta 4, the mentioned error appeared.
If Beta is the default browser, the error does not display.

Mention:
The error does not display if Beta is the default browser and you select to open the webpage in Nightly.
Could you please provide the 2 installers used and the exact steps used to reproduce?
Do you get any other errors or only with that menu? For example double clicking on an html file. Or clicking on the taskbar icon.
The full installers were downloaded from:
1. http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/
2. http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/29.0b4-candidates/build1/win32/en-US/

The STR I've used are:
1. Download full Nightly installer from link 1 (firefox-30.0a1.en-US.win32.installer.exe).
2. Install Nightly and set it as default browser.
3. Open it and save a webpage as a simple HTML file (eg. google.com).
4. Close Nightly.
5. Download full FF 29 beta 4 installer from link 2 (Firefox Setup 29.0b4.exe) and install it.
6. After the install is completed, right click on the webpage saved at step 3 and choose to open it using Firefox 29 beta 4.

This is the only way I get this error. 
Double clicking the html file will open it with the default browser.
FF will correctly open by clicking on it's taskbar icon.
Cornel, which build did you use to reproduce the problem? There's two different versions of Nightly available in that location:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/

- firefox-30.0a1.en-US.win32.installer.exe (has metrofx still enabled)
- firefox-31.0a1.en-US.win32.installer.exe (has metrofx disabled)

I can reproduce the issue using FF 30.0a1 but cannot reproduce using FF 31.0a1:

- Download/Install FF 30.0a1 and set it as the default browser
- Download/Install FF 29.0b4 (Run it and don't set it as default)
- Close FF 29.0b4 and try re-opening FF 30.0a1 via any of the shortcuts available (Desktop, Taskbar)
Cornel, another quick question:

When you're right clicking on the HTML file and selecting "Open With", how do you manage to see both Nightly and Firefox BETA entries? While Nightly is set as the default browser, the only FF item in the "Open With" context menu will be the Nightly icon and won't list Firefox BETA (it's not even listed under "How do you want to open this file?")

The only way for me to open the HTML file via the context menu when Nightly is the default browser, is to select "Look for another app on this PC" and than manually selecting the firefox.exe executable from the BETA directory.
I was using the FF 31.0a1 build, without metro (pasted the wrong installer in previous comment).

Indeed, I've manually selected the firefox executable from the Beta directory using "Look for another app on this PC"
Cornel, I went through the STR outlined in comment #115 on 4 different computers and couldn't reproduce the issue.. Please let me know if there's something I'm doing wrong or missing below:

* Download/Install FF 31.0a1 from the following location: (make this the default browser when prompted)
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
* Open FF 31.0a1, visit google.com and "Save As" the page to the desktop
* Download/Install FF 29.0b4 from the following location:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/29.0b4-candidates/build1/win32/en-US/Firefox Setup 29.0b4.exe
* Once installed, right click on Google.htm and select "Open with" -> "Choose default program..."
* Once "How do you want to open this file?" modal appears, select "More Options" -> "Look for another app on this PC"
* Browse to "C:\Program Files (x86)\Mozilla Firefox\" and select "firefox.exe and than click on "Open"

Doing this, the Google.htm file will always open under Nightly without any issues

Computers/VM's used:
- Surface Pro 2 with Win 8.1 x64 (Went through the test case x3 times without issues)
- Lenovo X1 Carbon with Win 8.1 x64 (Went through the test case x3 times without issues)
- Windows 8 x86 VM (Went through the test case x3 times without issues)
- Windows 8.1 x86 VM (Went through the test case x3 times without issues)
- Windows 8 x64 VM (Went through the test case x3 times without issues)
- Windows 8.1 x64 VM (Went through the test case x3 times without issues)

Cornel, could you maybe add a video that shows the entire process from the beginning? Maybe you're doing something different that I'm missing?
I'm unable to reproduce the mentioned issue after a system restore back to February.
It seems like this was caused by the OS, guessing some registry entries were still left behind due to the multiple Firefox versions previously installed on the device.

Tried to reproduce the issue on other two machines running Windows 8.1 (32bit and 64bit) and could not reproduce it.
Sounds good! Thanks for checking Cornel, much appreciated. If you run into this issue again, let me know.
Went through the following verification process using the latest BETA build from the following location:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/29.0b3/win32/en-US/ (metrofx enabled and than updated)

- Went through all the test cases from comment # 77 without any issues
- Went through all the test cases from comment # 91 without any issues
- Went through all the test cases from comment # 119 without any issues

(tested this extensively yesterday, went through all the other tickets and ensured everything was still working correctly)

I also installed the latest BETA and insured that metrofx has been removed without any issues, used the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/29.0b5/win32/en-US/
No longer depends on: 982478
Duplicate of this bug: 982478
Depends on: 995310
Status: RESOLVED → VERIFIED
Keywords: verifyme
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 31 → mozilla31
You need to log in before you can comment on or make changes to this bug.