505925 - ExplicitAppUserModelID needs to be synced for all app resources

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Jim Mathies [:jimm]]

There's a new way Win7 identifies apps for association. See the docs here -

http://msdn.microsoft.com/en-us/library/dd378459(VS.85).aspx

I've added initial support in WinTaskbar for creating and assigning this for jump lists and assigning it to the process in nsWindow. However, we may need to add the id property to other items like shortcuts that get created. We'll likely need to expose this to other areas of the code and possibly to the install/uninstaller as well.

Comment 1

[Jim Mathies [:jimm]]

Looks like this is causing some problems for recent and frequent jump list display. We'll need to come up with something that blends this with our current protocol registration.

Comment 2

[Jim Mathies [:jimm]]

The code in bug 473045 now has a white wash solution for Firefox. We have the "Firefox" id embedded in a number of places, Fx's shell component, the installer/uninstaller/helper, and now down in widget. It would be nice if we could define this through the build or similar, and version it according msdn specs.

Comment 3

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

If you have a value that should be different based on branch / trunk / release / etc. and should be Firefox for releases you should be using MOZ_APP_DISPLAYNAME.

We already use MOZ_APP_DISPLAYNAME in the installer/uninstaller/helper where appropriate and the string Firefox where it is not (e.g. shell integ).

Comment 4

[Jim Mathies [:jimm]]

(In reply to comment #3)
> If you have a value that should be different based on branch / trunk / release
> / etc. and should be Firefox for releases you should be using
> MOZ_APP_DISPLAYNAME.
> 
> We already use MOZ_APP_DISPLAYNAME in the installer/uninstaller/helper where
> appropriate and the string Firefox where it is not (e.g. shell integ).

Don't we call the vista app registration apis through the helper with app id "Firefox"?

Comment 5

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

We do. The HKLM\Software\Clients\StartMenuInternet\ key for Firefox is FIREFOX.EXE (as the exe name is used for all browsers that register in StartMenuInternet) and is also part of the taking over defaults so there isn't much value in having the different values. It would also need different FirefoxHTML and FirefoxHTML to provide the ability to have multiple and it would still fall short due to this.

Comment 6

[Jim Mathies [:jimm]]

(In reply to comment #5)
> We do. The HKLM\Software\Clients\StartMenuInternet\ key for Firefox is
> FIREFOX.EXE (as the exe name is used for all browsers that register in
> StartMenuInternet) and is also part of the taking over defaults so there isn't
> much value in having the different values. It would also need different
> FirefoxHTML and FirefoxHTML to provide the ability to have multiple and it
> would still fall short due to this.

I get this. Of course now windows is using this id to store internal data structures specific to an instance/version of the app, so we are starting to see cases where our generic, non-versioned prog ids are causing problems.

With jump lists it doesn't look too bad so far, although i haven't actually built and installed multiple versions to see what issues crop up. Worst case scenario, a jump list shows up on the icon of of an older version of firefox not compatible with jump list links. Also, different versions may modify the same list. I still have to do some testing of this, I'm hoping ms uses the instance path internally as a differentiating value. 

Generally though, wouldn't versioning our protocol and file handling prog ids solve all these crazy issues with having to check to see if we "own something" when working in the install? If "FirefoxHTML" became "Mozilla.Firefox.File.3.5" for example, we'd never have to worry about other versions messing with our config. With Vista and up it becomes even simpler since we have the app registration apis now.

Comment 7

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

(In reply to comment #6)
> (In reply to comment #5)
> > We do. The HKLM\Software\Clients\StartMenuInternet\ key for Firefox is
> > FIREFOX.EXE (as the exe name is used for all browsers that register in
> > StartMenuInternet) and is also part of the taking over defaults so there isn't
> > much value in having the different values. It would also need different
> > FirefoxHTML and FirefoxHTML to provide the ability to have multiple and it
> > would still fall short due to this.
> 
> I get this. Of course now windows is using this id to store internal data
> structures specific to an instance/version of the app, so we are starting to
> see cases where our generic, non-versioned prog ids are causing problems.
I get this and agree it should definitely be fixed but I don't see how this has anything to do with app assoc reg. For example, I have 21 apps under RegisteredApplications and only one of them has a version.

>...
> Generally though, wouldn't versioning our protocol and file handling prog ids
> solve all these crazy issues with having to check to see if we "own something"
> when working in the install? If "FirefoxHTML" became "Mozilla.Firefox.File.3.5"
> for example, we'd never have to worry about other versions messing with our
> config. With Vista and up it becomes even simpler since we have the app
> registration apis now.
For starters, not as long as we allow side by side installations. As for it being crazy I highly suspect that trying to manage version numbers on these registry entries including cleaning them up when the install location no longer exists will be magnitudes crazier than just checking if the binary in the DefaultIcon or open/command is the one for this install location.

Comment 8

[Jim Mathies [:jimm]]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > We do. The HKLM\Software\Clients\StartMenuInternet\ key for Firefox is
> > > FIREFOX.EXE (as the exe name is used for all browsers that register in
> > > StartMenuInternet) and is also part of the taking over defaults so there isn't
> > > much value in having the different values. It would also need different
> > > FirefoxHTML and FirefoxHTML to provide the ability to have multiple and it
> > > would still fall short due to this.
> > 
> > I get this. Of course now windows is using this id to store internal data
> > structures specific to an instance/version of the app, so we are starting to
> > see cases where our generic, non-versioned prog ids are causing problems.
> I get this and agree it should definitely be fixed but I don't see how this has
> anything to do with app assoc reg. For example, I have 21 apps under
> RegisteredApplications and only one of them has a version.

app assoc reg. ids and user model ids have to be associated. You can't create a jump list with links in it unless the app your working from is the registered protocol handler for the link type. 

> 
> >...
> > Generally though, wouldn't versioning our protocol and file handling prog ids
> > solve all these crazy issues with having to check to see if we "own something"
> > when working in the install? If "FirefoxHTML" became "Mozilla.Firefox.File.3.5"
> > for example, we'd never have to worry about other versions messing with our
> > config. With Vista and up it becomes even simpler since we have the app
> > registration apis now.
> For starters, not as long as we allow side by side installations. As for it
> being crazy I highly suspect that trying to manage version numbers on these
> registry entries including cleaning them up when the install location no longer
> exists will be magnitudes crazier than just checking if the binary in the
> DefaultIcon or open/command is the one for this install location.

I don't follow this. The whole point of a versioned progid is to alleviate issues with side-by-side installs of multiple versions. When the same version is installed side-by-side, the system only knows about one of them - the one that is marked as the default browser.

If we had versioned prog ids, older versions could install along side new versions and the two would never have to know anything about each other. On updates, the updater would delete the old prog id version (if it owned it) and create the new version on the update. 

Each version would be responsible for registering it's prog id, and removing it on uninstall.

One example I can think of where versioned ids would solve a problem - the DDE issues we have.

Comment 9

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

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > We do. The HKLM\Software\Clients\StartMenuInternet\ key for Firefox is
> > > > FIREFOX.EXE (as the exe name is used for all browsers that register in
> > > > StartMenuInternet) and is also part of the taking over defaults so there isn't
> > > > much value in having the different values. It would also need different
> > > > FirefoxHTML and FirefoxHTML to provide the ability to have multiple and it
> > > > would still fall short due to this.
> > > 
> > > I get this. Of course now windows is using this id to store internal data
> > > structures specific to an instance/version of the app, so we are starting to
> > > see cases where our generic, non-versioned prog ids are causing problems.
> > I get this and agree it should definitely be fixed but I don't see how this has
> > anything to do with app assoc reg. For example, I have 21 apps under
> > RegisteredApplications and only one of them has a version.
> 
> app assoc reg. ids and user model ids have to be associated. You can't create a
> jump list with links in it unless the app your working from is the registered
> protocol handler for the link type. 
App assoc reg uses a string name for the app (e.g. Firefox) and I believe what you are referring to is the handler itself which is different (e.g. FirefoxURL, etc. currently).

What does IE use?

> > 
> > >...
> > > Generally though, wouldn't versioning our protocol and file handling prog ids
> > > solve all these crazy issues with having to check to see if we "own something"
> > > when working in the install? If "FirefoxHTML" became "Mozilla.Firefox.File.3.5"
> > > for example, we'd never have to worry about other versions messing with our
> > > config. With Vista and up it becomes even simpler since we have the app
> > > registration apis now.
> > For starters, not as long as we allow side by side installations. As for it
> > being crazy I highly suspect that trying to manage version numbers on these
> > registry entries including cleaning them up when the install location no longer
> > exists will be magnitudes crazier than just checking if the binary in the
> > DefaultIcon or open/command is the one for this install location.
> 
> I don't follow this. The whole point of a versioned progid is to alleviate
> issues with side-by-side installs of multiple versions. When the same version
> is installed side-by-side, the system only knows about one of them - the one
> that is marked as the default browser.
> 
> If we had versioned prog ids, older versions could install along side new
> versions and the two would never have to know anything about each other. On
> updates, the updater would delete the old prog id version (if it owned it) and
> create the new version on the update. 
I am referring to side by side of the same version. Think of nightly builds for example. Though I guess a really long version string could be used and updated with each nightly which would be an even worse nightmare than the current "own" method.

> Each version would be responsible for registering it's prog id, and removing it
> on uninstall.
> 
> One example I can think of where versioned ids would solve a problem - the DDE
> issues we have.
How specifically? My experience with troubleshooting our current DDE issues has nothing to do with the registry keys unless I am mistaken.

Comment 10

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

(In reply to comment #9)
> > One example I can think of where versioned ids would solve a problem - the DDE
> > issues we have.
> How specifically? My experience with troubleshooting our current DDE issues has
> nothing to do with the registry keys unless I am mistaken.
To be clear, there are bugs in the OS (as noted in Bug 491947) and our Startup code that break DDE and as a workaround to these we can remove DDE support. Using version numbers in these names doesn't fix that any more than removing DDE support.

Comment 11

[Jim Mathies [:jimm]]

> With jump lists it doesn't look too bad so far, although i haven't actually
> built and installed multiple versions to see what issues crop up. Worst case
> scenario, a jump list shows up on the icon of an older version of firefox
> not compatible with jump list links. Also, different versions may modify the
> same list. I still have to do some testing of this, I'm hoping ms uses the
> instance path internally as a differentiating value. 

So it turns out that this problem of sharing jump lists across versions is real. Win7 doesn't use the exe path to differentiate. I've installed two copies side by side (now that we have win7 sdk builds on try!) and they modify the same jump list, so I'll have to use a versioned app local id. 

The good news is, I've since reworked the upper level patch for fx to get around the DDE problem and the protocol handler id issues with adding links to jump lists.

Comment 12

[Jim Mathies [:jimm]]

Attachment: add app.ini entry v.1

Comment 13

[Jim Mathies [:jimm]]

Attachment: use appinfo for reg data v.1

Comment 14

[Jim Mathies [:jimm]]

Comment on attachment 440619 [details] [diff] [review]
add app.ini entry v.1

Not sure if ifdef'ing here is acceptable.

Comment 15

[Benjamin Smedberg]

Comment on attachment 440619 [details] [diff] [review]
add app.ini entry v.1

What kind of thing is a model ID? Does it need to be in a specific format, or can we just use the application ID that's already in application.ini?

Comment 16

[Benjamin Smedberg]

Comment on attachment 440619 [details] [diff] [review]
add app.ini entry v.1

This at least need better documentation. But what does "may be null" mean? From the implementation, it looks like it may be empty, but will never be null.

Comment 17

[Jim Mathies [:jimm]]

(In reply to comment #15)
> (From update of attachment 440619 [details] [diff] [review])
> What kind of thing is a model ID? Does it need to be in a specific format, or
> can we just use the application ID that's already in application.ini?

It should be user specified so that xul runner apps can customize it based on the app and version. A generic, global id doesn't provide needed granularity, and breaks with the formatting conventions specified on MSDN.

As far as docs goes, I'll mark the bug as dev doc needed after this lands.

The null comment on the nsAString I can remove, copy paste error. Other than that, I think this patch should land as is, unless you have additional issues.

Comment 18

[Jim Mathies [:jimm]]

Comment on attachment 440621 [details] [diff] [review]
use appinfo for reg data v.1

This is pretty straight forward, default to the application.ini version first, so that xul runner apps can override the default settings. I've also pulled the #define junk and now rely on other application.ini settings.

Comment 19

[Jim Mathies [:jimm]]

Comment on attachment 440619 [details] [diff] [review]
add app.ini entry v.1

Spoke with bsmedberg, we're going to skip the custom value for now and rely on the default values in the ini.

Comment 20

[Jim Mathies [:jimm]]

Attachment: use appinfo for reg data v.1

Comment 21

[Jim Mathies [:jimm]]

Crap, I left a nsCOMPtr<nsIWinAppHelper> winHelper in there, I'll pull that from the final.

Comment 23

[[e-mail]]

Comment on attachment 442150 [details] [diff] [review]
use appinfo for reg data v.1

>   *_retval = PR_FALSE;
> 
>   if (!mJumpListMgr)
>     return NS_ERROR_NOT_AVAILABLE;
> 
>   if(sBuildingList)
>     AbortListBuild();
> 
>-  if (SUCCEEDED(mJumpListMgr->DeleteList(gMozillaJumpListIDGeneric)))
>+  nsAutoString uid;
>+  if (!WinTaskbar::GetAppUserModelID(uid)) {
>+    *_retval = PR_FALSE;
Nit: *_retval is already PR_FALSE

>+  nsAutoString uid;
>+
>+  nsCOMPtr<nsIXULAppInfo> appInfo =
>+    do_GetService("@mozilla.org/xre/app-info;1");
>+  nsCOMPtr<nsIWinAppHelper> winHelper;
>+  if (!appInfo)
>+    return PR_FALSE;
>+
>+  // The default, pulled from application.ini:
>+  // 'vendor.application.version'
>+  if (!uid.Length()) {
This will always be true, given that you only just created it.

>+      uid.Append(NS_ConvertASCIItoUTF16(val));
AppendASCIItoUTF16

>+      uid.AppendLiteral(".");
Or possibly use Append(PRUnichar('.'))

>+  if (uid.Length()) {
We normally use !IsEmpty()

>+    aId.Assign(uid);
Is there in fact any reason to use uid when you could use aId all along?

>+  if (!funcAppUserModelID) {
>+    ::FreeLibrary(hDLL);
>+    return PR_FALSE;
>+  }
>+
>+  if (funcAppUserModelID && SUCCEEDED(funcAppUserModelID(uid.get())))
Duplicate null-check? (The first one seems more redundant to me.)

Comment 24

[Jim Mathies [:jimm]]

Attachment: use appinfo for reg data v.2

Comment 25

[Jim Mathies [:jimm]]

Comment on attachment 442212 [details] [diff] [review]
use appinfo for reg data v.2

comments addressed

Comment 27

[[e-mail]]

Comment on attachment 442212 [details] [diff] [review]
use appinfo for reg data v.2

[Note that I don't yet have a build, let alone test, environment for W7.]

>   if (!mJumpListMgr)
>     return NS_ERROR_NOT_AVAILABLE;
> 
>   if(sBuildingList)
>     AbortListBuild();
> 
>-  if (SUCCEEDED(mJumpListMgr->DeleteList(gMozillaJumpListIDGeneric)))
>+  nsAutoString uid;
>+  if (!WinTaskbar::GetAppUserModelID(uid)) {
>+    return NS_OK;
>+  }
[Dunno which bracing style is right, but inconsistency looks wrong.]

>+  return aId.IsEmpty() ? PR_FALSE : PR_TRUE;
[Could use ! operator]

Comment 28

[Jim Mathies [:jimm]]

Attachment: use appinfo for reg data v.3

Comment 29

[Jim Mathies [:jimm]]

Attachment: use appinfo for reg data v.3

Added a new info property interface for taskbar features. There are a number of patches built on top of this patch, all of which have to check the os version and build the app id. So I centralized that code here.

Comment 30

[Jim Mathies [:jimm]]

(Based on how this is being used in other patches, I'll be changing the imp of WinTaskbarInfo to thread safe.)

Comment 31

[[e-mail]]

Comment on attachment 443358 [details] [diff] [review]
use appinfo for reg data v.3

>+  if (nsWindow::GetWindowsVersion() < WIN7_VERSION)
>+    *aResult = PR_FALSE;
>+  else
>+    *aResult = PR_TRUE;
[Could write *aResult = nsWindow::GetWindowsVersion() >= WIN7_VERSION; or at least use ? PR_FALSE : PR_TRUE like you did for GetAppUserModelID]

>+#include "nsString.h"
Unused?

>+  virtual ~WinTaskbarInfo();
[This probably doesn't need to be virtual. The one case where it matters is when you want to inherit from this class but without overriding Release.]

Comment 32

[Jim Mathies [:jimm]]

Attachment: patch v.3

Comment 33

[Jim Mathies [:jimm]]

Attachment: patch v.3

- nix'd NS_IF_RELEASE in the dtor.

Comment 34

[Jim Mathies [:jimm]]

Attachment: patch v.3

(In reply to comment #31)
> (From update of attachment 443358 [details] [diff] [review])
> >+  if (nsWindow::GetWindowsVersion() < WIN7_VERSION)
> >+    *aResult = PR_FALSE;
> >+  else
> >+    *aResult = PR_TRUE;
> [Could write *aResult = nsWindow::GetWindowsVersion() >= WIN7_VERSION; or at
> least use ? PR_FALSE : PR_TRUE like you did for GetAppUserModelID]

updated.

> >+#include "nsString.h"
> Unused?

I need it for GetAppUserModelID.

> >+  virtual ~WinTaskbarInfo();
> [This probably doesn't need to be virtual. The one case where it matters is
> when you want to inherit from this class but without overriding Release.]

nixed!

requesting sr? from roc on the interface changes.

Comment 35

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/8ba23ea5df29

Dev docs - for xulrunner apps, the grouping id is now based on app.ini settings, and is of the format "vendor.application.version". There are related settings in the installer that need to sync to this for things to work. (bug 521141)

Comment 36

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/nsIWinTaskbar