Closed Bug 1260483 Opened 3 years ago Closed 3 years ago

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

Categories

(Core Graveyard :: File Handling, defect)

45 Branch
defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: firefox.bug.filer, Assigned: chutten, NeedInfo)

References

Details

Attachments

(2 files)

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.
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.
Component: Untriaged → File Handling
Product: Firefox → Core
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)
Flags: needinfo?(ted)
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.
Flags: needinfo?(ted)
Gijs, is this bug about file handling in your scope?
Flags: needinfo?(gijskruitbosch+bugs)
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.
Blocks: 772711
Flags: needinfo?(jmathies)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(chutten)
See Also: → 772863
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 :)
Assignee: nobody → chutten
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(chutten)
(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.
Flags: needinfo?(jmathies)
Note bug 772863 was filed against metrofx, so I've closed it out. Anything we do should happen here.
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/
Attachment #8737862 - Flags: review?(jmathies)
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
Attachment #8737862 - Flags: review?(jmathies)
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.
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
Flags: needinfo?(jmathies)
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.
Attachment #8737862 - Flags: review+
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.
Flags: needinfo?(jmathies)
(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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/97b3ac8e4c24
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Core → Core Graveyard
Blocks: 1199678

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.

Flags: needinfo?(chutten)

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.

Flags: needinfo?(chutten) → needinfo?(igoldan)
You need to log in before you can comment on or make changes to this bug.