789550 - data:image/gif URI `Save Page As` and `Save Image As` do not use the same filename

Status: VERIFIED FIXED

(Firefox :: File Handling, enhancement, P3)

Description

[Chris Peterson [:cpeterson]]

This bug was forked from bug 254139 comment 62.

STR:
1. View a data:image URI

2. Selecting `File menu > Save Page As` will save a file called something like "(GIF Image, 16 × 14 pixels).gif" (named after the document's title).

3. But right-clicking on the image and selecting `Save Image As` will save a file called simply "index.gif" (because the image does not have its own name or title).

In contrast, Chrome will save both files as "download.gif" and Safari will save them as "Unknown.gif".

ER:
`Save Page As` and `Save Image As` should use the same filename when you are viewing an image directly (even if it is a data:image/gif).

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is all Firefox code, not core code.

Comment 2

[Raymond Lee [:raymondlee]]

When we try to save an image, it would call saveImageURL/saveDocument => internalSave => initFileInfo => getDefaultFilename (http://mxr.mozilla.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#822). For data:image, when saving a document (File -> save page as), it would extract the file name from the document'stitle.  However, when saving an image via content menu, the document is not passed into that method so it returns a default name "index".

It seems logical to me.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

We could special case images that are in image documents and have them use the document filename.

Comment 4

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 5

[Raymond Lee [:raymondlee]]

Comment on attachment 664846 [details] [diff] [review]
v1

Didn't pass try, fixing it

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: v2

Pushed to try and waiting for results.
https://tbpl.mozilla.org/?tree=Try&rev=e43d82d32185

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 665044 [details] [diff] [review]
v2

Seems to me like it would be better to fix this in getDefaultFileName, assuming that's possible (it gets passed the document being saved, so looks like it should be).

Comment 8

[Raymond Lee [:raymondlee]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #7)
> Comment on attachment 665044 [details] [diff] [review]
> v2
> 
> Seems to me like it would be better to fix this in getDefaultFileName,
> assuming that's possible (it gets passed the document being saved, so looks
> like it should be).

if we pass a document to getDefaultFileName(), it would think that we are trying to save a document and returns the document.title.  However, there is a case which we want to save a normal image in a document using the "Save Image As" context menu item (the same method call as saving data:image: saveImageURL => internalSave => initFileInfo => getDefaultFilename); if we pass in the document to getDefaultFileName, it would return the document.title instead of the actual image title.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

How about making getDefaultFilename not use document.title for image documents?

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: v3

OK, when using `Save Page As` and `Save Image As`for data:image, it would use index.*

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 669178 [details] [diff] [review]
v3

Looks reasonable to me, but I wonder whether we want to check aDocument.mozSyntheticDocument instead (to also catch e.g. standalone video documents). Chris has looked at this code recently, let's see what he has to say!

Comment 12

[Chris Peterson [:cpeterson]]

Comment on attachment 669178 [details] [diff] [review]
v3

Review of attachment 669178 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. I agree with Gavin's comment that other document types (such as video) might not be covered, but your patch would not make those rare cases any worse. :)

Comment 13

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Passed Try
https://tbpl.mozilla.org/?tree=Try&rev=9d4dfebecae1

Comment 14

[:Gavin Sharp [email: [e-mail]]]

(In reply to Chris Peterson (:cpeterson) from comment #12)
> LGTM. I agree with Gavin's comment that other document types (such as video)
> might not be covered, but your patch would not make those rare cases any
> worse. :)

It's easy enough to fix that by just switching to checking mozSyntheticDocument, as I suggested. Is that the behavior that we want, for audio and video?

Comment 15

[Raymond Lee [:raymondlee]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #14)
> (In reply to Chris Peterson (:cpeterson) from comment #12)
> > LGTM. I agree with Gavin's comment that other document types (such as video)
> > might not be covered, but your patch would not make those rare cases any
> > worse. :)
> 
> It's easy enough to fix that by just switching to checking
> mozSyntheticDocument, as I suggested. Is that the behavior that we want, for
> audio and video?

I think the issue of this bug is that data:image doesn't have a filename so the 'save image as' context menu item uses the default string "index" and the 'save page as' document.title.  For audios and videos,  I haven't seen data:mp3, data:mp4 (I am not sure whether it's possible or not) so those files would have filenames and I believe it should be handled correctly.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

This patch doesn't only affect the behavior for data: URIs, as I understand it. If you're viewing a standalone video, using the filename instead of the title makes more sense in theory (though at the moment, AFAICT the document title *is* the filename, so this doesn't matter in practice).

So let's just change the check to use mozSyntheticDocument.

Comment 17

[Raymond Lee [:raymondlee]]

Attachment: v4

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 669998 [details] [diff] [review]
v4

Are there any tests for this functionality? If not there probably should be...

Comment 19

[Chris Peterson [:cpeterson]]

Here are some data: URI tests, including a data:mp3 file. Unfortunately, data:mp3 is handled by the QuickTime plugin, so I can't compare "Save Page As" versus right-click "Save As".

http://www-archive.mozilla.org/quality/networking/testing/datatests.html

Comment 20

[Ava Katushka]

Hello, I'm Ava, an outreachy intern, and I would like to take this bug to work on.

Comment 21

[Ava Katushka]

I have mozilla local build set up, but I'm not sure I can reproduce the issue.

1. I use photo of the dog: https://pocket-syndicated-images.s3.amazonaws.com/60639cdae32bd.jpg for the testing purposes.
2. When I save it through right-click + "Save image as..." file is saved as "60639cdae32bd.jpg".
3. Also when I click File > Save page as... file is saved as "60639cdae32bd.jpg".
4. I also have the same names through bough paths while attempting to save an audio: https://www.mediacollege.com/audio/_assets/white-noise.mp3. In both cases, the file name is "white-noise.mp3".

ER:
Save Page As and Save Image As should use the same filename when you are viewing an image directly (even if it is a data:image/gif).

The names are identical, so is this bug still actionable? Please tell me, if I'm missing something.

Comment 22

[Chris Peterson [:cpeterson]]

Hi, Ava, thanks for taking on this bug! I'll assign this bug report to you.

I see that my bug summary does not match my steps to reproduce in comment 0. I'll fix that. This bug is specifically about a "data:image" URI (where the image data is actually encoded in the URI itself as Base64 text), not a URL pointing to an image file like https://pocket-syndicated-images.s3.amazonaws.com/60639cdae32bd.jpg.

Here is the "data:image" URI that I originally tested with. Just copy this URI, paste it into the address bar, and press Enter to load:

data:image/gif;base64,R0lGODlhEAAOALMAAOazToeHh0tLS/7LZv/0jvb29t/f3//Ub//ge8WSLf/rhf/3kdbW1mxsbP//mf///yH5BAAAAAAALAAAAAAQAA4AAARe8L1Ekyky67QZ1hLnjM5UUde0ECwLJoExKcppV0aCcGCmTIHEIUEqjgaORCMxIC6e0CcguWw6aFjsVMkkIr7g77ZKPJjPZqIyd7sJAgVGoEGv2xsBxqNgYPj/gAwXEQA7

I can still reproduce this bug in Firefox 89 Nightly:

• File menu > "Save Page As" defaults to filename (GIF Image, 16 × 14 pixels).gif.
• Right-click on the tiny (folder) icon > "Save Page As" defaults to filename index.gif. (Right-clicking on the page's black background and selecting "Save Page As" defaults to filename (GIF Image, 16 × 14 pixels).gif.

Open question: while we agree that the saved file should probably have the same filename, regardless of how it was saved, what do we actually want that filename to be? (GIF Image, 16 × 14 pixels).gif, index.gif, or something else?

Precedents from other browsers for comparison:

• Chrome (and Microsoft's Chromium Edge) on Windows and macOS default to filename download.gif.
• Safari on macOS defaults to filename Unknown.gif.

As a user, if I see a data:image URI in the address bar and save it to a file, I would probably expect a filename like data.gif or image.gif. However, copying the precedent set by Chrome (download.gif) is always a safe choice. The engineer who reviews your code will probably have their own opinion. :) (We can't use a filename like data:image.gif because colon characters are not allowed in filenames on Windows or macOS.)

This bug report has an attachment with ancient code patch that apparently fixed this bug (nine years ago). I suggest reading that patch to get an idea of where to begin fixing this bug now. But beware that the Firefox code has changed a lot in the last nine years, so that patch's code changes might not be the fix needed today.

Comment 23

[Ava Katushka]

Attachment: Bug 789550 - Avoid using document title while saving data URIs. r?mccr8

Comment 24

[Ava Katushka]

Thank you so much Chris for giving the detailed explanations on how to reproduce the bug and also guidance for my first bug as I'm trying to contribute. That was super, super helpful!!

Previous patches

I've tested the previous patches to this bug,

• !(aDocument instanceof Ci.nsIImageDocument) [669178]
• !aDocument.mozSyntheticDocument [669998]
  None of them seems to work now.

Idea & Implementation

My idea is to check if it's a data URI of image/audio/video and return some standart response in this case.

Current implementation: https://phabricator.services.mozilla.com/D111857

I tested several cases with data URIs.

Test sketch

Image (Works with patch)

Currently: image data file, if saved through "Save page as", saves it as title, e.g "(GIF Image ..).gif" and when saved through "Save image as" saves as "index.gif"

Should be: saved in the same way through "Save page as" and "Save audio as" as "index.gif"

Audio (Doesn't work)

Currently: audio data mp3 file, if saved through "Save page as" is saved as "index.mp3" and if saved as "Save audio as" saves as "mp3.mp3".

Should be saved in the same way through "Save page as" and "Save audio as" as "index.mp3"

Problem

I figured out that while audio is saved, it doesn't use the same functions the images do. So it doesn't use the result of "getDefaultFileName" function or any functions two levels up. There it goes, I didn't still understand.

I hope my patch for images at least makes sense, and I'll wait the reviewer's feedback on it.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/714e6e7b8eef
Avoid using document title while saving data URIs. r=Gijs

Comment 26

[Noemi Erli[:noemi_erli]]

Backed out changeset 714e6e7b8eef (Bug 789550) for causing failures in browser_default_audio_filename.js CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=714e6e7b8eefb2d8965952416d95085b859d9d5f&selectedTaskRun=JwzWDxT5TzGnM1Ae3vyj6A.0

Log: https://treeherder.mozilla.org/logviewer?job_id=337218854&repo=autoland&lineNumber=3258

Backout: https://hg.mozilla.org/integration/autoland/rev/65ae09788b53e3e06a831ebea3fa74c51d398394

Comment 27

[Chris Peterson [:cpeterson]]

Ava, looks like your patch was backed out because your browser_default_audio_filename.js test is failing:

TEST-UNEXPECTED-FAIL | toolkit/content/tests/browser/browser_default_audio_filename.js | undefined assertion name - Got "index", expected "index.mp3"

You will need to fix your patch or the test and submit the new fix to Phabricator for a second review.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16b7ab3bd643
Avoid using document title while saving data URIs. r=Gijs

Comment 29

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/16b7ab3bd643

Comment 30

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial issue using old Nightly from 2021-04-07, verified that saving the image and audio file from both page context menu or Main Menu will have the same name proposed to save. Verified on 90.0b5 across platforms (Windows 10, macOS 11.3 and Ubuntu 18.04).