Closed Bug 1211243 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Page Info Window, defect, critical)

41 Branch
Unspecified
Windows
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Firefox 46
Tracking Status
firefox41 --- wontfix
firefox42 - wontfix
firefox43 - wontfix
firefox44 - wontfix
firefox45 --- verified
firefox46 --- fixed
firefox-esr38 - affected

People

(Reporter: magicp.jp, Assigned: emk)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

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
Component: Untriaged → Page Info Window
OS: Unspecified → Windows
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
Blocks: 921229
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(dteller)
Keywords: regression
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.
Flags: needinfo?(dteller)
How about next step?
Flags: needinfo?(dteller)
Florian, do you have any idea how Page Info works? Where would it use OS.File?
Flags: needinfo?(dteller) → needinfo?(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.
Flags: needinfo?(florian)
(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`.
Flags: needinfo?(florian)
I don't see any obvious reason for remove() to be called while saving images.
Flags: needinfo?(florian)
How about next steps?
Flags: needinfo?(dteller)
No idea, I'm afraid.
Flags: needinfo?(dteller)
Long outstanding problem
Version: 41 Branch → unspecified
Depends on: 1207168
Bug 921229 destroy localized windows file system!
Please backed out Bug 921229!
Severity: normal → critical
Flags: needinfo?(fhd)
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?
Flags: needinfo?(magicp.jp)
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?
Flags: needinfo?(alice0775)
(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #14)

> Alice0775, are you confident about your regression range?

100% sure.
Flags: needinfo?(alice0775)
(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
Flags: needinfo?(magicp.jp)
So what happened at low-level? Did Desktop.ini disappear? Was it modified by the call to Save All?
Flags: needinfo?(magicp.jp)
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.
So it's always the name of the directory to which the files were saved, right? No other directories are affected?
Flags: needinfo?(alice0775)
(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.
Flags: needinfo?(alice0775)
(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.
Flags: needinfo?(magicp.jp)
(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.
Flags: needinfo?(magicp.jp)
(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?
Flags: needinfo?(magicp.jp)
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.
Attached file firefox-log.txt
> 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.
Summary: Windows folder is broken by running save all media in the Page Info → Windows folder is broken by running save all media in the Page Info if the media contain an blob URL
(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.
(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.
Summary: Windows folder is broken by running save all media in the Page Info if the media contain an blob URL → Windows folder is broken by running save all media in the Page Info if the media contain an blob/data URL
I made another try build. Please test.

Win32:
https://archive.mozilla.org/pub/firefox/try-builds/VYV03354@nifty.ne.jp-70ed1514a1204fd714234e6dda18ea43286b8c92/try-win32/firefox-46.0a1.en-US.win32.installer.exe
Win64:
https://archive.mozilla.org/pub/firefox/try-builds/VYV03354@nifty.ne.jp-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).
Flags: needinfo?(magicp.jp)
(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.
Flags: needinfo?(magicp.jp)
(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.
(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.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #8709912 - Flags: review?(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.
Attachment #8709912 - Flags: review?(florian) → review-
(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.
(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.
(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.
Attachment #8709912 - Attachment is obsolete: true
Attachment #8710352 - Flags: review?(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!
Attachment #8710352 - Flags: review?(florian) → review+
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
https://hg.mozilla.org/mozilla-central/rev/77b033798079
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
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
Attachment #8710352 - Flags: approval-mozilla-beta?
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.
Attachment #8710352 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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/
Flags: needinfo?(magicp.jp)
(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.
Flags: needinfo?(magicp.jp)
(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.
Version due to report
Version: unspecified → 41 Branch
[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
VERIFIED per comment 51.
Status: RESOLVED → VERIFIED
Flags: needinfo?(fhd)
You need to log in before you can comment on or make changes to this bug.