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

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Zack Weinberg (:zwol)]

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 ;-)

Comment 1

[Mark Hammond [:markh] [:mhammond]]

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

Comment 2

[Zack Weinberg (:zwol)]

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.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

(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.

Comment 4

[Magnus Melin [:mkmelin]]

At the least, the readonly attribute should be set on the file when setting permissions like 0400. 
nsIFile SetPermissions already does supports this.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(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?)

Comment 6

[Mark Hammond [:markh] [:mhammond]]

(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)

Comment 7

[Zack Weinberg (:zwol)]

(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.

Comment 8

[Magnus Melin [:mkmelin]]

(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.

Comment 9

[Zack Weinberg (:zwol)]

(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

[Magnus Melin [:mkmelin]]

(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.

Comment 11

[Mark Hammond [:markh] [:mhammond]]

(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.

Comment 12

[:Paolo Amadini]

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.

Comment 13

[Ganesh Sahukari]

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

Comment 14

[Mark Hammond [:markh] [:mhammond]]

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

[:Paolo Amadini]

(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.

Comment 16

[:Paolo Amadini]

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

[:Paolo Amadini]

(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?

Comment 18

[Magnus Melin [:mkmelin]]

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.

Comment 19

[Mark Hammond [:markh] [:mhammond]]

(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

[:Paolo Amadini]

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: [ ... ]}).

Comment 21

[David Teller [:Yoric] - still alive but not very active]

I prefer a single `setPermissions`, but I'm willing to go either way.

Comment 22

[Mark Hammond [:markh] [:mhammond]]

(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

[:Paolo Amadini]

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

Oooh, that's nice.

Comment 24

[Ganesh Sahukari]

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

Comment 25

[Mark Hammond [:markh] [:mhammond]]

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

[:Paolo Amadini]

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!

Comment 27

[Ganesh Sahukari]

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

Comment 28

[Mark Hammond [:markh] [:mhammond]]

(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?

Comment 29

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 30

[Mark Hammond [:markh] [:mhammond]]

(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.

Comment 31

[Ganesh Sahukari]

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"

Comment 32

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 33

[Ganesh Sahukari]

Attachment: 1022816.patch

Comment 34

[Mark Hammond [:markh] [: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)

Comment 35

[:Paolo Amadini]

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 } },
];

Comment 36

[Ganesh Sahukari]

Attachment: 1022816_v2.patch

Comment 37

[:Paolo Amadini]

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"

Comment 38

[:Paolo Amadini]

(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.

Comment 39

[Ganesh Sahukari]

Attachment: 1022816_v3.patch

Comment 40

[:Paolo Amadini]

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(...)

Comment 41

[:Paolo Amadini]

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.

Comment 42

[Ganesh Sahukari]

Attachment: 1022816_v4.patch

Comment 43

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0335036d66f4

Comment 44

[Ganesh Sahukari]

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

[:Paolo Amadini]

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 46

[David Teller [:Yoric] - still alive but not very active]

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`.

Comment 47

[Ganesh Sahukari]

Attachment: 1022816_v5.patch

Added 2 new xpcshell test cases.

Comment 48

[David Teller [:Yoric] - still alive but not very active]

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`.

Comment 49

[:Paolo Amadini]

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.

Comment 50

[:Paolo Amadini]

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

Comment 51

[Ganesh Sahukari]

Attachment: 1022816_v6.patch

Comment 52

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/9790bffba1f3

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9790bffba1f3

Comment 54

[Ryan VanderMeulen [:RyanVM]]

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

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/8a2c933394da

Comment 56

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8576786 [details] [diff] [review]
1022816_v6.patch

thanks for taking care of this.

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8a2c933394da