952121 - Nightly uninstaller doesn't unregister the command execute handler properly

Status: VERIFIED FIXED

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

Description

[Masatoshi Kimura [:emk]]

Steps to reproduce:
1. Install Firefox release.
2. Install Nightly.
3. Uninstall Nightly.
3. Open a URL from an external app (e.g. Thunderbird).

Actual result:
URL doesn't open. No response from UI at all.

Expected result:
URL should open. At least some error messages should be displayed (but it might not be our problem).

Comment 1

[Masatoshi Kimura [:emk]]

Ah, please add the following step between 2. and 3.:
Set release Firefox as default.

Comment 2

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Time to post some investigation results.

These two reg keys seem to tell Windows how to open Firefox when Firefox is the default browser and the user has done something in an external app that should load a particular web page:
  HKCR\FirefoxURL\shell\open\command
  HKCR\FirefoxHTML\shell\open\command

Under those two keys are the following two values:
  Path (points to the currently-default firefox.exe)
  DelegateExecute {5100FEC1-212B-4BF5-9BF8-3E650FD794A3}

The DelegateExecute value does not exist if a non-metro Firefox is installed on a clean system. The installers for metro-enabled Firefox create that value.

The UUID that the DelegateExecute value contains points to another reg key that is also added by installers for metro-enabled Firefox:
  HKCR\Wow6432Node\CLSID\{5100FEC1-212B-4BF5-9BF8-3E650FD794A3}\LocalServer32

That key has an unnamed value that contains the path to CommandExecuteHandler.exe. This value gets updated whenever a metro-enabled Firefox is set as the default browser, meaning that the correct CommandExecuteHandler.exe is used as the "DelegateExecute" for whichever metro-enabled Firefox happens to be the default.

However, the value is NOT updated when a non-metro Firefox is set as the default browser. This means that (e.g.) clicking a link in Thunderbird causes the CommandExecuteHandler.exe of the previously-default metro-enabled Firefox to be launched. The CommandExecuteHandler mostly does the right thing (it's able to launch the actual default non-metro Firefox) but things go really awry when the metro-enabled Firefox is uninstalled; at this point the "DelegateExecute" value of our "FirefoxURL\shell\open\command" key is pointing to a path that doesn't exist.

Ideally, whenever a non-metro Firefox is set as the default browser, the "DelegateExecute" value would not exist. I've discovered that setting the DelegateExecute value to a an empty string seems to have the same effect as deleting the value, so I think it would be worth investigating whether we can make non-metro Firefox builds set the DelegateExecute value to an empty string when they are set as default.

Obviously that approach wouldn't help for non-metro Firefox installations that already exist out in the wild, so we could additionally do the following when uninstalling metro-enabled versions of Firefox:
  1) Check whether our CommandExecuteHandler path is the one pointed to by HKCR\Wow6432Node\CLSID\{5100FEC1-212B-4BF5-9BF8-3E650FD794A3}\LocalServer32
  2) If so, remove the DelegateExecute entry from HKCR\FirefoxURL\shell\open\command and HKCR\FirefoxHTML\shell\open\command
This would fix the issue where uninstalling the metro-enabled Firefox whose CommandExecuteHandler is currently registered causes external links to stop working.

Comment 3

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Unsetting milestone flag. Not sure how it got set.

Comment 4

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v1

Tested locally and this seems to fix the issue at hand.

Ideally we would also make it so that whenever a non-metro Firefox becomes the default, the DelegateExecute value would be deleted (or become an empty string). That way we wouldn't have weird activation sequences like this:
  1. User clicks external link
  2. CommandExecuteHandler from a metro-enabled Fx runs
  3. CEH launches the non-metro Fx that is actually set as default

Somehow, when metro-enabled Fx is set as the default, Windows knows to update the DelegateExecute reg value to the UUID of CommandExecuteHandler, which makes me optimistic that we could trick Windows into setting DelegateExecute to a blank string when a non-metro Fx becomes the default. I'm not yet sure how to accomplish this, so maybe it's best to solve the current issue and file this remaining work as a follow-up.

Comment 5

[Brian R. Bondy [:bbondy]]

Comment on attachment 8375765 [details] [diff] [review]
Patch v1

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

I agree with posting a followup for the rest, I don't think it's as easily fixable.

The patch looks like the right approach, thanks!
 
A couple changes are needed though:

::: toolkit/mozapps/installer/windows/nsis/common.nsh
@@ +7714,5 @@
>      StrCmp "$R7" "$R8" clearHKCU next
>      clearHKCU:
>      DeleteRegKey HKCU "Software\Classes\CLSID\$R0"
> +    DeleteRegValue HKCU "Software\Classes\FirefoxURL\shell\open\command" "DelegateExecute"
> +    DeleteRegValue HKCU "Software\Classes\FirefoxHTML\shell\open\command" "DelegateExecute"

These FirefoxURL and FirefoxHTML handler values need to be passed into common.nsh since this is generic toolkit code for the installer.

I think we also should have .exe\shell\open\command

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v2

> I agree with posting a followup for the rest, I don't think it's as easily
> fixable.

Filed bug 973103.

> These FirefoxURL and FirefoxHTML handler values need to be passed into
> common.nsh since this is generic toolkit code for the installer.

Done!
 
> I think we also should have .exe\shell\open\command

I believe that the uninstaller already correctly removes that key (it seems to remove the entire HKCR\app_id_goes_here key)

Comment 7

[Brian R. Bondy [:bbondy]]

Comment on attachment 8376583 [details] [diff] [review]
Patch v2

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

::: toolkit/mozapps/installer/windows/nsis/common.nsh
@@ +7753,2 @@
>      Pop $R0
> +    Pop $R6

I don't personally mind but I've gotten r- on patches before for having the popping of the register names out of order, and the pushing in the wrong order.  So the start of the function should go from highest to lowest register use (9, 8, 7, 6, 5), and the end should go from lowest to highest (5,6,7,8,9).  To do this you have to rename existing usage of those registers.  So to keep with the rest of the file let's do that please.

I think this cleanup is also off, I think it'll restore the stack in a different state than it was before the function call.  Exchs need to match up at the start and the end. Pushes match with pops though.

For example if you have at the top:
Exch $R9
Exch 1
Exch $R8
Exch 2
Push $R7

Then the bottom should match to:
Pop $R7
Exch 2
Exch $R8
Exch 1
Exch $R9

If it gets confusing (It does for me! :)) then check out the other functions and how they handle it in the file.

Comment 8

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v3

Incorporated review comments.

The previous patch had made sure to set the *registers* back to how they were before the function call (I'll spare you the hand-written examples showing that it worked :), but had modified the stack to remove all of the function arguments. I'm surprised that we make our functions leave their arguments on the stack but if that's the way the rest of our installer works I'll just make this function do the same.

Comment 9

[Brian R. Bondy [:bbondy]]

The calls like Exch $R7 swaps out the top of the stack with $R7, so it preserves it in the same order with those.

Comment 10

[Brian R. Bondy [:bbondy]]

Ya I just worked through the example here and see that the args are left over after the function call, strange, but ya I think we should stay consistent to the rest of the way it works in the file.

Comment 11

[Brian R. Bondy [:bbondy]]

Comment on attachment 8377785 [details] [diff] [review]
Patch v3

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

::: toolkit/mozapps/installer/windows/nsis/common.nsh
@@ +7773,2 @@
>    Push ${DELEGATE_EXECUTE_HANDLER_ID}
>    Call ${un}RemoveDEHRegistrationIfMatchingCall

Thanks for updating it. 

FWIW I think that having the function preserve the stack and registry values the way they were (and as you did in this new patch) is the correct thing to do, but I think the caller who modifies the stack here should be doing pops.  But I'd prefer we just stay consistent with the way it works everywhere else in the file.

So let's go with this.

Comment 12

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

I don't think this change can break any tests so I'm not running through tryserver.

Pushed to fx-team:
  https://hg.mozilla.org/integration/fx-team/rev/750e90d5ee9a

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/750e90d5ee9a

Comment 14

[Marco Mucci [:MarcoM]]

Flagged for testing and verification.

Comment 15

[Masatoshi Kimura [:emk]]

How can I test this? Do I have to make a custom build without Metro support?

Comment 16

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Following the STR in comment 0 and comment 1 should be sufficient for now since the release channel currently doesn't have metroFx.

Once metroFx lands in release then you will only be able to test with custom non-metroFx builds.

Comment 17

[Masatoshi Kimura [:emk]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #16)
> Following the STR in comment 0 and comment 1 should be sufficient for now
> since the release channel currently doesn't have metroFx.

But the current release does not include this fix yet. So I have to use custom non-metroFx builds instead of the current release to follow steps described in comment #0 and #1. Am I missing something?

Comment 18

[Masatoshi Kimura [:emk]]

Nominating to uplift because non-metroFx side requires this fix.

Comment 19

[Brian R. Bondy [:bbondy]]

What is fixed here is only run for metro enabled builds. If you remove the Nightly metro enabled build, it'll cleanup after itself better so it doesn't get in the way of release Firefox non Metro launching. 

If there's another problem that needs code run from release Firefox, I think we need another bug posted.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Masatoshi, why do you think we should track it ? It seems it affected many releases and does not seems critical

Comment 21

[Masatoshi Kimura [:emk]]

Indeed the reported problem was fixed.

(In reply to Sylvestre Ledru [:sylvestre] from comment #20)
> Masatoshi, why do you think we should track it ? It seems it affected many
> releases and does not seems critical

Sorry, it was my misunderstanding.

Comment 22

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 8377785 [details] [diff] [review]
Patch v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Present in all metro-enabled builds
User impact if declined: Users who have a non-metro Firefox set as default and who then uninstall a metro-enabled Firefox will find that external links fail to launch the browser.
Testing completed (on m-c, etc.): Some local testing and some bake-time on m-c, though I imagine that since this affects the installer it isn't getting as much coverage as other m-c changes get.
Risk to taking this patch (and alternatives if risky): The changes in this patch are pretty minimal and easy to reason about. However, if some logic error snuck in, the installer could accidentally trash user registries. The alternative is to wait for this patch to reach release (meaning that Fx28 and Fx29 would continue to exhibit this issue).
String or IDL/UUID changes made by this patch: None

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/beade305b6f4

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

> However, if some logic
> error snuck in, the installer could accidentally trash user registries. The
> alternative is to wait for this patch to reach release (meaning that Fx28
> and Fx29 would continue to exhibit this issue).

So is this something we can get QA to verify on Nightly/Aurora before we consider for the wider Beta population? If not, we should leave this on 29 and get a full Beta cycle for testing rather than just a few weeks.

Comment 25

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Lukas Blakk [:lsblakk] from comment #24)
> > However, if some logic
> > error snuck in, the installer could accidentally trash user registries. The
> > alternative is to wait for this patch to reach release (meaning that Fx28
> > and Fx29 would continue to exhibit this issue).
> 
> So is this something we can get QA to verify on Nightly/Aurora before we
> consider for the wider Beta population? If not, we should leave this on 29
> and get a full Beta cycle for testing rather than just a few weeks.

Honestly I think the best way to verify this is to get more eyes on the patch. If something *is* wrong with the logic of it, the most likely symptom I can think of is the uninstaller failing to remove some registry entries. That's hard for QA to verify but also likely wouldn't be caught by additional time on Beta.

rstrong - I'm curious what your thoughts are on this patch. Does it seem safe to uplift to Beta at this stage in the cycle?

Comment 26

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

I don't think these changes require a full beta cycle and I do think the changes are insignificant enough to take on beta with very little risk. As for testing having both Tim and QA verify the registry keys values after are correct should be sufficient and have been in the past when we've uplifted similar patches.

Comment 27

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Steps to verify:

1. Install a non-metro Firefox (current release works for this purpose)
2. Install a patched metro-enabled Firefox (whichever build needs to be tested)
3. Set the Fx from 1 as default

Check the following registry keys:
  HKCR\FirefoxURL\shell\open\command
  HKCR\FirefoxHTML\shell\open\command

Each one should have a "Path" value pointing to the Fx installed in 1. Each one should have a "DelegateExecute" value with the UUID "{5100FEC1-212B-4BF5-9BF8-3E650FD794A3}" (Note that once bug 973103 is fixed, there should not be a "DelegateExecute" value at this point)

4. Uninstall the Fx from 2

The "DelegateExecute" values should now have been removed.

5. Open a URL from an external app (e.g. Thunderbird).

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8377785 [details] [diff] [review]
Patch v3

Thanks, good info.  Let's go ahead with uplift to today's Beta.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7324fbaa37ed

Comment 30

[u279076]

Flagging for verification in the next Beta.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/7324fbaa37ed

Comment 32

[Manuela Muntean [Away]]

Verified as fixed using the STR from comment 27. For the patched metro-enabled Firefox in step 2 I used Firefox 28 beta 6.

Comment 33

[Marco Mucci [:MarcoM]]

For testing and verification.  Reopen if any defects found.

Comment 34

[u279076]

Kamil, can you take a look at this?

Comment 35

[Kamil Jozwiak [:kjozwiak]]

Went through the following verification process using the following builds:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-02-27-03-02-03-mozilla-central/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-02-27-00-40-02-mozilla-aurora/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/28.0b6/win32/en-US/

Before going through the verification process, I reproduced the issue from comment #0 using an older build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013/12/2013-12-19-03-02-02-mozilla-central/

- Read through the entire ticket to understand the problem before verification
- Went through the added test cases from comment #27
- Ensured that "DelegateExecute" was removed from HKCR\FirefoxURL\shell\open\command
- Ensured that "DelegateExecute" was removed from HKCR\FirefoxHTML\shell\open\command
- Made sure that links from external applications work correctly once the metro-enabled browser has been uninstalled

Tested on both the X1 Carbon and the Surface Pro 2 without any issues