If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

OS.File should be able to change the read-only attribute on Windows

RESOLVED FIXED in Firefox 38

Status

()

Toolkit
OS.File
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: zwol, Assigned: Ganesh Sahukari, Mentored)

Tracking

unspecified
mozilla39
All
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 fixed, firefox38.0.5 fixed, firefox39 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

3 years ago
Bug 1001849 adds a .setPermissions() API to OS.File.  Its interface and behavior are currently specified only for Unixy systems; it does nothing on Windows.  We need to figure out what a reasonable API to Windows file access permissions looks like, and then someone needs to implement it.  Both tasks are better done by someone with a much deeper understanding of Windows' native filesystem semantics than me (read: any understanding at all ;-)
What's the use-case?  I think we will find a "full" implementation isn't feasible and a separate js-ctypes-based wrapper of the raw APIs might make more sense.  OTOH, I doubt you want a "full" impl...
(Reporter)

Updated

3 years ago
Blocks: 1023402
(Reporter)

Comment 2

3 years ago
The one thing we know we need to do (per bug 961080) is "this file was created in a way that makes it accessible only to the current user; reset its permissions so that, if user-account-wide configuration says so, it will be accessible to other users as well."  But I don't know if that is even a coherent problem statement in Windows land.
(In reply to Zack Weinberg (:zwol) from comment #2)
> The one thing we know we need to do (per bug 961080) is "this file was
> created in a way that makes it accessible only to the current user; reset
> its permissions so that, if user-account-wide configuration says so, it will
> be accessible to other users as well."  But I don't know if that is even a
> coherent problem statement in Windows land.

I believe that if a file is created without explicitly specifying security attributes, the file will inherit the permissions from the directory (which itself is inherited from its parent, etc).

This means that the default download location, which is under the user's "home" directory, will automatically do the right thing.  If the user specifies a folder on a network share, for example, I'd argue the user *does not* want such permissions applied (unless that shared folder already has such permissions) - they are presumably saving the file to a shared location on purpose with the intent of actually sharing it.

To put it another way: on Windows, I think we would expect that saving a downloaded file to a specific directory should end up with the same permissions as if the user saved it to their "private" downloads directory then used Windows Explorer to copy it to the final location.  I believe this will happen automatically.
(Reporter)

Updated

3 years ago
Depends on: 1028366
(Reporter)

Updated

3 years ago
No longer depends on: 1028366

Comment 4

3 years ago
At the least, the readonly attribute should be set on the file when setting permissions like 0400. 
nsIFile SetPermissions already does supports this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to Magnus Melin from comment #4)
> At the least, the readonly attribute should be set on the file when setting
> permissions like 0400. 
> nsIFile SetPermissions already does supports this.

Agreed - and that's likely to the the real use-case for this bug.  I'd still argue that flag is a distinct concept from "permissions", but we should manage the r/o flag (and I'd be happy to morph this bug into that - Zach - what do you think?)
Flags: needinfo?(zackw)
(to be clear - I'm talking about the r/o "attribute" in Windows which is unrelated to "permissions" - even though permissions can obviously have the same affect)
(Reporter)

Comment 7

3 years ago
(In reply to Mark Hammond [:markh] from comment #5)
> (In reply to Magnus Melin from comment #4)
> > At the least, the readonly attribute should be set on the file when setting
> > permissions like 0400. 
> > nsIFile SetPermissions already does supports this.
> 
> Agreed - and that's likely to the the real use-case for this bug.  I'd still
> argue that flag is a distinct concept from "permissions", but we should
> manage the r/o flag (and I'd be happy to morph this bug into that - Zach -
> what do you think?)

I can't really speak to this; I don't know how the r/o file "attribute" in Windows is used in practice, and the use cases I'm familiar with, from the Unix side, do not involve setting files read-only for *everyone*.  In particular I think the download manager never does that.

I've just noticed I never replied to an earlier thing:

(In reply to Mark Hammond [:markh] from comment #3)
> (In reply to Zack Weinberg (:zwol) from comment #2)
> > The one thing we know we need to do (per bug 961080) is "this file was
> > created in a way that makes it accessible only to the current user; reset
> > its permissions so that, if user-account-wide configuration says so, it will
> > be accessible to other users as well."  But I don't know if that is even a
> > coherent problem statement in Windows land.
> 
> I believe that if a file is created without explicitly specifying security
> attributes, the file will inherit the permissions from the directory (which
> itself is inherited from its parent, etc).

So this is how the download manager *used* to do things on the Unix side, and this was deliberately changed such that the file is initially created with permissions that forbid access to anyone other than the creating user ID.  I do not think I ever saw a full explanation for this change, but it was intentional.  To the extent that the download manager *doesn't* do that on Windows I suspect it is simply because no one involved knew how to implement it, and the same security considerations, whatever they were, apply.

Digging into the history a little I see this (Paolo Amadini, bug 919076 comment 96):
> The technical issue with this specific patch is that we should ensure that
> files that are initially downloaded to the global temporary directory and
> then moved to the final destination should not be group- or world-writable
> while they are in the temporary folder.

so perhaps he can explain further and maybe even comment on the relevance to Windows.

I would imagine that the use case that originally led to my involvement in bug 961080 (corporate environment, downloading files into shared folders on network file servers) comes up just as much or more with Windows clients as it does on Mac and Linux; so while my general attitude continues to be that we should not devote any more resources to non-open-source desktops than we absolutely have to, this particular case seems worth making sure it's reliable.
Flags: needinfo?(zackw) → needinfo?(paolo.mozmail)

Comment 8

3 years ago
(In reply to Zack Weinberg (:zwol) from comment #7)
> I can't really speak to this; I don't know how the r/o file "attribute" in
> Windows is used in practice, and the use cases I'm familiar with, from the
> Unix side, do not involve setting files read-only for *everyone*. 

The use case is that you don't want the user to edit a file, hit save and later find out the file was deleted. So, you do want the file to be readonly to everyone. 

> In particular I think the download manager never does that.

It does, for files that will get deleted automatically later. ("Open with app", and those dl'd in private mode.)

> So this is how the download manager *used* to do things on the Unix side,
> and this was deliberately changed such that the file is initially created
> with permissions that forbid access to anyone other than the creating user
> ID.  I do not think I ever saw a full explanation for this change, but it
> was intentional. 

The file might be in the (global) temp folder so it's a privacy issue to have permissions for others, on shared systems.
(Reporter)

Comment 9

3 years ago
(In reply to Magnus Melin from comment #8)
> (In reply to Zack Weinberg (:zwol) from comment #7)
> > I can't really speak to this; I don't know how the r/o file "attribute" in
> > Windows is used in practice, and the use cases I'm familiar with, from the
> > Unix side, do not involve setting files read-only for *everyone*. 
> 
> The use case is that you don't want the user to edit a file, hit save and
> later find out the file was deleted. So, you do want the file to be readonly
> to everyone.

I'm sorry, I don't follow.  If the file is readonly for *everyone*, then *no one* can write edits to it, including the owning user!  Is that not the effect of the read-only attribute on Windows?

(And *deleting* a file involves a separate privilege from being able to read or write it, I believe on both Unix and Windows.)

> > In particular I think the download manager never does that.
> 
> It does, for files that will get deleted automatically later. ("Open with
> app", and those dl'd in private mode.)

Oh wait, are you saying that the dl manager marks files read-only if it's going to delete them later, so that the downloading user *cannot* save back changes to the original file and has to use "Save As" or equivalent?

> > So this is how the download manager *used* to do things on the Unix side,
> > and this was deliberately changed such that the file is initially created
> > with permissions that forbid access to anyone other than the creating user
> > ID.  I do not think I ever saw a full explanation for this change, but it
> > was intentional. 
> 
> The file might be in the (global) temp folder so it's a privacy issue to
> have permissions for others, on shared systems.

This does seem like it would apply on Windows just as well as Unix.

Comment 10

3 years ago
(In reply to Zack Weinberg (:zwol) from comment #9)
> I'm sorry, I don't follow.  If the file is readonly for *everyone*, then *no
> one* can write edits to it, including the owning user!  Is that not the
> effect of the read-only attribute on Windows?

Yes. Apps handle that the way they want. Some open the file only for viewing, others let you edit, but once you hit save you get "save as" instead. The end result is the same, you have to save it somewhere safe and won't end up losing possibly hours of work.

> (And *deleting* a file involves a separate privilege from being able to read
> or write it, I believe on both Unix and Windows.)

Own files can always be deleted - I believe technically by adding the write bit first. Usually apps ask before they do it.

> Oh wait, are you saying that the dl manager marks files read-only if it's
> going to delete them later, so that the downloading user *cannot* save back
> changes to the original file and has to use "Save As" or equivalent?

Yes.

> > The file might be in the (global) temp folder so it's a privacy issue to
> > have permissions for others, on shared systems.
> 
> This does seem like it would apply on Windows just as well as Unix.

Indeed.
(In reply to Magnus Melin from comment #8)
> 
> The file might be in the (global) temp folder so it's a privacy issue to
> have permissions for others, on shared systems.

On Windows, this temp directory is per-user (ie, under their user profile) and thus is already unavailable to other users.

Updated

3 years ago
Blocks: 1009465

Comment 12

3 years ago
Unfortunately, management of Windows file access privileges is quite complex, and while we don't want to deal with all the edge cases, we have to do a few things, one of which is handling the read-only attribute.

As mentioned, attributes are definitely distinct from the ACL (access lists). On Windows there are no permissions in the Unix sense, but the term permission is used in documentation to describe the privileges granted or denied by the access lists. The concept of ownership is also different from the Unix concept.

(In reply to Magnus Melin from comment #4)
> At the least, the readonly attribute should be set on the file when setting
> permissions like 0400.
> nsIFile SetPermissions already does supports this.

This is to say that I'm not sure if we should map Unix permissions to Windows attributes automatically, support a separate "readOnly" option, or maybe define a separate setAttributes API. The "hidden" attribute is another thing we might want to control with the API.

(In reply to Zack Weinberg (:zwol) from comment #7)
> I can't really speak to this; I don't know how the r/o file "attribute" in
> Windows is used in practice

I believe applications recognize the attribute and may suggest saving a copy to a different folder instead of overwriting if they find it on a file. On the other hand, I'm not sure whether a restrictive ACLs that denies write permissions to the current user has the same effect, or just results in an error when saving.

On the code side, most Windows APIs deny opening a file with the read-only attribute for writing.

> so perhaps he can explain further and maybe even comment on the relevance to
> Windows.

I see most of this has already been discussed in this bug, if you have any question on why we did something in the Downloads code, let me know and I'll be happy to answer.
Flags: needinfo?(paolo.mozmail)
Summary: OS.File.setPermissions needs a Windows implementation → OS.File should be able to change the read-only attribute on Windows
(Assignee)

Comment 13

3 years ago
i would like to work on this bug. Can you assign me the bug
I meant to reply to this earlier but it slipped away!  The way I see it is:

* setFilePermissions is already solved for Unix - see the various linked bugs.

* there's no identified requirement for that under Windows, mainly as temp directories aren't shared by default.

* there is, however, a requirement for something like "try and make this file look transient and like something you don't want to edit/touch" - not a security guarantee, more a UX detail.

On Windows, that probably means "set the hidden and read-only attributes" - I'm not sure what it means on Unix, and it may be that Unix also *enforces* it whereas under Windows it's more a convention - but that sounds OK as the desired API doesn't guarantee enforcement (or non-enforcement ;)

We still have bug 1009465 - shall we morph this bug into the above ill-defined requirement, or maybe just a straight-up Windows-only SetFileAttributes wrapper (https://msdn.microsoft.com/en-us/library/windows/desktop/aa365535%28v=vs.85%29.aspx)?

(In reply to Ganesh Sahukari from comment #13)
> i would like to work on this bug. Can you assign me the bug

How do you intend approaching it?

Comment 15

3 years ago
(In reply to Mark Hammond [:markh] from comment #14)
> On Windows, that probably means "set the hidden and read-only attributes" -
> I'm not sure what it means on Unix, and it may be that Unix also *enforces*
> it whereas under Windows it's more a convention - but that sounds OK as the
> desired API doesn't guarantee enforcement (or non-enforcement ;)

Yeah, for downloads on Windows the read-only attribute is enough to tell the application that opened the file that it is a transient file that cannot be saved in place.

> We still have bug 1009465

This is the bug that will use the API defined here, yes.

> maybe just a straight-up Windows-only SetFileAttributes wrapper
> (https://msdn.microsoft.com/en-us/library/windows/desktop/aa365535%28v=vs.
> 85%29.aspx)?

This is exactly what we need here.
Mentor: paolo.mozmail@amadzone.org

Comment 16

3 years ago
Ganesh, you can start by taking a look at the patch in bug 1001849 - it does a similar thing to what we need here, with the difference that the API is unimplemented on Windows and implemented on Unix, while it will be the opposite here.

The patch includes a test - you can run it locally as a start, later you'll need to add a new test file for the new API. As always, let me know if you have any questions!

David is the module owner of OS.File, I'll ask him for more details about the API to use.

Comment 17

3 years ago
(In reply to :Paolo Amadini from comment #12)
> This is to say that I'm not sure if we should map Unix permissions to
> Windows attributes automatically, support a separate "readOnly" option, or
> maybe define a separate setAttributes API. The "hidden" attribute is another
> thing we might want to control with the API.

David, what do you think is the best API here? Some ideas:

OS.File.setPermissions({ unixMode: 0o400, winReadOnly: true });
OS.File.winSetAttributes({ readOnly: true });
OS.File.setAttributes({ winReadOnly: true });
OS.File.setAttributes({ readOnly: true });

Should we map unixMode: 0o400 to winReadOnly automatically or not?
Flags: needinfo?(dteller)

Comment 18

3 years ago
My 5c: just use unixMode everywhere, maybe rename it to just mode, and make it readonly on windows if the suitable bit are set so. I think APIs like this should be a platform agnostic as possible.

Updated

3 years ago
Assignee: nobody → sahukariganesh2
(In reply to Magnus Melin from comment #18)
> My 5c: just use unixMode everywhere, maybe rename it to just mode, and make
> it readonly on windows if the suitable bit are set so. I think APIs like
> this should be a platform agnostic as possible.

The problem I see with this is that the APIs simply aren't platform agnostic - a chmod is completely different to windows attributes with quite distinct semantics.

The risk is that the casual Unix-experienced developer will not be aware of the differences.  By having it explicit that you *aren't* setting permissions on Windows the developer is forced to make the choice between saying "oh, this API sucks a bit, but in this use-case using the read-only attribute is fine" versus "oh, I really need permissions here even on Windows, but no such API exists - I better dig deeper".  It's difficult to predict the consequences if the developer in that second situation naively assumes the agnostic API works like permissions on Windows.

Comment 20

3 years ago
I tend to agree with comment 19, this would mean we won't map permissions and attributes automatically. I think this still leaves us with two alternatives, for example in the use case of downloads:

  OS.File.setPermissions({ unixMode: 0o400 });
  OS.File.setAttributes({ readOnly: true });

Or:

  OS.File.setPermissions({ unixMode: 0o400, winReadOnly: true });

The first one is more correct because technically we aren't changing "permissions". On the other hand, we might want to treat setPermissions as a generic entry point, because if we choose the first path and then add a way to set ACLs, we should then go for a third API like OS.File.setACLs() rather than something like setPermissions({ winACL: [ ... ]}).
I prefer a single `setPermissions`, but I'm willing to go either way.
Flags: needinfo?(dteller)
(In reply to David Rajchenbach-Teller [:Yoric] (away until Febrary - use "needinfo") from comment #21)
> I prefer a single `setPermissions`, but I'm willing to go either way.

That sounds good to me!

> OS.File.setPermissions({ unixMode: 0o400, winReadOnly: true });

Although maybe ({ unixMode: ..., winAttributes: {readOnly: true, system: true, hidden: true}})

</bikeshed>

Comment 23

3 years ago
(In reply to Mark Hammond [:markh] from comment #22)
> Although maybe ({ unixMode: ..., winAttributes: {readOnly: true, system: true, hidden: true}})

Oooh, that's nice.
(Assignee)

Comment 24

3 years ago
Hi Paolo,

I made the changes and to test that i wrote the xpctest code in similar lines of bug 1001849, where i set the FileAttributes to a file, and to cross verify whether fileAtrributes are set correctly i'm using the OS.File.stat(). But the return object does not contain dwFileAttributes, can i add it or should i need to add individual file properties like readonly, system, hidden.

In the function "AbstractInfo"

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_win_allthreads.jsm
Flags: needinfo?(paolo.mozmail)
I'm not answering for Paolo, but my take is that having .stat() return this data is fine as (a) the .stat() api for windows is already slightly different and (b) the underlying win32 call returns this data so the overhead for people who don't need the attributes is tiny.  However, I'd recommend the result be an object in the same format as accepted via setPermissions - ie, assuming something like comment 23, AbstractInfo would have a 'winAttributes: {readOnly: true, ...}' entry.

What's not clear is what attributes we support and whether 'false' values should be returned.  Off the top of my head, I'd say we just return the same subset we support for setPermissions and always return values, false or true. This way the object can be inspected to see if a value is supported or not (ie, the lack of an attribute implies we *never* return it)

Comment 26

3 years ago
I totally agree with Mark in comment 25, including returning all attributes we know about, with either true or false values. Thank you Ganesh and Mark!
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 27

3 years ago
I have added some logs in setPermissions() fn in "toolkit/components/osfile/modules/osfile_win_front.jsm" via "LOG" for development debugging, but those logs are not showing up when i run setPermissions from an xpcshell test.


I see the setPermissions() in "osfile_async_front.jsm" is called initally. Which in turn call setPermissions() of "osfile_win_front.jsm" on a worker thread.

So, how can i see the debugs inserted via "LOG" from a worker thread while running a xpcshell test
(In reply to Ganesh Sahukari from comment #27)
> I have added some logs in setPermissions() fn in
> "toolkit/components/osfile/modules/osfile_win_front.jsm" via "LOG" for

dump?
Normally, if your xpcshell test is in toolkit/components/osfile/modules/tests/xpcshell, the logs should show up in the terminal. If they don't... it means we have broken something again.
(In reply to David Rajchenbach-Teller [:Yoric] (away until Febrary - use "needinfo") from comment #29)
> Normally, if your xpcshell test is in
> toolkit/components/osfile/modules/tests/xpcshell, the logs should show up in
> the terminal. If they don't... it means we have broken something again.

Only if you run a single test, or use -v, or cause the test to fail - so yeah, great point - make sure you specify just a single test on the command-line.
(Assignee)

Comment 31

3 years ago
i ran single test only and the xpcshell test is in toolkit/components/osfile/modules/tests/xpcshell.
Even i tried with an existing test "toolkit/components/osfile/modules/tests/xpcshell/test_osfile_async_setPermissions.js"

Logs in "osfile_async_front.jsm" are showing up, but the logs in "osfile_win_front.jsm" or "osfile_unix_front.jsm" are not showing up.

setPermissions api in "osfile_async_front.jsm" in turn calling setPermissions api of "osfile_win_front.jsm" via a worker thread


I tried LOG with both "dump" and "console.log", in either case it is not showing up the logs in "osfile_win_front.jsm" or "osfile_unix_front.jsm"
I'm on Windows, so I'll have a look over the next few days if you attach your patch, specify your exact command-line and needinfo me.
(Assignee)

Comment 33

3 years ago
Created attachment 8568635 [details] [diff] [review]
1022816.patch
Attachment #8568635 - Flags: review?(paolo.mozmail)
Attachment #8568635 - Flags: review?(mhammond)
Comment on attachment 8568635 [details] [diff] [review]
1022816.patch

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

LGTM, although I think the validation of the input object is worthwhile.  Note also that I'm not completely confident reviewing OS.File code (I'm not even confident *reading* OS.File code ;), so please wait for Paolo to give it a once-over too.

::: dom/system/OSFileConstants.cpp
@@ +732,5 @@
>    INT_CONSTANT(FILE_ATTRIBUTE_READONLY),
>    INT_CONSTANT(FILE_ATTRIBUTE_REPARSE_POINT),
>    INT_CONSTANT(FILE_ATTRIBUTE_TEMPORARY),
>    INT_CONSTANT(FILE_FLAG_BACKUP_SEMANTICS),
> +  INT_CONSTANT(FILE_ATTRIBUTE_SYSTEM),

it looks like these are alphabetical, so this line should move up a few lines.

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +907,5 @@
>         let size = Type.uint64_t.importFromC(value);
> +       let winAttributes = {
> +                            readOnly: !!(stat.dwFileAttributes & Const.FILE_ATTRIBUTE_READONLY),
> +                            system: !!(stat.dwFileAttributes & Const.FILE_ATTRIBUTE_SYSTEM),
> +                            hidden: !!(stat.dwFileAttributes & Const.FILE_ATTRIBUTE_HIDDEN)

it seems we might as well support all FILE_ATTRIBUTE_* constants we have defined, but not too bothered by this as it is a trivial followup should a use-case be identified for other attributes.

@@ +1187,5 @@
> +      */
> +     function fileAttributes(options) {
> +       let dwAttrs = Const.FILE_ATTRIBUTE_NORMAL;
> +
> +       if ("winAttributes" in options) {

I wonder if this could/should be rewritten to iterate over the object and catch any expected attributes - eg, I think it is a bit of a footgun that 'readonly' will silently do nothing (and if we do this, we'd also want to test it)
Attachment #8568635 - Flags: review?(mhammond)

Comment 35

3 years ago
Comment on attachment 8568635 [details] [diff] [review]
1022816.patch

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

This is a very high quality patch for your third bug, you're doing things right.

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +239,5 @@
>        */
>       File.prototype.setPermissions = function setPermissions(options = {}) {
> +       throw_on_zero("setPermissions",
> +                     WinFile.SetFileAttributes(this._path, fileAttributes(options)),
> +                     this._path);

Apparently, we're hitting a limitation of the Win32 API here, as it always sets all the attributes at the same time. To change only some attributes, we need to read the existing attributes using GetFileAttributes first, as an integer (including attributes we don't support), apply the changes to the bits as requested, and then set the new attributes.

This requires one more file system operation, but since we'll be using this only on download completion, this isn't going to have a real performance impact.

@@ +907,5 @@
>         let size = Type.uint64_t.importFromC(value);
> +       let winAttributes = {
> +                            readOnly: !!(stat.dwFileAttributes & Const.FILE_ATTRIBUTE_READONLY),
> +                            system: !!(stat.dwFileAttributes & Const.FILE_ATTRIBUTE_SYSTEM),
> +                            hidden: !!(stat.dwFileAttributes & Const.FILE_ATTRIBUTE_HIDDEN)

I think supporting just readOnly, system and hidden should be fine for now.

@@ +1187,5 @@
> +      */
> +     function fileAttributes(options) {
> +       let dwAttrs = Const.FILE_ATTRIBUTE_NORMAL;
> +
> +       if ("winAttributes" in options) {

With regard to Mark's comment, we want to support extensibility by ignoring attribute names we don't support. As for case mismatches, this unfortunately may be a general issue with most of our JavaScript APIs, including the rest of OS.File, so I don't think we should be concerned with solving this only for this occurrence.

::: toolkit/components/osfile/tests/xpcshell/test_osfile_win_async_setPermissions.js
@@ +14,5 @@
> +let testSequence = [
> +  [{winAttributes: {readOnly: true, system: true, hidden: true}}],
> +  [{winAttributes: {readOnly: true, system: false, hidden: false}}],
> +  [{winAttributes: {readOnly: false, system: true, hidden: false}}],
> +  [{winAttributes: {readOnly: false, system: false, hidden: true}}]

We'll need tests for cases where we leave attributes unchanged, something like:

let testSequence = [
  { attributesToSet: { readOnly: true, system: true, hidden: true },
    attributesExpected: { readOnly: true, system: true, hidden: true } },
  { attributesToSet: { readOnly: false },
    attributesExpected: { readOnly: false, system: true, hidden: true } },
];
Attachment #8568635 - Flags: review?(paolo.mozmail) → feedback+
(Assignee)

Comment 36

3 years ago
Created attachment 8572722 [details] [diff] [review]
1022816_v2.patch
Attachment #8572722 - Flags: review?(paolo.mozmail)

Comment 37

3 years ago
Comment on attachment 8572722 [details] [diff] [review]
1022816_v2.patch

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

::: dom/system/OSFileConstants.cpp
@@ +732,5 @@
>    INT_CONSTANT(FILE_ATTRIBUTE_READONLY),
>    INT_CONSTANT(FILE_ATTRIBUTE_REPARSE_POINT),
>    INT_CONSTANT(FILE_ATTRIBUTE_TEMPORARY),
>    INT_CONSTANT(FILE_FLAG_BACKUP_SEMANTICS),
> +  INT_CONSTANT(FILE_ATTRIBUTE_SYSTEM),

Alphabetical order.

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +239,3 @@
>        */
>       File.prototype.setPermissions = function setPermissions(options = {}) {
> +       let newAttributes = fileAttributes(options, WinFile.GetFileAttributes(this._path));

GetFileAttributes returns INVALID_FILE_ATTRIBUTES on error, should check if that's the case.

@@ +909,5 @@
> +       let winAttributes = {
> +                            readOnly: !!(stat.dwFileAttributes & Const.FILE_ATTRIBUTE_READONLY),
> +                            system: !!(stat.dwFileAttributes & Const.FILE_ATTRIBUTE_SYSTEM),
> +                            hidden: !!(stat.dwFileAttributes & Const.FILE_ATTRIBUTE_HIDDEN)
> +                           };

nit: you can indent this just two spaces like below, also a comma after the last property is valid JavaScript and we generally recommend it:

let winAttributes = {
  readOnly: val,
  system: val,
  hidden: val,
};

@@ +1186,5 @@
>  
> +     /**
> +      * Helper used by both versions of setPermissions
> +      */
> +     function fileAttributes(options, oldDwAttrs) {

You can call this "changeFileAttributes" now.

@@ +1188,5 @@
> +      * Helper used by both versions of setPermissions
> +      */
> +     function fileAttributes(options, oldDwAttrs) {
> +
> +       if ("winAttributes" in options) {

As an optimization, you should return early if "winAttributes" is not in "options", in the calling function, so we don't access the file system at all on Windows if this for example was a unixMode call.

Then, this function would just take the winAttributes object as its first argument.

::: toolkit/components/osfile/tests/xpcshell/test_osfile_win_async_setPermissions.js
@@ +6,5 @@
> +/**
> + * A test to ensure that OS.File.setPermissions and
> + * OS.File.prototype.setPermissions are all working correctly.
> + * (see bug 1022816)
> + * The manifest tests on Windows.

The test is looking great!

@@ +20,5 @@
> +    { readOnly: false, system: false, hidden: true } ],
> +  [ { winAttributes: { hidden: false } },
> +    { readOnly: false, system: false, hidden: false } ],
> +  [ { winAttributes: {readOnly: true, system: false, hidden: false} },
> +    {readOnly: true, system: false, hidden: false} ],

nit: adjust inconsistent spacing

::: toolkit/components/osfile/tests/xpcshell/xpcshell.ini
@@ +47,5 @@
>  skip-if = os == "win" || os == "android"
> +
> +# Windows test
> +[test_osfile_win_async_setPermissions.js]
> +skip-if = os == "unix" || os == "android"

I've just noticed that we can probably just check:

os != "win"
Attachment #8572722 - Flags: review?(paolo.mozmail) → feedback+

Comment 38

3 years ago
(In reply to :Paolo Amadini from comment #37)
> GetFileAttributes returns INVALID_FILE_ATTRIBUTES on error

Which I believe (but I'm not sure) is -1, we may have a helper function to throw on negative.
(Assignee)

Comment 39

3 years ago
Created attachment 8573340 [details] [diff] [review]
1022816_v3.patch
Attachment #8573340 - Flags: review?(paolo.mozmail)

Comment 40

3 years ago
Comment on attachment 8573340 [details] [diff] [review]
1022816_v3.patch

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

One last change required, please upload a new version and I'll run a tryserver build. No need to request review again.

Note that you can mark previous versions as "obsolete" when you upload a new version, so it's clearer which version needs to be used.

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +243,5 @@
> +       }
> +       let newAttributes = changeFileAttributes(options.winAttributes, WinFile.GetFileAttributes(this._path));
> +       if (newAttributes == Const.INVALID_FILE_ATTRIBUTES) {
> +         throw new File.Error("setPermissions", ctypes.winLastError, this._path);
> +       }

The return value must be checked before it can be inadvertently changed:

let oldAttributes = WinFile.GetFileAttributes(this._path);
if (oldAttributes == Const.INVALID_FILE_ATTRIBUTES) {
  throw ...
}
let newAttributes = ...
throw_on_zero(...)
Attachment #8573340 - Flags: review?(paolo.mozmail) → review+

Comment 41

3 years ago
Comment on attachment 8573340 [details] [diff] [review]
1022816_v3.patch

David, I think this is right apart from the minor issue of the return value check.

While technically my review is enough for Toolkit code, I'd appreciate to hear if you have any other comments on the public API, as the OS.File module owner.
Attachment #8573340 - Flags: feedback?(dteller)
(Assignee)

Comment 42

3 years ago
Created attachment 8573983 [details] [diff] [review]
1022816_v4.patch
Attachment #8568635 - Attachment is obsolete: true
Attachment #8572722 - Attachment is obsolete: true

Comment 43

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0335036d66f4
(Assignee)

Comment 44

3 years ago
The build has 7 unclassified failures, seems there are bugs raised for those failures other than the following one

OS X 10.8 opt
command timed out: 10800 seconds without output running ['/tools/buildbot/bin/python', 'scripts/scripts/fx_desktop_build.py', '--config', 'builds/releng_base_mac_64_builds.py', '--branch', 'try', '--build-pool', 'production'], attempting to kill

Comment 45

3 years ago
Yes, I think this patch is ready to land if David doesn't have any further comments.

In the meantime, you may want to prototype the fix for bug 1009465 on top of this new API? You should be able to keep multiple patches in your working directory if you're using Mercurial Queues, and easily attach a patch on that bug.
Comment on attachment 8573983 [details] [diff] [review]
1022816_v4.patch

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

Looks good with minor nits.
Would it be possible to add a test that checks the `if (oldAttributes == Const.INVALID_FILE_ATTRIBUTES) {` branch?

::: toolkit/components/osfile/modules/osfile_win_allthreads.jsm
@@ +290,5 @@
> +  },
> +  /**
> +   * The File Attributes of this file.
> +   *
> +   * @type {object}

Nit: That type information is not clear. Could you detail what's in the object?

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +1200,5 @@
>  
> +     /**
> +      * Helper used by both versions of setPermissions
> +      */
> +     function changeFileAttributes(winAttributes, oldDwAttrs) {

Since this doesn't actually change file attributes, just converts, I'd prefer if it were called something along the liens of `toFileAttributes`.
Attachment #8573983 - Flags: feedback+
Attachment #8573340 - Flags: feedback?(dteller)
(Assignee)

Comment 47

3 years ago
Created attachment 8576125 [details] [diff] [review]
1022816_v5.patch

Added 2 new xpcshell test cases.
Attachment #8573340 - Attachment is obsolete: true
Attachment #8573983 - Attachment is obsolete: true
Attachment #8576125 - Flags: review?(paolo.mozmail)
Attachment #8576125 - Flags: feedback?(dteller)
Comment on attachment 8576125 [details] [diff] [review]
1022816_v5.patch

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

I'm good with the patch, although I would even prefer with the two changes below.

::: toolkit/components/osfile/tests/xpcshell/test_osfile_win_async_setPermissions.js
@@ +96,5 @@
> +    }
> +    do_print("Caught the following exception" + e);
> +  } finally {
> +    do_check_true(isFileNotFound);
> +  }

The following should be a more concise version of your `try/finally`:

yield Assert.rejects(
  () => OS.File.setPermissions(...),
  e => e instanceof OS.File.Error && e.becauseNoSuchFile,
  "setPermissions failed as expected on a non-existant file path
)

@@ +119,5 @@
> +      }
> +      do_print("Caught the following exception" + e);
> +    } finally {
> +      do_check_true(isInvalidFileHandle);
> +    }

As above, I would prefer using `Assert.rejects`.
Attachment #8576125 - Flags: feedback?(dteller) → feedback+

Comment 49

3 years ago
Comment on attachment 8576125 [details] [diff] [review]
1022816_v5.patch

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

What David said, and:

::: toolkit/components/osfile/tests/xpcshell/test_osfile_win_async_setPermissions.js
@@ +83,5 @@
> +  }
> +});
> +
> +// Test application to Check setPermissions on a non-existant file path.
> +add_task(function* test_path_setPermissions() {

Please give unique names to test functions, as they're displayed in the log file.
Attachment #8576125 - Flags: review?(paolo.mozmail) → review+

Comment 50

3 years ago
(I've added dev-doc-needed to bug 984172 since the useful Assert.rejects function is still undocumented.)
(Assignee)

Comment 51

3 years ago
Created attachment 8576786 [details] [diff] [review]
1022816_v6.patch
Attachment #8576125 - Attachment is obsolete: true
Attachment #8576786 - Flags: review?(paolo.mozmail)

Updated

3 years ago
Attachment #8576786 - Flags: review?(paolo.mozmail) → review+

Comment 52

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/9790bffba1f3
https://hg.mozilla.org/mozilla-central/rev/9790bffba1f3
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment on attachment 8576786 [details] [diff] [review]
1022816_v6.patch

I uplifted this as a bustage fix for bug 1009465. Asking for retroactive approval.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Blocks bug 1009465 per the whiteboard status there.
[Describe test coverage new/current, TreeHerder]: Adds a test on its own, reliable test bustage in bug 1009465 without it.
[Risks and why]: No known regressions since it landed on m-c over a month ago.
[String/UUID change made/needed]: None
Flags: needinfo?(sledru)
Attachment #8576786 - Flags: approval-mozilla-release?
https://hg.mozilla.org/releases/mozilla-release/rev/8a2c933394da
status-firefox38: --- → fixed
Comment on attachment 8576786 [details] [diff] [review]
1022816_v6.patch

thanks for taking care of this.
Flags: needinfo?(sledru)
Attachment #8576786 - Flags: approval-mozilla-release? → approval-mozilla-release+
https://hg.mozilla.org/releases/mozilla-beta/rev/8a2c933394da
status-firefox38.0.5: --- → fixed
You need to log in before you can comment on or make changes to this bug.