User Agent: Mozilla/5.0 (Windows NT 5.1; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release) Build ID: 20140506152807 Steps to reproduce: I click on a link to a document, and choose "Open with" Actual results: The temporary file is not set read only Expected results: The temporary file should be set read only. Previous versions of Firefox set the file as read only. I think this is a regression, there are many previous bugs marked as fixed, for example https://bugzilla.mozilla.org/show_bug.cgi?id=280419

Does this reproduce if you restart with add-ons disabled (Help > Restart with add-ons disabled) ?

So I don't see the read only flag being set on either 29 or 28, at least on my Windows XP system. Paolo, do you know if there was a conscious decision made here, or if this is simply an older regression, or...?

This reproduce if I restart with add-ons disabled

(In reply to :Gijs Kruitbosch from comment #2) > So I don't see the read only flag being set on either 29 or 28, at least on > my Windows XP system. Paolo, do you know if there was a conscious decision > made here, or if this is simply an older regression, or...? It is not intentional, in fact we should probably mark the file in the temporary directory as read-only when we plan to open it directly. It might not be trivial, as we need to do this only in the right case and asynchronously. I couldn't find and OS.File API to change file attributes asynchronously. David, is there an API to do that? Or maybe we can enhance the proposed setPermissions API?

So far, the API doesn't exist. As discussed somewhere else, I'd be quite happy if we got around to implementing it as (e.g. OS.File.setPermissions).

In the meantime, based on comment #0 this is a regression. Paolo, from what you're saying, that's due to the new downloads API? Is there a bug on file for the setPermissions work, David? I recall a newsgroup thread, at least...

(In reply to :Gijs Kruitbosch from comment #6) > In the meantime, based on comment #0 this is a regression. Paolo, from what > you're saying, that's due to the new downloads API? It's related to the changes to the downloads code, probably the background file saving is involved. > Is there a bug on file for the setPermissions work, David? I recall a > newsgroup thread, at least... The bug filed at the time of the newsgroup thread was bug 1001849, but I've not seen progress since then.

I know this is how we used to do it, but is there an independent reason why we *want* files downloaded as a side-effect of "Open with" to be read-only?

(In reply to Zack Weinberg (:zwol) from comment #8) > I know this is how we used to do it, but is there an independent reason why > we *want* files downloaded as a side-effect of "Open with" to be read-only? The file is saved to a system temporary directory to which the user has no easy access, thus the program opening the file should not allow any changes to be saved in place.

Regression window(m-c) Good: http://hg.mozilla.org/mozilla-central/rev/43c0123f158b Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130604 Firefox/24.0 ID:20130604141615 Bad: http://hg.mozilla.org/mozilla-central/rev/22cb668fd727 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130604 Firefox/24.0 ID:20130604174517 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=43c0123f158b&tochange=22cb668fd727 Regression window(m-i) Good: http://hg.mozilla.org/integration/mozilla-inbound/rev/ca43cd65708b Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130604 Firefox/24.0 ID:20130604084416 Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/b6cce1e41253 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130604 Firefox/24.0 ID:20130604092414 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ca43cd65708b&tochange=b6cce1e41253 Regressed by: b6cce1e41253 Monica Chew — Move execution from nsExternalAppHandler to nsDownload (b=858234, r=paolo) The offending patch was backed out by Bug 916126 in Firefox ESR24 and Firefox25.0. But not Firefox26+.

Bug 1074793 fixed this for the old download manager, and for *nix platforms in the (new) js download manager. Fixing bug 1022816 would solve this. I guess the other option is using a nsIFile.permissions = 0400 somewhere.

(In reply to Magnus Melin from comment #12) > I guess the other option is using a nsIFile.permissions = 0400 somewhere. We don't really have this option as the call would introduce synchronous I/O and block the UI. Marking bug 1022816 as a dependency.

I've just come across this code: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadPlatform.cpp#147 It seems that files on Windows are indexed by default, so this code should not be needed. What I think happened is that when we set the file as read-only we also excluded it from indexing as an unwanted side-effect: http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileWin.cpp#3293 We're creating a better API in bug 1022816, so this whole code block can be removed.

The code to adjust with the new OS.File API is here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#647 We should only set the read only flag in the isTemporaryDownload case, otherwise we should not specify any winAttributes, in order to avoid unnecessary I/O.

i would like to work on this bug. Can you assign me the bug

Comment on attachment 8576811 [details] [diff] [review] bug-1009465-fix.patch Review of attachment 8576811 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Have you tested manually that this makes the downloaded file read-only? For a performance improvement, do you think you can try removing the C++ code from comment 14, rebuild, and see from the properties if the file now is not excluded from Windows indexing, thanks to the new OS.File API doing the right thing? We'll need an xpcshell test, I suggest editing this one to run also when Services.appinfo.OS == "WINNT" and do the correct check in that case: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/test/unit/common_test_Download.js#191 I believe this will correctly exclude Android from running the test. ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm @@ +656,5 @@ > // this system, while temporary downloads are marked as read-only. > + let options = {}; > + if (isTemporaryDownload) { > + options["unixMode"] = 0o400; > + options["winAttributes"] = {readOnly: true}; These can simply be options.unixMode and options.winAttributes.

Removed the following code segment http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadPlatform.cpp#147 But i haven't removed the other code segment mentioned in comment 14, as i see setFileAttributesWin is being used in one other place http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#2852 Can i know the difference between these two "downloads" & "jsdownloads"

(In reply to Ganesh Sahukari from comment #19) > Can i know the difference between these two "downloads" & "jsdownloads" In short, "jsdownloads" is the newest code, while "downloads" is the old version, still used by Thunderbird and SeaMonkey, although these applications are now migrating to the new version as well. So, yes, we should keep the setFileAttribtesWin API in place.

Comment on attachment 8586280 [details] [diff] [review] bug-1009465-fix_v2 Review of attachment 8586280 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Only a few comment changes are needed, so I started a tryserver build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eff3cba0d5c1 Can you please attach a new version with the following full commit message, then set the "checkin-needed" keyword if the tryserver push is green? Bug 1009465 - Set the read-only attribute for temporary downloads on Windows. r=paolo There is no need to ask for review again on the new patch. ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm @@ -654,5 @@ > Services.prefs.getBoolPref("browser.helperApps.deleteTempFileOnExit")); > // Permanently downloaded files are made accessible by other users on > // this system, while temporary downloads are marked as read-only. > - let unixMode = isTemporaryDownload ? 0o400 : 0o666; > - // On Unix, the umask of the process is respected. This call has no Should keep the part saying "On Unix, the umask of the process is respected." ::: toolkit/components/jsdownloads/test/unit/common_test_Download.js @@ +194,5 @@ > add_task(function test_unix_permissions() > { > // This test is only executed on Linux and Mac. > + if (Services.appinfo.OS != "Darwin" && Services.appinfo.OS != "Linux" && > + Services.appinfo.OS != "WINNT") { Update comment: "This test is only executed on some Desktop systems." @@ +239,5 @@ > + // On Linux, Mac > + // Temporary downloads should be read-only and not accessible to other > + // users, while permanently downloaded files should be readable and > + // writable as specified by the system umask. > + do_check_eq(stat.unixMode, isTemporary ? 0o400 : (0o666 & ~OS.Constants.Sys.umask)); Indent like this to stay within 80 characters: do_check_eq(stat.unixMode, isTemporary ? 0o400 : (0o666 & ~OS.Constants.Sys.umask));

Also, after removing the C++ code, have you tested manually that the attribute that excludes the file from search indexing is still not set on a normal download?

(In reply to :Paolo Amadini from comment #22) > Also, after removing the C++ code, have you tested manually that the > attribute that excludes the file from search indexing is still not set on a > normal download? But checked the file Attributes before and after removing the C++ code, i see only "RA" attributes are set in both cases. I see the "I Not content-indexed" have not been set in both cases.

Comment on attachment 8586912 [details] [diff] [review] bug-1009465-fix_v3 Pretty sure you meant to ask for review here.

(In reply to :Paolo Amadini from comment #22) > Also, after removing the C++ code, have you tested manually that the > attribute that excludes the file from search indexing is still not set on a > normal download? Verified after removing the C++ code, the file Attribute that excludes the file from search indexing is still not been set on normal download. Even it was not set on temp file download also, as windows is by default is indexing all files. Try build have been failed in 4 cases, but there are bugs raised for those build failures

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25) > Pretty sure you meant to ask for review here. That was actually a checkin request, though the commit message in the patch wasn't updated to include the reviewer. Ganesh, for future reference, with Mercurial Queues you can use the command "hg qrefresh -e" and update the commit message to include, for example, "r=paolo" at the end. You can then add "checkin-needed" to the "keywords" on the bug to request the patch to be checked in. I've done the checkin with the updated message now: https://hg.mozilla.org/integration/fx-team/rev/c061b1170d6a Sorry for this procedural delay, this change should be in mozilla-central soon!

While working on this bug, I have seen the following code snippet in the toolkit/components/jsdownloads/src/DownloadIntegration.jsm downloadDone: function(aDownload) { ... gDownloadPlatform.downloadDone(NetUtil.newURI(aDownload.source.url), new FileUtils.File(aDownload.target.path), aDownload.contentType, aDownload.source.isPrivate); } gDownloadPlatform.downloadDone is a C++ function from DownloadPlatform.cpp, How can we call a C++ function from javascript code, Can someone explain this.

Comment on attachment 8586912 [details] [diff] [review] bug-1009465-fix_v3 Approval Request Comment [Feature/regressing bug #]: bug 858234 [User impact if declined]: windows users easily lose their changes, if they open a downloaded document for editing and don't realize their changes are only in a temp file that they won't find later, or the system removes at some point [Describe test coverage new/current, TreeHerder]: has test [Risks and why]: does not look risky. Due to the use case this bug affects thunderbird a great deal, which I'd like it to get uplifted to 38 esr. [String/UUID change made/needed]: none (this patch already landed, paolo please mark it r+)

Ganesh, bugzilla is not the good place for support.

Comment on attachment 8586912 [details] [diff] [review] bug-1009465-fix_v3 We have this bug for almost a year, we shipped esr 31 with it. I am not sure to see the point of taking this risk for beta. However, let's take it in aurora and see what happens.

Hey Ganesh, congratulations for fixing this bug! This was one of the long-standing issues we'd like to fix but tend to slip in our prioritization due to our focus on new features, so it was a big help. (In reply to Ganesh Sahukari from comment #29) > gDownloadPlatform.downloadDone is a C++ function from DownloadPlatform.cpp, > > How can we call a C++ function from javascript code, Can someone explain > this. The technologies involved are XPCOM and XPConnect: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_bindings/XPConnect Unfortunately, the articles above aren't really a good introduction as they tend to focus on the internals. You may simply want to take a look at the IDL file that enables the communication: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/public/mozIDownloadPlatform.idl Confusingly, the fact that DownloadPlatform.cpp is the implementation file for the component is defined here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/build/nsToolkitCompsModule.cpp#164 I can find you some bugs that involve IDL work if you'd like to explore this technology, or let me know if there is something else you'd be interested into. (By the way, it's fine to ask for technical details in bugs, especially if they're mentored, I think what Sylvestre meant is that we also have technical newsgroups like mozilla.dev.platform where you can ask for help from our wider community if you're working on something and are stuck on some detail.)

Comment on attachment 8586912 [details] [diff] [review] bug-1009465-fix_v3 (In reply to Magnus Melin from comment #30) > (this patch already landed, paolo please mark it r+) This already had r+ in a previous version, but repeating doesn't hurt :-)

(In reply to :Paolo Amadini from comment #34) > > I can find you some bugs that involve IDL work if you'd like to explore this > technology, or let me know if there is something else you'd be interested > into. > Thanks Paolo, i would be very helpful if you can find me some bugs involving IDL work.

(In reply to :Paolo Amadini from comment #34) > (By the way, it's fine to ask for technical details in bugs, especially if > they're mentored, I think what Sylvestre meant is that we also have > technical newsgroups like mozilla.dev.platform where you can ask for help > from our wider community if you're working on something and are stuck on > some detail.) Sorry, I realized later that he was the assignee of this bug. Sorry about that.

Florin, is it possible to verify this on nightly & aurora? Thanks!

Assigning to Catalin as he is responsible for Downloads bugs.

(In reply to Ganesh Sahukari from comment #36) > Thanks Paolo, i would be very helpful if you can find me some bugs involving > IDL work. Sorry for the delay, it actually took some time to find a good next bug. I've filed bug 1155643, let me know if you'd like to take it. Another option is bug 973757, mostly involving moving files around and updating moz.build accordingly. I've also asked around about a few more bugs I'm not sure about, I'll get back to you if something becomes available.

Verified as fixed using the following environment: FF 40 Build Id: 20150421092928 FF 39 Build Id: 20150421004053 OS: Win 7 x64

Comment on attachment 8586912 [details] [diff] [review] bug-1009465-fix_v3 [Triage Comment] Thanks, let's take it for 38 beta 7 then.

Verified as fixed using : FF 38.0b9 Build Id: 20150429135941 OS: Win 7 x64

Verified on 40 Nightly, Aurora 39 and Beta 38 - removing qe-verify flag.