1452819 - Save Image without file extension does not use ContentType

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[choladay]

Attachment: ff_save_image.swf

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36

Steps to reproduce:

When viewing an image via a URL without a file extension, the "Save Image As..." right-click option defaults to `*.htm *.html` format instead of the ContentType provided.


Actual results:

1) View a png image with a URL that does not have a file extension
2) Right-click and use "Save Image As..."
3) Browser opens save dialogue with `*.htm *.html` file extension


Expected results:

1) View a png image with a URL that does not have a file extension
2) Right-click and use "Save Image As..."
3) Browser opens save dialogue with `.png` file extension

Comment 1

[choladay]

In my specific case the png headers are sending back `Content-Type: image/png; charset=utf-8`

Comment 2

[choladay]

https://www.screencast.com/t/OXwSrAiIUx4

Comment 3

[Gingerbread Man]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
20180409220118

https://hg.mozilla.org/integration/autoland/json-pushes?changeset=8f9e9de253230bf864198fd7e56b637d66578f31&full=1

Patch author is unavailable, NI reviewers.

Bug 1180126 fixed this, which was at the time a regression as well.
Bug 1406253 regressed this.
Bug 1182654 proposes a test for this, but hasn't seen any progress in 3 years.

Comment 4

[Dão Gottwald [:dao]]

Hmm, I'm guessing nsIImageLoadingContent.currentRequestFinalURI takes care of redirects but not content headers. Assuming this in itself isn't a bug: What's the right API to use if we want to take into account both redirects and content headers?

Comment 5

[Boris Zbarsky [:bzbarsky]]

So..  we shouldn't be messing up the extension even if we ignore the Content-Disposition header, just based on the Content-Type.

But the real problem, I expect, is this change from bug 1406253:

  let props = imageCache.findEntryProperties(aEvent.target.currentRequestFinalURI, doc);

That's wrong; the image cache is caching this thing under aEvent.target.currentURI.  And that "props" is where we get our type and filename data.

We should clearly have tests for the type being respected in this case too, not just the content-disposition.  And note that this only broke for images that involve a redirect, not in general.  So even if we had a test per bug 1182654 it might not have caught this...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Totally a frontend issue...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Use the right URI when getting data from the image cache for the "Save Image As..." context menu options

MozReview-Commit-ID: zZggyJ6y7n

Comment 8

[Boris Zbarsky [:bzbarsky]]

I have no idea how to write a sane test for this, but the patch fixes https://p14.zdusercontent.com/attachment/2021495/tbEB0EREUN1g3gsTAUSNTQJxy for me when I test manually.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00be17b4d1e3
Use the right URI when getting data from the image cache for the "Save Image As..." context menu options.  r=dao

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/00be17b4d1e3

Comment 11

[choladay]

Thanks everyone for your help.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Looks like this missed the boat for possible Fx60 consideration. Not sure if it's worth considering for ESR60, though?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Probably worth it, yes...  It's an annoying functionality regression with a simple fix.

Comment 14

[Daniel Bodea [:danibodea]]

Hi choladay,

I've tried to reproduce this issue on an older version before I validate the fix, but I could not reproduce it at all. Can you confirm that the issue is fixed with the latest nightly build? Please download Firefox Nightly from here: https://nightly.mozilla.org/ and retest the problem.

Thank you!

Comment 15

[Boris Zbarsky [:bzbarsky]]

> but I could not reproduce it at all.

Looks like your main problem is that the testcase linked above went away (now shows totally different content)....

Comment 16

[choladay]

Appears fixed using Nightly 61.0a1 (2018-05-02) (64-bit)

Thank you!

Comment 17

[Daniel Bodea [:danibodea]]

Based on comment 15, this issue is verified.

Comment 18

[Julien Cristau [:jcristau]]

Do you want to request uplift for 60.1esr now?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8966829 [details] [diff] [review]
Use the right URI when getting data from the image cache for the "Save Image As..." context menu options

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: User-visible functionality regression
User impact if declined: When saving images, they end up with the wrong filename.
Fix Landed on Version: 61
Risk to taking this patch (and alternatives if risky): Pretty low-risk; just reverts to the 58 behavior.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 20

[Julien Cristau [:jcristau]]

Comment on attachment 8966829 [details] [diff] [review]
Use the right URI when getting data from the image cache for the "Save Image As..." context menu options

fix filename when saving images, verified in beta, approved for 60.1esr

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/094590abbf52

Comment 22

[Cristian Baica [:cbaica], Release Desktop QA]

Looks like the fix did not make it in time for the 60.1ESR version. Looks like the push was made after the esr launch (I can still reproduce the issue on the ESR build).

Ryan, if the fix needs an earlier check, could you please provide a temporary ESR build containing this fix?

Comment 23

[Julien Cristau [:jcristau]]

Comment 22 sounds confused.  60.1esr doesn't ship until next month.

You can download CI builds from https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr60

Comment 24

[Cristian Baica [:cbaica], Release Desktop QA]

Indeed I was a bit confused. Nonetheless, I reproduced the issue in Fx ESR 60.0.1.

I have verified the fix using the CI build from the provided link on Win 10 x64 and Ubuntu 16.04 x64. I can confirm that the issue is fixed for Fx 60.0.2 ESR (buildID: 20180527170520).

Thank you Julien.