352424 - Use the Vista Default Application API

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Doug Turner (:dougt)]

Setting the default application on Vista doesn't seam to work.

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

not finish.  still includes MessageBox's for debuggin.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Do we really need this for 2.0, or can we fit it into a security and stability release? If we can, I would vote to make it non-blocking, though we'd take a patch if it makes it before freeze or if we have to respin.

Comment 3

[Doug Turner (:dougt)]

I think i would agree with your vote -- this patch isn't ready just yet.  the patch needs to be tested against rob strong's installer changes.

Comment 4

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

*** Bug 339306 has been marked as a duplicate of this bug. ***

Comment 5

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

A couple of notes:

It appears that our app still has a ddeexec hack where it removes the ddeexec key on shutdown and adds it back on startup... this is bad / wrong on so many
levels. If the user is not admin it can't do this, if the app crashes it can't remove it, etc. This really should be removed if for no other reason than
consistent behavior for admin and non-admin users. I suppose it could be doing this under HKCU for non-admin users but iirc it doesn't.

Currently the app adds the protocol handlers which doesn't make sense in Vista so I've done this in the installer.

I've tested this patch along with the patch in bug 336469 and it worked well. I think we are going to have to come up with a better string to use to register than brandFullName but I may be incorrect in this... it depends on how Vista handles the StartMenuInternet shortcut. Also, the app should not be touching the HKEY_CLASSES_ROOT keys on Vista since Vista handles this for the app.

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.2

cleaned up version of patch v.1.  I did not include the removal of the DDE stuff.  I think we can do this later since we want to try to make the 2.0 train.

Comment 7

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

I recant the statement about not setting keys under classes since we need to do so for side by side installs though they are different keys. Turns out that the protocol and file handlers we use will need to be checked to verify that it refers to this install location and if not set to this install location either in HKEY_CURRENT_USER or HKEY_LOCAL_MACHINE. Otherwise, when set as default is selected we will just fail and prompt again on next launch.

Comment 8

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

The app is going to need to set new reg keys for protocol handlers. IE has one for each file and protocol handler so we may want to bite the bullet and add new reg keys for individual file handlers instead of just reusing FirefoxHTML. Suggested names that I already have in the installer

FirefoxGopher
FirefoxFTP
FirefoxHTTP
FirefoxHTTPS

This is inline with our current FirefxHTML but we could choose to abandon it and use something mroe similar to what IE uses. The value in using new names for each file handler is that we can specify individual names to be displayed in Set User Defaults (e.g. SUD)... for example IE uses HTML Document and without having individual keys we would display HTML File, HTM File, XHTML File, etc. for the file handlers.

A couple of the names IE is using
IE.AssocFile.HTM
IE.HTTP
and so on...

Comment 9

[Doug Turner (:dougt)]

this should be done by the installer, right?

Comment 10

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

No, per comment #7

If one version of the app is installed and then another version of the app is installed the second install will take over those keys. Then if the second app is uninstalled there is no way to set the original install as default without re-installing. Also, if the first install is intended to be the default the second install is going to make itself the default. There are other scenarios as well but the gist is the app is going to have to set these keys as well.

Comment 11

[Doug Turner (:dougt)]

Comment on attachment 238625 [details] [diff] [review]
patch v.2

this doesn't work for side by side.

Comment 12

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

Doug, the following is the action plan after talking with Schrep... no support for side by side on Vista for now. The installer will alway force to install into the location of an existing install on Vista. This should lessen the complexity quite a bit and mitigate risk. We will still need to be able to write to HKCU\Software\Classes when we can't write to HKLM\Software\Classes. I'll send you more details later when the details become more clear

Comment 13

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

The main thing that needs to be changed in this patch is the use of Firefox instead of brandFullName for the Vista API calls. Also, the protocol DefaultIcon reg values should specify 0 instead of 1 for the resource since we don't want to show the file icon for protocols

Comment 14

[Doug Turner (:dougt)]

the protocol DefaultIcon reg values should just be for vista right?

The other thing is that if we don't use brandFullName, then using a bon echo build will not work (this side by side case will not work, right)

Doug

Comment 15

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

(In reply to comment #14)
> the protocol DefaultIcon reg values should just be for vista right?
No, they should be for all but hold off on it... they just aren't shown except in Vista

> The other thing is that if we don't use brandFullName, then using a bon echo
> build will not work (this side by side case will not work, right)
Per our discussion last Friday it should use DDE_NAME though you can use anything that has the string Firefox... we can't use brandFullName because when using a bon echo or minefield build it will not work since brandFullName will be Bon Echo or Minefield... so, this has nothing to do with side by side installs.

Comment 16

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

How about the following?

(In reply to comment #5)
...
> Currently the app adds the protocol handlers which doesn't make sense in Vista
> so I've done this in the installer.
> 
> I've tested this patch along with the patch in bug 336469 and it worked well. I
> think we are going to have to come up with a better string to use to register
> than brandFullName but I may be incorrect in this... it depends on how Vista
> handles the StartMenuInternet shortcut. Also, the app should not be touching
> the HKEY_CLASSES_ROOT keys on Vista since Vista handles this for the app.

Comment 17

[Mike Schroepfer]

Ok - we are cutting 1.8.1 in minutes so this will have to defer for a 2.0.0.1 branch.

Comment 18

[Doug Turner (:dougt)]

Attachment: patch v.3

Uses DDE_NAME.

Comment 19

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

Comment on attachment 239188 [details] [diff] [review]
patch v.3

>Index: src/nsWindowsShellService.cpp
...
>+    nsCOMPtr<nsIStringBundleService> bundleService(do_GetService("@mozilla.org/intl/stringbundle;1"));
>+    if (!bundleService)
>+      return NS_ERROR_FAILURE;
>+    
>+    nsCOMPtr<nsIStringBundle> brandBundle;
>+    nsresult rv = bundleService->CreateBundle(BRAND_PROPERTIES, getter_AddRefs(brandBundle));
>+    if (NS_FAILED(rv))
>+      return rv;
>+    
>+    nsXPIDLString brandFullName;
>+    brandBundle->GetStringFromName(NS_LITERAL_STRING("brandFullName").get(), getter_Copies(brandFullName));
>+    
>+    hr = pAAR->QueryAppIsDefaultAll(AL_EFFECTIVE,
>+                                    brandFullName.get(),
>+                                    aIsDefaultBrowser);
That isn't DDE_NAME

Comment 20

[Doug Turner (:dougt)]

Attachment: the real patch v.3

wrong patch.

Comment 21

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

Comment on attachment 239189 [details] [diff] [review]
the real patch v.3

>? x
>Index: nsWindowsShellService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/shell/src/nsWindowsShellService.cpp,v
>retrieving revision 1.21.2.2
>diff -u -1 -0 -r1.21.2.2 nsWindowsShellService.cpp
>--- nsWindowsShellService.cpp	28 Jun 2006 23:44:17 -0000	1.21.2.2
>+++ nsWindowsShellService.cpp	19 Sep 2006 16:45:51 -0000
>@@ -185,20 +185,21 @@
> 
>   PRInt32 flags;
> } SETTING;
> 
> #define SMI "SOFTWARE\\Clients\\StartMenuInternet\\"
> #define CLS "SOFTWARE\\Classes\\"
> #define DI "\\DefaultIcon"
> #define SOP "\\shell\\open\\command"
> #define DDE "\\shell\\open\\ddeexec\\"
> #define DDE_NAME "Firefox" // This must be kept in sync with ID_DDE_APPLICATION_NAME as defined in splash.rc
>+#define DDE_NAME_W L"Firefox"
You defined this and then didn't use it

Comment 22

[Doug Turner (:dougt)]

Comment on attachment 239189 [details] [diff] [review]
the real patch v.3

okay.  if that is the only change, r= and I will land it on the trunk.

Comment 23

[Doug Turner (:dougt)]

Attachment: patch v.4

removing unused wide define.

Comment 24

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

Comment on attachment 239193 [details] [diff] [review]
patch v.4

Doug, this won't work... you need a LPCWSTR. Also, now that the fix for the ddeexec hack you no longer need to protect in Observe. It looks like that would have also prevented IsDefaultBrowser in Observe from being called on Vista.

Comment 25

[Doug Turner (:dougt)]

Attachment: patch v.5

trunk patch including wide string.

Comment 26

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

Thanks Doug... the patch now compiles. I did a real quick review and it looks fine but you might want to get another set of eyes on this due to my lack of sleep. Since DDE_NAME is not going to be reused the name should probably reflect its actual use. Perhaps APP_REG_NAME?

Comment 27

[(not reading, please use [e-mail] instead)]

doug / robert.  here come questions.  please forgive my Vista / COM ignorance in advance!

1) for non-Vista, in nsWindowsShellService::SetDefaultBrowser(), we write the existing settings to MOZ_BACK_REGISTRY, and then in nsWindowsShellService::RestoreFileSettings(), we restore them.

So for Vista, shouldn't that mean we should be calling ClearUserAssociations() in nsWindowsShellService::RestoreFileSettingsVista()?

2)  nsWindowsShellService::SetDefaultBrowser() takes a bool, aForAllUsers.  Shouldn't this be passed through to SetDefaultBrowserVista(), and if false, shouldn't we calling SetAppAsDefault() with the appropriate ASSOCIATIONLEVEL?

3)  I am guess thing that the IApplicationAssociationRegistration interface is duplicated in nsWindowsShellService.h so that will compile on versions of versions of windows without shobjidl.h.  is that right?  But I think we should move it to nsWindowsShellService.cpp (instead of having it in nsWindowsShellService.h)

4)  Any reason not to use CComPtr?

Comment 28

[Doug Turner (:dougt)]

1.  I think we are going to end the ablity to roll back (RestoreFileSettings).  Rob knows more about this.  

2.  yes, we should be doing that, but we should also attempt to write all of the defaults to the registry just like the installer (or at least update the paths).   This will allow side-by-side installs.  Basically for this patch, since we were shooting for 2.0, we dropped the side-by-side case.

3.  yes, this interface is not present on any of the current development environments.  when we migrate to newer versions of the SDK, we will be able to remove this stuff.  I have no problem moving it to the .cpp if you think it is a better place. 

Comment 29

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

1. The reg key backup / restore is broken under several conditions and is something we should not do on Vista anyways. I haven't tested ClearUserAssociations yet and it is what we want to use. We would only want to call that when we are default and we would need to check if additional logic to handle the individual associations is needed when we are only default for a subset of the associations.

2. By default we don't want to do this. Perhaps it could be added it as a command line handler for now.

Comment 30

[(not reading, please use [e-mail] instead)]

robert / dougt, thanks for the quick responses.

I've provided the other set of eyes, and I think we should that code from the .h to the .cpp.  I defer to (a rested) robert to give the official review for this patch, as he understands this code the best.

Comment 31

[Doug Turner (:dougt)]

Attachment: patch v.6

renames DDE_NAME_W to rob's suggestion.
moves vista defines from .h to .cpp.

Comment 32

[(not reading, please use [e-mail] instead)]

robert asked if I had any other concerns about this patch.

I don't assumming that we are ok with this:

from http://windowssdk.msdn.microsoft.com/en-us/library/aa360791.aspx

IApplicationAssociationRegistration::SetAppAsDefaultAll "Sets an application as the default for all of the registered assocations of any type and level for that application."

If we are ok with that, no matter what that params are for SetDefaultBrowser(PRBool aClaimAllTypes, PRBool aForAllUsers), then I'm ok.

Comment 33

[Doug Turner (:dougt)]

This isn't what it does on RC1.  Even with an admin account, i can not change the defaults of new or existing users of the system. 

I think the documentation you are reading might need to be updated as the vista application cookbook simply says:

"Pass in the name of the registered app and it will set all the defaults registered to the application."

I wonder if this is because UAC protects different users.  Yet another thing to ask Microsoft about.

Comment 34

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

The All in SetAppAsDefaultAll I believe refers to all of the defaults for the app and not for all users. It doesn't specifically state whether it should be for all users or only the current user

Comment 35

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

Comment on attachment 239432 [details] [diff] [review]
patch v.6

Thanks Doug

Comment 36

[Doug Turner (:dougt)]

Comment on attachment 239432 [details] [diff] [review]
patch v.6

Fixed on Trunk

Checking in nsWindowsShellService.cpp;
/cvsroot/mozilla/browser/components/shell/src/nsWindowsShellService.cpp,v  <--  nsWindowsShellService.cpp
new revision: 1.33; previous revision: 1.32
done
Checking in nsWindowsShellService.h;
/cvsroot/mozilla/browser/components/shell/src/nsWindowsShellService.h,v  <--  nsWindowsShellService.h
new revision: 1.9; previous revision: 1.8

This does not address side-by-side installs.  I have opened bug 353814 for this issue.

Comment 37

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

Doug, since this is fixed on trunk do you want to resolve this as fixed or are you planning on additional work to be done in this bug for the trunk?

Comment 38

[Mike Connor [:mconnor]]

Wanted ASAP on the 1.8.1 branch, not a strict blocker for 1.8.1.1

Comment 39

[Jonathan Yaniv]

This still happens. So fix it please. 

Comment 40

[Jonathan Yaniv]

Even happens on 2.0.0.1. 

Comment 41

[Jonathan Yaniv]

Why not change the registry key value that Windows Vista uses to find the default browser? Instead of pointing to IE, change it to the path that Firefox uses. Its not that hard!!!

Comment 42

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

We know it isn't fixed yet. There is still lots more to be done. It is planned to be fixed for 2.0.0.2. Also, see bug 352420 for some of the additional bugs that need to be fixed.

Comment 43

[Andreas Wolf]

It doesnt work from within Firefox, but if you use the Tool Vista gives you, you can set 9 (i donnt know why 9 but it says so) priviledges to firefox.
After that it works like a charm.

Andi

Comment 44

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #42)
> We know it isn't fixed yet. There is still lots more to be done. It is planned
> to be fixed for 2.0.0.2. Also, see bug 352420 for some of the additional bugs
> that need to be fixed.

If that's the case, should this be RESO FIXED as it is right now?

Comment 45

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

(In reply to comment #44)
> If that's the case, should this be RESO FIXED as it is right now?
I changed the summary so it doesn't infer this bug covers everything needed.

Comment 46

[Daniel Veditz [:dveditz]]

If this patch is good for the branch please ask for approval, or create an appropriate branch-merged patch for this blocking bug.

Comment 47

[Doug Turner (:dougt)]

Attachment: patch v.6 (For Branch)

This is the untested branch patch based on v.6.  These needs to be tested with robert's changes a bit before landing (right robert?)

Comment 48

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

I already have a patch that includes the changes in v.6 along with the additional changes required.

Comment 49

[(not reading, please use [e-mail] instead)]

fixed on the branch.

Comment 50

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

To verify Start -> Default Programs -> Set your default programs and verify that setting Firefox as the default actually sets Firefox as the default program for you.

Comment 51

[Carsten Book [:Tomcat]]

verified fixed for 1.8.1.2 using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.2) Gecko/2007020823 Firefox/2.0.0.2 (RC2)

Firefox 2.0.0.2 has all defaults under Start -> Default Programs -> Set your default program (like https, gopher, xht, Start Menu Link, etc)

Comment 52

[pls.hermes]

(In reply to comment #1)
> Created an attachment (id=238356) [details]
> patch v.1
> 
> not finish.  still includes MessageBox's for debuggin.
> 

(In reply to comment #0)
> Setting the default application on Vista doesn't seam to work.
> 
sorry but even if at first sight it seems impossible to set Firefox and Thunderbird as default programs in Vista, the workaround is to disable UAC (user account control first),reboot, set again FF and Thunderbird as default, done. Now you can if you want turn UAC on again.

Comment 53

[Doug Turner (:dougt)]

Comment on attachment 251226 [details] [diff] [review]
patch v.6 (For Branch)

clearing flag.