1260483 - Firefox displays the wrong app name for protocol schemes associated with Universal apps on Windows 10 (e.g. Microsoft Office)

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[firefox.bug.filer]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2486.0 Safari/537.36 Edge/14.14309

Steps to reproduce:

1. Install a Universal app on Windows, like Office (e.g. Excel Mobile).
2. Ensure that it is the default handler for the protocol scheme it supports (e.g. ms-excel for Excel Mobile)
3. Start FireFox
4. Open Options->Applications
5. Scroll down to ms-excel (or whatever protocol scheme is appropriate for the Universal app you are using)
6. Look at the dropdown


Actual results:

The Universal app's name is listed as "TWINUI" 


Expected results:

It should be listed as the actual name of the application.

Comment 1

[firefox.bug.filer]

I debugged the issue and it seems that GetApplicationDescription (http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/win/nsOSHelperAppService.cpp) calls QueryCurrentDefault (https://msdn.microsoft.com/en-us/library/windows/desktop/bb776336(v=vs.85).aspx).

For Universal apps (like Excel Mobile), it gets back a value like this:
AppXexdj7z4pw3j58v673rcmzydf8mq7th61

It then calls nsOSHelperAppService::GetDefaultAppInfo, which opens this registry key:
HKCR\AppXexdj7z4pw3j58v673rcmzydf8mq7th61\shell\open\command

…and tries to read the "(Default)" value to get the "description of the type."  In the Excel Universal case, this value is empty/null/not set so FF then tries to read the DelegateExecute value.  For Excel Universal this is set to be {4ED3A719-CEA8-4BD9-910D-E252F997AFC2}.

It then tries to open HKCU\CLSID\{4ED3A719-CEA8-4BD9-910D-E252F997AFC2}\InProcServer32 and read the default value.  It is set to %SystemRoot%\system32\twinui.dll.

It then looks at that binary on disk and reads the "File Description" string, which is set to TWINUI for twinui.dll.

I don't know if there was ever Microsoft-supported documentation that said this was the way to find the name of the associated program, but I kind of doubt it, given the registry groveling involved.

I think the supported way (which would yield the right answer in this case) is to call the AssocQueryString (https://msdn.microsoft.com/en-us/library/windows/desktop/bb773471(v=vs.85).aspx) function instead, passing ASSOCSTR_FRIENDLYAPPNAME as the second parameter.  This appears to be what Chrome does, and it gets the correct display name for Universal apps.

Note that this bug does not seem to desktop for Win32 apps, only for Universal apps.

Also, don't let the "Excel Mobile" name fool you, you can install this universal app on a desktop machine.

This problem appears to occur for any Universal app that is associated with a protocol/extension, not just Excel. I only used that as an example.

Comment 2

[firefox.bug.filer]

Attachment: Screenshot of the confusing/bad string

Comment 3

[Loic]

Ted, I see you worked on nsOSHelperAppService in the past (bug 1171560). As the reporter provided some details about the code issue, could you check, please? (or set the NI flag to someone who knows this part of code)

Comment 4

[(not currently active) Ted Mielczarek]

Sorry, that was just stubbing it out to make it build on iOS, not actually implementing anything useful. I don't know offhand anyone that knows this code. CCing a few folks with Windows platform knowledge though, maybe they can help.

Comment 5

[Loic]

Gijs, is this bug about file handling in your scope?

Comment 6

[:Gijs (he/him)]

This code was introduced in bug 772711 by Brian Bondy, and reviewed by Jim Mathies. That bug already noted this problem, and bug 772863 was filed as a follow-up.

I don't know if the API described in comment #1 could also be used to determine the command to execute for the protocol / file, but I guess it's worth looking at either way. 

I'm absolutely swamped, don't know my way around these APIs, and don't have time to do the looking, never mind the implementing. Brian is at Brave right now, AIUI. Maybe Jim or Chris can help.

Comment 7

[Chris H-C :chutten|PTO (back March 17)]

The API from comment #1 ought to work, as in addition to the mentioned ASSOCSTR_FRIENDLYAPPNAME there's also a ASSOCSTR_FRIENDLYDOCNAME for getting the document type string (see MSDN https://msdn.microsoft.com/en-us/library/windows/desktop/bb762475%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396 ). The API's good for WinXP+, which means we should be clear to use it.

HOWEVER, there is a caveat for protocol->app mappings, as those are Win8 and up, only (see chromium's code https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/shell_integration_win.cc&q=ASSOCSTR_FRIENDLY&sq=package:chromium&type=cs&l=174 ). This is fine, as this is only for Win8+ apps.

This API bears absolutely no resemblance to how we currently look for string associations in nsOSHelperAppService. This might mean we can perform a concise rewrite and modernization of this code... or that maybe we should tread lightly and leave as much as possible alone.

bug 772863 looks to have a minimally-working patch that we might be able to take right away, but I haven't looked at it from the point of view of trying to apply it.

I might have some time to look at this, so I can take it. I'll leave the ni? for :jimm so he can contribute his $0.02 if he so wishes :)

Comment 8

[Jim Mathies [:jimm]]

(In reply to Chris H-C :chutten from comment #7)
> The API from comment #1 ought to work, as in addition to the mentioned
> ASSOCSTR_FRIENDLYAPPNAME there's also a ASSOCSTR_FRIENDLYDOCNAME for getting
> the document type string (see MSDN
> https://msdn.microsoft.com/en-us/library/windows/desktop/bb762475%28v=vs.
> 85%29.aspx?f=255&MSPPError=-2147217396 ). The API's good for WinXP+, which
> means we should be clear to use it.
> 
> HOWEVER, there is a caveat for protocol->app mappings, as those are Win8 and
> up, only (see chromium's code
> https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/
> shell_integration_win.cc&q=ASSOCSTR_FRIENDLY&sq=package:
> chromium&type=cs&l=174 ). This is fine, as this is only for Win8+ apps.
> 
> This API bears absolutely no resemblance to how we currently look for string
> associations in nsOSHelperAppService. This might mean we can perform a
> concise rewrite and modernization of this code... or that maybe we should
> tread lightly and leave as much as possible alone.

If we can query for the right string via an api vs. searching through the registry I'm a fan of that. Check for regressions in the application associations section of preference, and in the open with dialog that pops up for unknown file types.

Comment 9

[Jim Mathies [:jimm]]

Note bug 772863 was filed against metrofx, so I've closed it out. Anything we do should happen here.

Comment 10

[Chris H-C :chutten|PTO (back March 17)]

Attachment: MozReview Request: bug 1260483 - Use AssocQueryString to get friendly protocol handler names r?jimm

In Win8+, AssocQueryString supports ASSOCF_IS_PROTOCOL which simplifies fetching
the friendly application name for a given protocol/scheme. For "Universal" apps,
this simplified mechanism is required to get something other than TWINUI.

Review commit: https://reviewboard.mozilla.org/r/44147/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44147/

Comment 11

[Jim Mathies [:jimm]]

Comment on attachment 8737862 [details]
MozReview Request: bug 1260483 - Use AssocQueryString to get friendly protocol handler names r?jimm

https://reviewboard.mozilla.org/r/44147/#review40763

::: uriloader/exthandler/win/nsOSHelperAppService.cpp:171
(Diff revision 1)
>    NS_ConvertASCIItoUTF16 buf(aScheme);
>  
> -  // Vista: use new application association interface
> +  if (mozilla::IsWin8OrLater()) {
> +    wchar_t result[1024];
> +    DWORD resultSize = 1024;
> +    HRESULT hr = AssocQueryString(0x1000 /* ASSOCF_IS_PROTOCOL */,

Is ASSOCF_IS_PROTOCOL not defined by our sdk? Looks like it should be - 

https://msdn.microsoft.com/en-us/library/windows/desktop/bb762471(v=vs.85).aspx

Comment 12

[Jim Mathies [:jimm]]

I'd like to take this for a spin, need to build up mc on my win 8.1 laptop. will get back to this later today.

Comment 13

[Chris H-C :chutten|PTO (back March 17)]

https://reviewboard.mozilla.org/r/44147/#review40763

> Is ASSOCF_IS_PROTOCOL not defined by our sdk? Looks like it should be - 
> 
> https://msdn.microsoft.com/en-us/library/windows/desktop/bb762471(v=vs.85).aspx

I thought so, too, and that I might have to compile-guard around it for >= Win8... but it turns out, even on my Win10 machine it wasn't defined, as I had expected it to be, in shlwapi.h

Comment 14

[Jim Mathies [:jimm]]

Comment on attachment 8737862 [details]
MozReview Request: bug 1260483 - Use AssocQueryString to get friendly protocol handler names r?jimm

https://reviewboard.mozilla.org/r/44147/#review41623

I see this api working, although I think there are outstanding bugs. On windows 8 for example, it doesn't get called for the built-in pdf viewer. I still see that listed as TWINUI.

Comment 15

[Jim Mathies [:jimm]]

I guess the pdf viewer would be covered by the file extension search which isn't included her since we're dealing with protocol handlers only. If you don't mind though I'd appreciate a follow up if you're not interested in testing/fixing file handlers here too.

Comment 16

[Chris H-C :chutten|PTO (back March 17)]

(In reply to Jim Mathies [:jimm] from comment #15)
> I guess the pdf viewer would be covered by the file extension search which
> isn't included her since we're dealing with protocol handlers only. If you
> don't mind though I'd appreciate a follow up if you're not interested in
> testing/fixing file handlers here too.

Correct. I limited the scope to just the bug-at-hand when I noticed how convoluted the call paths are for getting app names and launch instructions from extensions and mime types :S

I have filed bug 1263176 for the follow-up. Maybe I'll be brave enough a little later to look at it.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97b3ac8e4c24

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/97b3ac8e4c24

Comment 19

[Ionuț Goldan [:igoldan]]

I think we need to revisit the implementation details from this bug. Currently, they are causing some serious intermittents, according to bug 1548630, as :aklotz points out in this comment.

Comment 20

[Chris H-C :chutten|PTO (back March 17)]

How can I help? I'm working on very different things now compared to 3 years ago, so I might not be the best person to tackle it (or have room in my schedule), but I'm willing to try.

Comment 21

[Ionuț Goldan [:igoldan]]

Removing ni?, as this got addressed in bug 1548630.