opened files (using "Open with") are no longer set read only

VERIFIED FIXED in Firefox 38

Status

()

defect
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: ngrcld, Assigned: sahukariganesh2, Mentored)

Tracking

(Depends on 1 bug, Blocks 1 bug, {regression})

26 Branch
mozilla40
All
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(firefox32 wontfix, firefox33 wontfix, firefox34 wontfix, firefox35 wontfix, firefox37 wontfix, firefox38 verified, firefox38.0.5 fixed, firefox39 verified, firefox40 verified, firefox-esr31 wontfix)

Details

(Whiteboard: [unix fixed by bug 1074793, bug 1022816 needed for windows])

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

5 years ago
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

5 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

5 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

5 years ago
This reproduce if I restart with add-ons disabled

Comment 4

5 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

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
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

5 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...
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(dteller)
Keywords: regression

Comment 7

5 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)
Flags: needinfo?(dteller)
Blocks: 1023402
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

5 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.
Depends on: 1028366
No longer depends on: 1028366

Updated

5 years ago
Duplicate of this bug: 1062524

Updated

5 years ago
Blocks: jsdownloads
Flags: firefox-backlog+

Comment 11

5 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
Version: 29 Branch → 26 Branch
Flags: qe-verify?

Updated

5 years ago
Flags: qe-verify? → qe-verify+
Assignee: nobody → mkmelin+mozilla
OS: Windows XP → All
Hardware: x86 → All

Updated

5 years ago
Depends on: 1093054
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

5 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
Assignee: mkmelin+mozilla → nobody
Whiteboard: [unix fixed by bug 1074793, bug 1022816 needed for windows]

Comment 14

4 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

4 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

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

Updated

4 years ago
Assignee: nobody → sahukariganesh2

Updated

4 years ago
Status: NEW → ASSIGNED
Assignee

Comment 17

4 years ago
Posted patch bug-1009465-fix.patch (obsolete) — Splinter Review
Attachment #8576811 - Flags: review?(paolo.mozmail)

Comment 18

4 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

4 years ago
Posted patch bug-1009465-fix_v2 (obsolete) — Splinter Review
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

4 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

4 years ago
Attachment #8586280 - Attachment is patch: true

Comment 21

4 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

4 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

4 years ago
Attachment #8586280 - Attachment is obsolete: true
Attachment #8586912 - Flags: checkin?(paolo.mozmail)
Assignee

Comment 24

4 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 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

4 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

4 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!
https://hg.mozilla.org/mozilla-central/rev/c061b1170d6a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Iteration: --- → 40.1 - 13 Apr
Assignee

Comment 29

4 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.
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 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 34

4 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

4 years ago
Mentor: paolo.mozmail

Comment 35

4 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

4 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.
(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!
Flags: needinfo?(florin.mezei)
Assigning to Catalin as he is responsible for Downloads bugs.
Flags: needinfo?(florin.mezei)
QA Contact: catalin.varga

Comment 40

4 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.
Verified as fixed using the following environment:

FF 40
Build Id: 20150421092928
FF 39
Build Id: 20150421004053
OS: Win 7 x64
Status: RESOLVED → VERIFIED
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+

Updated

4 years ago
Depends on: 1159632
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.