Closed Bug 1452819 Opened 2 years ago Closed 2 years ago

Save Image without file extension does not use ContentType

Categories

(Firefox :: Menus, defect)

59 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 61
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- verified
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- verified

People

(Reporter: choladay, Assigned: bzbarsky)

References

(Blocks 1 open bug, )

Details

(Keywords: regression, testcase)

Attachments

(2 files)

Attached file 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
In my specific case the png headers are sending back `Content-Type: image/png; charset=utf-8`
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.
Blocks: 1406253
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → DOM
Ever confirmed: true
Flags: needinfo?(dao+bmo)
Flags: needinfo?(bzbarsky)
Keywords: regression, testcase
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → All
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?
Blocks: 1182654
Flags: needinfo?(dao+bmo)
See Also: → 1180126
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...
Flags: needinfo?(bzbarsky)
Totally a frontend issue...
Component: DOM → Menus
Product: Core → Firefox
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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.
Attachment #8966829 - Flags: review?(dao+bmo) → review+
Pushed by bzbarsky@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/00be17b4d1e3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Thanks everyone for your help.
Looks like this missed the boat for possible Fx60 consideration. Not sure if it's worth considering for ESR60, though?
Flags: qe-verify+
Flags: needinfo?(bzbarsky)
Probably worth it, yes...  It's an annoying functionality regression with a simple fix.
Flags: needinfo?(bzbarsky)
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!
Flags: needinfo?(choladay)
> 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)....
Appears fixed using Nightly 61.0a1 (2018-05-02) (64-bit)

Thank you!
Flags: needinfo?(choladay)
Based on comment 15, this issue is verified.
Status: RESOLVED → VERIFIED
Do you want to request uplift for 60.1esr now?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Attachment #8966829 - Flags: approval-mozilla-esr60?
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
Attachment #8966829 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
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 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
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.
You need to log in before you can comment on or make changes to this bug.