Closed Bug 1597963 Opened 4 years ago Closed 4 years ago

The file does not have an app associated with it for performing this action error when opening .ttf extensions through download panel

Categories

(Firefox :: File Handling, defect)

Desktop
Windows 10
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 72
Tracking Status
firefox-esr68 72+ verified
firefox70 --- unaffected
firefox71 --- verified
firefox72 --- verified

People

(Reporter: atrif, Assigned: toshi)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Attached image .ttf_extension.gif

Affected versions

  • 71.0b11 (20191118154140)
  • 72.0a1 (20191120094758)

Affected platforms

  • Windows10 x64

Steps to reproduce

  1. Launch Firefox and download https://bugzilla.mozilla.org/attachment.cgi?id=9087648.
  2. Open the downloaded item through the download panel.

Expected result
The font installer is successfully opened.

Actual result
The file does not have an app associated with it for performing this action error is shown.

Regression Range

Notes

  • Attached a screen recording with the issue.

Toshihito Kikuchi can you please have a look? Thank you!

Has Regression Range: --- → yes
Has STR: --- → yes
Flags: needinfo?(tkikuchi)

This is because the current code forces "open" verb and .ttf has no "open" verb. We should specify null verb. (although I don't know how can _variant_t represent null.)

(In reply to Masatoshi Kimura [:emk] from comment #2)

This is because the current code forces "open" verb and .ttf has no "open" verb. We should specify null verb. (although I don't know how can _variant_t represent null.)

Thank you for great suggestion! I verified the repro and a null verb (an empty _variant_t) worked nicely for .ttf file.

Flags: needinfo?(tkikuchi)
Assignee: nobody → tkikuchi

Looks like the default verb for a font file is "preview". So obviously passing "open" is bad.

I remember why I specified the open verb in that patch. With null verb for an unassociated file, ShellExecute shows the modeless dialog saying "How do you want to open this file?", while IShellDispatch2.ShellExecute shows the error dialog saying "This file does not have an app associated with...". I added the open verb in order to keep the former behavior. Anyway, let me find out what is our best option.

The patch for Bug 1588975 specified the "open" verb to execute a target, but
the default verb is not always "open". For example, the default verb for a font
file is "preview". We should specify null verb to start the default operation.

Now we use IShellDispatch2.ShellExecute to ask explorer.exe to call
ShellExecuteExW. That method takes an optional VARIANT parameter as a verb.
According to https://devblogs.microsoft.com/oldnewthing/20140919-00/?p=44023,
we need to pass VT_ERROR to omit an optional parameter. If we pass
other values such as nullptr with VT_BSTR or VT_EMPTY, explorer.exe calls
ShellExecuteExW with the empty string "" instead of nullptr, which is not
considered as a valid verb if the target file is not associated with any app.

We have shipped our last beta for 71, since there is a patch in the bug, I am setting the status as fix-optional for 71 in case there is a safe uplift possible in a potential dot release as a ride-along.

Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d9e4a5bff682
Pass VT_ERROR for Explorer to call ShellExecuteExW with null verb. r=aklotz
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 72

Verified the issue using Firefox 72.0a1 (20191125095200) on Windows 10x64. The .ttf file is correctly opened via downloads panel.

Status: RESOLVED → VERIFIED

Would it be safe to uplift this to 71 still?

Flags: needinfo?(tkikuchi)

(In reply to :Gijs (he/him) from comment #10)

Would it be safe to uplift this to 71 still?

Yes, the patch is to change one argument from "open" to null. Uplift to 71 and esr68 should be safe.

Flags: needinfo?(tkikuchi)

Please request uplift to mozilla-release now or it will ride the trains to 72, we start the RC builds in a few hours only. Thanks

Flags: needinfo?(tkikuchi)

Comment on attachment 9110372 [details]
Bug 1597963 - Pass VT_ERROR for Explorer to call ShellExecuteExW with null verb. r=aklotz

Beta/Release Uplift Approval Request

  • User impact if declined: This is a regression from Bug 1588975, the launching issue of Skype for Business. If a user opens a downloaded file on Windows and the default action associated with the file is not "open", such as a font file, the user will see an error dialog because Firefox always tries to "open" the file.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The patch is just to change an argument from "open" to null verb to let shell use the default verb to execute a file.
  • String changes made/needed: None
Flags: needinfo?(tkikuchi)
Attachment #9110372 - Flags: approval-mozilla-release?

Comment on attachment 9110372 [details]
Bug 1597963 - Pass VT_ERROR for Explorer to call ShellExecuteExW with null verb. r=aklotz

Low risk patch for a visible issue with Skype for Business, fix verified by QA, let's take it into our RC build, thanks.

Attachment #9110372 - Flags: approval-mozilla-release? → approval-mozilla-release+

Tried to uplift to mozilla-release, but I have a conflict:

hg graft -er d9e4a5bff682
grafting 579298:d9e4a5bff682 "Bug 1597963 - Pass VT_ERROR for Explorer to call ShellExecuteExW with null verb. r=aklotz"
merging xpcom/io/nsLocalFileWin.cpp
warning: conflicts while merging xpcom/io/nsLocalFileWin.cpp! (edit, then use 'hg resolve --mark')

:toshi, can you please take a look?

Flags: needinfo?(tkikuchi)

The patch for Bug 1588975 specified the "open" verb to execute a target, but
the default verb is not always "open". For example, the default verb for a font
file is "preview". We should specify null verb to start the default operation.

Now we use IShellDispatch2.ShellExecute to ask explorer.exe to call
ShellExecuteExW. That method takes an optional VARIANT parameter as a verb.
According to https://devblogs.microsoft.com/oldnewthing/20140919-00/?p=44023,
we need to pass VT_ERROR to omit an optional parameter. If we pass
other values such as nullptr with VT_BSTR or VT_EMPTY, explorer.exe calls
ShellExecuteExW with the empty string "" instead of nullptr, which is not
considered as a valid verb if the target file is not associated with any app.

I uploaded a new patch for mozilla-release. The change is the exact same, but the downstream code was a bit different because mozilla-release does not have a fix for Bug 1597794.

Flags: needinfo?(tkikuchi)
Flags: qe-verify+
Flags: needinfo?(alexandru.trif)
QA Whiteboard: [qa-triaged]

Verified with 71.0 (20191125204040) on Windows 10x64 and the file from the comment 0 is successfully opened.

Flags: qe-verify+
Flags: needinfo?(alexandru.trif)
Attachment #9111366 - Attachment is obsolete: true

Does this need an ESR68 approval request still?

Flags: needinfo?(tkikuchi)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)

Does this need an ESR68 approval request still?

Yes, I'll add a request.

Flags: needinfo?(tkikuchi)

Comment on attachment 9110372 [details]
Bug 1597963 - Pass VT_ERROR for Explorer to call ShellExecuteExW with null verb. r=aklotz

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a regression from Bug 1588975, the launching issue of Skype for Business, which was uplifted for ESR. If a user often downloads a file-type which causes this issue, their daily operation would be disturbed.
  • User impact if declined: If a user opens a downloaded file on Windows and the default action associated with the file is not "open", such as a font file, the user will see an error dialog but the file should be able to be opened with the default action.
  • Fix Landed on Version: 72.0a1, 71.0
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The patch is just to change an argument from "open" to null verb to let shell use the default verb to execute a file.
  • String or UUID changes made by this patch:
Attachment #9110372 - Flags: approval-mozilla-esr68?

Comment on attachment 9110372 [details]
Bug 1597963 - Pass VT_ERROR for Explorer to call ShellExecuteExW with null verb. r=aklotz

regression fix, approved for 68.4esr

Attachment #9110372 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

Verified with Firefox 68.4.0 (20191209231253) from comment 24 on Windows 10x64. The .ttf file is successfully opened from the download manager.

You need to log in before you can comment on or make changes to this bug.