1211243 - Windows folder is broken by running save all media in the Page Info if the media contain an blob/data URL

Status: VERIFIED FIXED

(Firefox :: Page Info Window, defect)

Description

[magicp]

Attachment: broken-windows-downloads-folder.png

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20151003030225

Steps to reproduce:

1. Run Firefox 41.0 on Windows7 to 10 (ja-JP)
2. Open "about:home"
3. View Page Info
4. Select "Media" tab
5. Click "Select All" button
6. Click "Save As" button
7. Select download folder (e.g. C:\Users\magicp\Downloads)


Actual results:

After save as (download), Download folder name is changed to "Downloads" from "ダウンロード", and also folder icon is changed to the standard one.

How to restore folder name and icon:
1. Push Win+R key
2. Run "regsvr32 /i:U shell32.dll" in command-line
3. Logout and Login Windows


Expected results:

Download folder should not be broken by save as.

Firefox 42.0 beta and later version has other issue.
See Also: Bug 1207168

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=82546b96471c&tochange=3de88d67b2de

Regressed by: 	66b33f3f3594	Felix H. Dahlke — Bug 921229 - Remove files with the read-only flag set. r=yoric

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Regression introduced in 28, not tracking...

Comment 3

[David Teller [:Yoric] - still alive but not very active]

I don't see a clear causality relationship between the patch and the issue. I suspect that the patch fixed a bug that some other feature depended on.

Comment 4

[magicp]

How about next step?

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Florian, do you have any idea how Page Info works? Where would it use OS.File?

Comment 6

[Florian Quèze [:florian]]

I see some use of OS.File in promiseTargetFile (http://mxr.mozilla.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#620) called from internalSave (http://mxr.mozilla.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#329) which is called by Page Info in saveMedia when multiple images are going to be saved at once (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/pageinfo/pageInfo.js#746). Note: the setTimeout in that function smells like a hack that remains from before saving files was async; it may be causing race conditions.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

(gosh, that code is ugly – mixing OS.File and FileUtils? not a good idea)
This doesn't seem to be the culprit. The patch pointed in comment 1 only affects `OS.File.remove`.

Comment 8

[Florian Quèze [:florian]]

I don't see any obvious reason for remove() to be called while saving images.

Comment 9

[magicp]

How about next steps?

Comment 10

[David Teller [:Yoric] - still alive but not very active]

No idea, I'm afraid.

Comment 11

[magicp]

Long outstanding problem

Comment 12

[Alice0775 White]

Bug 921229 destroy localized windows file system!
Please backed out Bug 921229!

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Reverting that bug would most likely break many other things, I'm afraid.

Also, magicp, I'm trying to understand: was the downloads folder initially named "Downloads" (as in C:\Users\magicp\Downloads) or "ダウンロード"? Or is it that Windows has a weird convention in which the folder has different names in different views?

Comment 14

[David Teller [:Yoric] - still alive but not very active]

I really can't see any output performed with OS.File in that code, so I would be surprised if OS.File (including bug 921229) was involved. Alice0775, are you confident about your regression range?

Comment 15

[Alice0775 White]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #14)

> Alice0775, are you confident about your regression range?

100% sure.

Comment 16

[magicp]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #13)
> Reverting that bug would most likely break many other things, I'm afraid.
> 
> Also, magicp, I'm trying to understand: was the downloads folder initially
> named "Downloads" (as in C:\Users\magicp\Downloads) or "ダウンロード"? Or is it
> that Windows has a weird convention in which the folder has different names
> in different views?

File system folder name is "Downloads" and localized name is "ダウンロード". It is customized with desktop.ini(hidden file). Please find below link.

[Customizing Folders with Desktop.ini]
https://msdn.microsoft.com/en-us/library/aa969337.aspx

Comment 17

[David Teller [:Yoric] - still alive but not very active]

So what happened at low-level? Did Desktop.ini disappear? Was it modified by the call to Save All?

Comment 18

[Alice0775 White]

Not only "ダウンロード" folder, but also "ピクチャ". And maybe other localized folders will be affected.

STR 
7. Select download folder e.g. >デスクトップ >[user name] >ピクチャ

Actual Results:
>デスクトップ >PC >ローカルディスク(C:) >ユーザー >[user name] >Pictures

Expected Results:
>デスクトップ >PC >ローカルディスク(C:) >ユーザー >[user name] >ピクチャ

should not change folder name.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

So it's always the name of the directory to which the files were saved, right? No other directories are affected?

Comment 20

[Alice0775 White]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #19)
> So it's always the name of the directory to which the files were saved,
> right? No other directories are affected?

Not sure. But, hopefully not.
Anyway, the products(mozilla) using OS.File module cannot trusted.

Comment 21

[magicp]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #17)
> So what happened at low-level? Did Desktop.ini disappear? Was it modified by
> the call to Save All?

desktop.ini did not disappear and was not modified. But the localized folder name and icon are affected by the call to Save All.

My guesses are the following...
- (data:image/png;base64...) Data image affect download.
- Can reproduce select a data image and another image. (not save all)
- It has no file name for Save As, and also user cannot decide it at Save All. 
- User select a download folder at Save All. However Firefox tries to download the data image using download folder name as file name.(e.g. Downloads, Pictures...) This behavior has a possibility overwrite the download folder.
- Download error message was displayed in Firefox 25(and earlier?)
 "C:\users\<user name>\Downloads could not be saved, because you cannot change the contents of that the folder. Change the folder properties and try again, or try saving in a different location [OK]"
- Probably, Bug 921229 enabled the overwriting of folder unfortunately.
- In latest Nightly, Save All does not work with Bug 1207168.
- blob resource has other issue.

Comment 22

[David Teller [:Yoric] - still alive but not very active]

(In reply to magicp from comment #21)
> - Download error message was displayed in Firefox 25(and earlier?)
>  "C:\users\<user name>\Downloads could not be saved, because you cannot
> change the contents of that the folder. Change the folder properties and try
> again, or try saving in a different location [OK]"
> - Probably, Bug 921229 enabled the overwriting of folder unfortunately.

Interesting. The only codepath that was affected by Bug 921229 is just that, when we're trying to remove a file under Windows, we're trying harder. The only way I could see this affecting us is if some code accidentally attempts to remove the Downloads (or Pictures, etc.) directory.

Could you run the following experiment for me? You'll need to capture stderr on Windows, I'm afraid I don't know how to do that.

1. Start Firefox normally, wait a little to be sure that loading is complete.
2. In about:config, set preference "toolkit.osfile.log" to "true".
3. Reproduce the issue.
4. In about:config, set preference "toolkit.osfile.log" to "false".
5. Attach the stderr logs.

Comment 23

[magicp]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #22)
> (In reply to magicp from comment #21)
> > - Download error message was displayed in Firefox 25(and earlier?)
> >  "C:\users\<user name>\Downloads could not be saved, because you cannot
> > change the contents of that the folder. Change the folder properties and try
> > again, or try saving in a different location [OK]"
> > - Probably, Bug 921229 enabled the overwriting of folder unfortunately.
> 
> Interesting. The only codepath that was affected by Bug 921229 is just that,
> when we're trying to remove a file under Windows, we're trying harder. The
> only way I could see this affecting us is if some code accidentally attempts
> to remove the Downloads (or Pictures, etc.) directory.
> 
> Could you run the following experiment for me? You'll need to capture stderr
> on Windows, I'm afraid I don't know how to do that.
> 
> 1. Start Firefox normally, wait a little to be sure that loading is complete.
> 2. In about:config, set preference "toolkit.osfile.log" to "true".
> 3. Reproduce the issue.
> 4. In about:config, set preference "toolkit.osfile.log" to "false".
> 5. Attach the stderr logs.

I am trying to capture stderr. Where can I get the stderr logs? browser console? Windows event log?

Comment 24

[David Teller [:Yoric] - still alive but not very active]

I think you need to launch Firefox from the console as follows:
firefox.exe > firefox-log.txt

The log should be in firefox-log.txt.

Comment 25

[magicp]

Attachment: firefox-log.txt

Comment 26

[Masatoshi Kimura [:emk]]

> 2. Open "about:home"

Oh, I must have missed this in the STR. Now I could reproduce the download failure.
"about:home" uses a blob URL for an image. If I excluded the blob URL from selected media, I could not reproduce the download failure.

Comment 27

[magicp]

(In reply to Masatoshi Kimura [:emk] from comment #26)
> > 2. Open "about:home"
> 
> Oh, I must have missed this in the STR. Now I could reproduce the download
> failure.
> "about:home" uses a blob URL for an image. If I excluded the blob URL from
> selected media, I could not reproduce the download failure.

I can reproduce in Google home page. (e.g. https://www.google.co.jp/) It is not contained the blob URL. "about:home" was contained the data URL (not blob) when I filed this bug.

Comment 28

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5eed34a4dc9

Comment 29

[Masatoshi Kimura [:emk]]

(In reply to magicp from comment #27)
> I can reproduce in Google home page. (e.g. https://www.google.co.jp/) It is
> not contained the blob URL.

Year, it was reproducible with not only blob URL but also data URL. Google home page contains some data URLs.

> "about:home" was contained the data URL (not
> blob) when I filed this bug.

Looks like Nightly about:home contains a blob URL, but Release about:home contains a data URL.

Comment 30

[Masatoshi Kimura [:emk]]

I made another try build. Please test.

Win32:
https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-70ed1514a1204fd714234e6dda18ea43286b8c92/try-win32/firefox-46.0a1.en-US.win32.installer.exe
Win64:
https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-70ed1514a1204fd714234e6dda18ea43286b8c92/try-win64/firefox-46.0a1.en-US.win64.installer.exe

This patch will not fix the download failure itself for blob URLs. I think it is out of scope of this bug. But it will no longer break the selected folder. Downloads for data URLs will succeed now, but you will have to add an extension to the downloaded files manully (again, it is out of scope of this bug).

Comment 31

[magicp]

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

I have tested your another try builds. The results are as follows...

(Win32/64)
- The selected download folder is broken: Fixed

> This patch will not fix the download failure itself for blob URLs. I think
> it is out of scope of this bug. But it will no longer break the selected
> folder. Downloads for data URLs will succeed now, but you will have to add
> an extension to the downloaded files manully (again, it is out of scope of
> this bug).

The following results will be out of scope in this bug. Could you please tell me bug numbers or next steps?

(Win32/64)
- Download a blob URL (single select): Degrade. The blob url could be downloaded in the single-process window, but it is failed in the try builds.
- Download data URLs (select all): I can download the data URLs by save all. But file name is strange for me. (e.g. download3, download4...) We can download data URL include the file extension when using its url in urlbar. I think download should be same result even if doing other operations, also blob URL. Because the file extension has been defined before downloading.

Comment 32

[magicp]

(In reply to magicp from comment #31)
> (Win32/64)
> - Download a blob URL (single select): Degrade. The blob url could be
> downloaded in the single-process window, but it is failed in the try builds.
Sorry, I can download the blob url in the single-process window with try builds.

Comment 33

[Masatoshi Kimura [:emk]]

(In reply to magicp from comment #31)
> The following results will be out of scope in this bug. Could you please
> tell me bug numbers or next steps?

AFAIK the bug is not filed yet. Please file a follow-up bug and Cc me. But I can not promise to fix it promptly because the fix would not be trivial.

Comment 34

[Masatoshi Kimura [:emk]]

Attachment: Do not swallow exceptions if the URI is not an instance of a standard URL

Comment 35

[Florian Quèze [:florian]]

Comment on attachment 8709912 [details] [diff] [review]
Do not swallow exceptions if the URI is not an instance of a standard URL

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

Thanks for looking into this. I know this is intended mostly as a temporary workaround before a real fix that sets file extensions, but I think we can easily do a little better for non-English users.

::: browser/base/content/pageinfo/pageInfo.js
@@ +758,5 @@
>            var uriString = gImageView.data[v][COL_IMAGE_ADDRESS];
>            var uri = makeURI(uriString);
>  
>            try {
>              uri.QueryInterface(Components.interfaces.nsIURL);

I wonder if this try/catch should be replaced with:
if (uri instanceof Ci.nsIURL)
  dir.append(decodeURIComponent(uri.fileName));
else
  ...

It may be better to not change this now if we are hoping to uplift this bug's fix though.

@@ +764,5 @@
>            } catch(ex) {
> +            // data:/blob: uris
> +            // Supply a dummy filename, otherwise Download Manager
> +            // will try to delete the base directory on failure.
> +            dir.append("download" + i);

The hard coded English "download" string is unfortunate here. How about using gImageView.data[v][COL_IMAGE_TYPE] instead?

Is the '+ i' part really needed? I thought we had code automatically appending a number to the file name to avoid overwriting an existing file when downloading 2 files with the same name.

Comment 36

[magicp]

(In reply to Masatoshi Kimura [:emk] from comment #33)
> AFAIK the bug is not filed yet. Please file a follow-up bug and Cc me. But I
> can not promise to fix it promptly because the fix would not be trivial.

Hi, I have filed Bug 1241383.

Comment 37

[Masatoshi Kimura [:emk]]

(In reply to Florian Quèze [:florian] [:flo] from comment #35)
> Is the '+ i' part really needed? I thought we had code automatically
> appending a number to the file name to avoid overwriting an existing file
> when downloading 2 files with the same name.

Hm, the auto-append didn't work somehow. If the page has multiple data: URL and I remove |+ i|, only one data: URL image remains. Moreover, this is not limited to data:/blob: URL. If the page has different images that have the same name, only one file will remain. Even worse, if the target folder contains a read-only file and one of saved file has the same name as the file, download will fail and the existing will be removed. The read-only attribute didn't prevent the file beeing removed due to bug 921229 change!

Probably we should not land the fix for bug 1207168 without fixing this problem because this is a potential dataloss bug.

Comment 38

[Masatoshi Kimura [:emk]]

Attachment: Do not swallow exceptions if the URI is not an instance of a standard URL, v2

(In reply to Florian Quèze [:florian] [:flo] from comment #35)
> I wonder if this try/catch should be replaced with:
> if (uri instanceof Ci.nsIURL)
>   dir.append(decodeURIComponent(uri.fileName));
> else
>   ...
> 
> It may be better to not change this now if we are hoping to uplift this
> bug's fix though.

OK, I didn't change this at the moment to make uplift easier.

> The hard coded English "download" string is unfortunate here. How about
> using gImageView.data[v][COL_IMAGE_TYPE] instead?

Good idea, done.

> Is the '+ i' part really needed? I thought we had code automatically
> appending a number to the file name to avoid overwriting an existing file
> when downloading 2 files with the same name.

If aChosenData is non-null, internalSave() will not guarantee the uniqueness of the file. I added an explicit uniqueFile() call to ensure the uniqueness.

Comment 39

[Florian Quèze [:florian]]

Comment on attachment 8710352 [details] [diff] [review]
Do not swallow exceptions if the URI is not an instance of a standard URL, v2

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

Looks good to me, thanks!

Comment 40

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d57240301025

Comment 41

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a390b0e2add

Comment 42

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77b033798079b489462c52da70e7cc1bc5852af6
Bug 1211243 - Do not swallow exceptions if the URI is not an instance of a standard URL. r=florian

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/77b033798079

Comment 44

[Masatoshi Kimura [:emk]]

Comment on attachment 8710352 [details] [diff] [review]
Do not swallow exceptions if the URI is not an instance of a standard URL, v2

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Approval Request Comment
[Feature/regressing bug #]: bug 921229
[User impact if declined]: Some Windows special folders will lose the localized name. Also, if bug 1207168 uplift is approved, save as from page info may delete read-only files silently.
[Describe test coverage new/current, TreeHerder]: Baked on m-c and manually tested
[Risks and why]: Low, only tweaks saved file names in some cases.
[String/UUID change made/needed]: none

Comment 45

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8710352 [details] [diff] [review]
Do not swallow exceptions if the URI is not an instance of a standard URL, v2

Fix a regression, taking it.
Should be in 45 beta 2.

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/aa50313832e7

Comment 47

[Andrei Vaida [:avaida]]

magicp, could you please confirm whether this is fixed on 45.0b2 [1]? I'd really appreciate it.

[1] https://archive.mozilla.org/pub/firefox/candidates/45.0b2-candidates/build1/

Comment 48

[magicp]

(In reply to Andrei Vaida, QA [:avaida] from comment #47)
> magicp, could you please confirm whether this is fixed on 45.0b2 [1]? I'd
> really appreciate it.

It works fine! Windows system folders are not broken. Thank you.

Comment 49

[Andrei Vaida [:avaida]]

(In reply to magicp from comment #48)
> (In reply to Andrei Vaida, QA [:avaida] from comment #47)
> > magicp, could you please confirm whether this is fixed on 45.0b2 [1]? I'd
> > really appreciate it.
> 
> It works fine! Windows system folders are not broken. Thank you.

Great! Thanks for taking the time to verify this. I'm gonna go ahead and update the status flags accordingly.

Comment 50

[Rainer Bielefeld]

Version due to report

Comment 51

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
STR: 
1. Open "about:home"
2. View Page Info
3. Select "Media" tab
4. Click "Select All" button
5. Click "Save As" button
6. Select download folder (e.g. C:\Users\magicp\Downloads)
7. After clicking on Downloads, leftside download folder should be highlighted. 

Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Expected Results: 
Downloads folder highlight in explorer window when 
"open folder" of downloads.

Actual Results: 
As expected

Comment 52

[Florian Quèze [:florian]]

VERIFIED per comment 51.