Closed
Bug 1009465
Opened 11 years ago
Closed 10 years ago
opened files (using "Open with") are no longer set read only
Categories
(Toolkit :: Downloads API, defect)
Tracking
()
People
(Reporter: ngrcld, Assigned: sahukariganesh2, Mentored)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: regression, Whiteboard: [unix fixed by bug 1074793, bug 1022816 needed for windows])
Attachments
(1 file, 2 obsolete files)
6.14 KB,
patch
|
Paolo
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
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
Comment 1•11 years ago
|
||
Does this reproduce if you restart with add-ons disabled (Help > Restart with add-ons disabled) ?
Component: Untriaged → Download Manager
Flags: needinfo?(ngrcld)
Product: Firefox → Toolkit
Comment 2•11 years ago
|
||
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...?
Flags: needinfo?(ngrcld) → needinfo?(paolo.mozmail)
Reporter | ||
Comment 3•11 years ago
|
||
This reproduce if I restart with add-ons disabled
Comment 4•11 years ago
|
||
(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?
Flags: needinfo?(paolo.mozmail) → needinfo?(dteller)
Updated•11 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 5•11 years ago
|
||
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).
Flags: needinfo?(dteller)
Comment 6•11 years ago
|
||
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...
Comment 7•11 years ago
|
||
(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.
Flags: needinfo?(paolo.mozmail)
Updated•11 years ago
|
Flags: needinfo?(dteller)
Comment 8•10 years ago
|
||
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?
Comment 9•10 years ago
|
||
(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.
Updated•10 years ago
|
Blocks: jsdownloads
Flags: firefox-backlog+
Comment 11•10 years ago
|
||
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+.
Blocks: 858234
status-firefox32:
--- → affected
status-firefox33:
--- → affected
status-firefox34:
--- → affected
status-firefox35:
--- → affected
status-firefox-esr31:
--- → affected
Version: 29 Branch → 26 Branch
Updated•10 years ago
|
Flags: qe-verify?
Updated•10 years ago
|
Flags: qe-verify? → qe-verify+
Updated•10 years ago
|
Assignee: nobody → mkmelin+mozilla
Updated•10 years ago
|
OS: Windows XP → All
Hardware: x86 → All
Comment 12•10 years ago
|
||
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.
Depends on: 1074793
OS: All → Windows 7
Comment 13•10 years ago
|
||
(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.
Depends on: 1022816
Updated•10 years ago
|
Assignee: mkmelin+mozilla → nobody
Whiteboard: [unix fixed by bug 1074793, bug 1022816 needed for windows]
Comment 14•10 years ago
|
||
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.
Comment 15•10 years ago
|
||
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.
Assignee | ||
Comment 16•10 years ago
|
||
i would like to work on this bug. Can you assign me the bug
Updated•10 years ago
|
Assignee: nobody → sahukariganesh2
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8576811 -
Flags: review?(paolo.mozmail)
Comment 18•10 years ago
|
||
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.
Attachment #8576811 -
Flags: review?(paolo.mozmail) → feedback+
Assignee | ||
Comment 19•10 years ago
|
||
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"
Attachment #8576811 -
Attachment is obsolete: true
Attachment #8586280 -
Flags: review?(paolo.mozmail)
Comment 20•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8586280 -
Attachment is patch: true
Comment 21•10 years ago
|
||
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));
Attachment #8586280 -
Flags: review?(paolo.mozmail) → review+
Comment 22•10 years ago
|
||
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?
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8586280 -
Attachment is obsolete: true
Attachment #8586912 -
Flags: checkin?(paolo.mozmail)
Assignee | ||
Comment 24•10 years ago
|
||
(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 25•10 years ago
|
||
Comment on attachment 8586912 [details] [diff] [review]
bug-1009465-fix_v3
Pretty sure you meant to ask for review here.
Attachment #8586912 -
Flags: checkin?(paolo.mozmail) → review?(paolo.mozmail)
Assignee | ||
Comment 26•10 years ago
|
||
(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
Comment 27•10 years ago
|
||
(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!
Comment 28•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•10 years ago
|
Iteration: --- → 40.1 - 13 Apr
Assignee | ||
Comment 29•10 years ago
|
||
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.
Updated•10 years ago
|
status-firefox38:
--- → affected
status-firefox39:
--- → affected
Comment 30•10 years ago
|
||
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+)
Attachment #8586912 -
Flags: approval-mozilla-beta?
Attachment #8586912 -
Flags: approval-mozilla-aurora?
Comment 31•10 years ago
|
||
Ganesh, bugzilla is not the good place for support.
Comment 32•10 years ago
|
||
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.
Attachment #8586912 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 33•10 years ago
|
||
Comment 34•10 years ago
|
||
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.)
Updated•10 years ago
|
Mentor: paolo.mozmail
Comment 35•10 years ago
|
||
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 :-)
Attachment #8586912 -
Flags: review?(paolo.mozmail) → review+
Assignee | ||
Comment 36•10 years ago
|
||
(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.
Comment 37•10 years ago
|
||
(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.
Comment 38•10 years ago
|
||
Florin, is it possible to verify this on nightly & aurora? Thanks!
Flags: needinfo?(florin.mezei)
Comment 39•10 years ago
|
||
Assigning to Catalin as he is responsible for Downloads bugs.
Flags: needinfo?(florin.mezei)
QA Contact: catalin.varga
Comment 40•10 years ago
|
||
(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.
Comment 41•10 years ago
|
||
Verified as fixed using the following environment:
FF 40
Build Id: 20150421092928
FF 39
Build Id: 20150421004053
OS: Win 7 x64
Updated•10 years ago
|
Status: RESOLVED → VERIFIED
Comment 42•10 years ago
|
||
Comment on attachment 8586912 [details] [diff] [review]
bug-1009465-fix_v3
[Triage Comment]
Thanks, let's take it for 38 beta 7 then.
Attachment #8586912 -
Flags: approval-mozilla-beta? → approval-mozilla-release+
Comment 43•10 years ago
|
||
Comment 44•10 years ago
|
||
Updated•10 years ago
|
status-firefox38.0.5:
--- → fixed
Comment 45•10 years ago
|
||
Verified as fixed using :
FF 38.0b9
Build Id: 20150429135941
OS: Win 7 x64
Comment 46•10 years ago
|
||
Verified on 40 Nightly, Aurora 39 and Beta 38 - removing qe-verify flag.
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•