1815613 - Filename name suggested for downloaded file accepted as it is, then doesn't show up in download manager

Status: RESOLVED FIXED

(Fenix :: Downloads, defect, P3)

Description

[gabri.ns]

Attachment: Screenshot of download confirmation.

Steps to reproduce:

1. Open https://en.savefrom.net/
2. Input https://youtube.com/watch?v=eMn_77h2_Kk
3. Continue from browser
4. Click download button

Actual results:

Download confirmation show up with name containing unsafe character as shown in attachment. After download complete, it doesn't show up in download manager and the file cannot be opened nor be found anywhere as it is containing invalid file name.

Expected results:

Download confirmation already change the filename to safe name and after download completion file exist in filesystem.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:cpeterson, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[jackyzy823]

Response's header
Content-Disposition: attachment; filename="HONDA ODYSSEY 2000 A/T | REVIEW & TEST DRIVE | UDAH MURAH SEHARGA GENIO.mp4"

the unsafe char is / (which indicates a folder in Linux based OS)

The file download successfully in Download folder , However chars before / are ignored in the filename resulting T | REVIEW & TEST DRIVE | UDAH MURAH SEHARGA GENIO.mp4

I thinks it is websites' duty to make filename not contains unsafe chars.

In desktop firefox, the "A/T" is translated to "A_T" and "|" is striped , I think Fenix should do this too.

In Edge (Android) , "/" and "|" are translated to "_".

Comment 3

[Titouan Thibaud [:titouan]]

Attachment: HONDA ODYSSEY 2000 A:T | REVIEW & TEST DRIVE | UDAH MURAH SEHARGA GENIO.txt

Attempt to create a downloadable file that can reproduce this bug

Comment 4

[Titouan Thibaud [:titouan]]

Hey gabri.ns,
Can you still reproduce the bug? 

It seems like the website provided in the STR (https://en.savefrom.net/) has changed the way it names the downloadable files, I could only have videoplayback.webm for example.

I tried with the file I attached on this bug, hosted from my computer, and it was correctly renamed as described on Comment 2.

QA, can you give it a try to confirm we can close this bug?

Comment 5

[Titouan Thibaud [:titouan]]

Actually my test might not have been enough, as I only downloaded the file with a name containing |, but maybe we need to try specifically with what jackyzy823 suggested:

Response's header
Content-Disposition: attachment; filename="HONDA ODYSSEY 2000 A/T | REVIEW & TEST DRIVE | UDAH MURAH SEHARGA GENIO.mp4"

Comment 6

[Lorand Janos]

This issue is still reproducible as described.

• The unsafe characters are still displayed as shown in the original attachment.
• After download complete, it doesn't show up in download manager and the file cannot be opened nor be found anywhere as it is containing invalid file name is also still the case.

Tested on the latest Nightly build (128.0a1 from 2024-05-23).
Tested device: Samsung Galaxy S23 Ultra (Android 14).

Comment 7

[Titouan Thibaud [:titouan]]

Thanks for testing :Lorand Janos!

Can you please provide a description of how you tested, so that I can try to reproduce and fix it?

Comment 8

[Lorand Janos]

Hi, sure,

1. I accessed the mentioned page to download videos: https://en.savefrom.net/
2. I used the URL of the video given in the second step which is still active: https://youtube.com/watch?v=eMn_77h2_Kk
3. After the video was loaded, I tapped on the Download button below which opened the download popup (with the mentioned "unsafe characters") .
4. After tapping on download, while it shows that the download is completed, tapping on "Open" gets a "Failed to play video" error message.
5. Searching for the downloaded video in the downloads returns no result.

Comment 9

[Lorand Janos]

Attachment: ImportedPhoto_1716445812267.jpg

Comment 10

[Nicholas Poon [:Nick]]

Notes after pairing with Titouan

• After following the instructions to reproduce the bug, we realize that the video gets saved to the downloads directory as "T _ REVIEW & TEST DRIVE _ UDAH MURAH SEHARGA GENIO.mp4". That is, besides the name getting concatenated, the "|" is replaced with "_". Upon opening the video, it plays successfully. However, the file does not appear in the fenix downloads directory. Furthermore, when fenix prompts to open the file, it results in an error.
• After further investigation, we realized that the illegal characters for filenames on Android are at least these characters: /, <, >, *, ", :, ?, , |. If we edit String.replaceEscapedCharacters in String.kt (link) to also replace "|" with some legal character, such as "", the feature works mostly as expected. That is, the file name replaces the "|" with "" and the file appears in fenix's download directory where the video can be played. Also, after the download completes, fenix opens a prompt to open (and play) the file which succeeds, and the file appears in the downloads directory where it can be played successfully. The one expected behaviour that is missing is that the name remains the same but the illegal characters are substituted with legal characters.
• We believe that the reason for the concatenation behaviour in the name lies in extractFileNameFromUrl (link) and String.sanitizeFileName (link).

Follow Up Actions

• Update String.replaceEscapedCharacters to also replace / substitute other illegal characters in Android filenames
• Update the tests in StringTest.kt (link) to take the illegal characters into account

Comment 11

[Nicholas Poon [:Nick]]

(In reply to Nicholas Poon [:Nick] from comment #10)

Notes after pairing with Titouan

• After following the instructions to reproduce the bug, we realize that the video gets saved to the downloads directory as "T _ REVIEW & TEST DRIVE _ UDAH MURAH SEHARGA GENIO.mp4". That is, besides the name getting concatenated, the "|" is replaced with "_". Upon opening the video, it plays successfully. However, the file does not appear in the fenix downloads directory. Furthermore, when fenix prompts to open the file, it results in an error.
• After further investigation, we realized that the illegal characters for filenames on Android are at least these characters: /, <, >, *, ", :, ?, \, |. If we edit String.replaceEscapedCharacters in String.kt (link) to also replace "|" with some legal character, such as "", the feature works mostly as expected. That is, the file name replaces the "|" with "" and the file appears in fenix's download directory where the video can be played. Also, after the download completes, fenix opens a prompt to open (and play) the file which succeeds, and the file appears in the downloads directory where it can be played successfully. The one expected behaviour that is missing is that the name remains the same but the illegal characters are substituted with legal characters.
• We believe that the reason for the concatenation behaviour in the name lies in extractFileNameFromUrl (link) and String.sanitizeFileName (link).

Follow Up Actions

• Update String.replaceEscapedCharacters to also replace / substitute other illegal characters in Android filenames
• Update the tests in StringTest.kt (link) to take the illegal characters into account

Comment 12

[Nicholas Poon [:Nick]]

Attachment: Bug 1815613 - When downloading, replace illegal filename characters (<, >, *, ", :, ?, \, |) with _

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3be3c6c0ef5e
When downloading, replace illegal filename characters (<, >, *, ", :, ?, \, |) with _ r=android-reviewers,tthibaud

Comment 14

[Nicholas Poon [:Nick]]

Follow up bug

Comment 15

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/3be3c6c0ef5e

Comment 16

[Daniel Veditz [:dveditz]]

The fix in this bug builds on the fix for the bug 1873528, although that one was filed much later.

Comment 17

[Nicholas Poon [:Nick]]

Attachment: Bug 1815613 - Updating KDoc for String.replaceEscapedCharacters

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/953fd3b8071d
Updating KDoc for String.replaceEscapedCharacters r=android-reviewers,calu

Comment 19

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/953fd3b8071d

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)