1001849 - OS.File APIs for setting file access permissions

Status: VERIFIED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Zack Weinberg (:zwol)]

Attachment: patch v1: Add mode-setting to OS.File

+++ This bug was initially created as a clone of Bug #961080 +++

from bug 961080 comment 19:

> I think it's better to design this API in another dependent bug in the
> OS.File component - there might have already been design or there might be
> work underway that relates to setting Unix permissions.

I should have done this ages ago; apologies.

> (We can move the umask patches there as they are a prerequisite, or land
> them in yet another bug that is a dependency of the others.)

The umask patches are now bug 1001842.

> My take is that this call could be a platform-specific
> "unixSetPermissions(path, permissions, options)" call, that respects umask
> by default and has an option like "{ ignoreUmask: true }".
> 
> I don't see the need of a cross-platform makeAccessibleByOthers because
> downloads are the only use case that comes to my mind, and we typically
> avoid generalizing an API when it has only one consumer (though we might do
> this in the future if more consumers need to the same thing).

As discussed on dev-platform, I think this is barking up entirely the wrong tree, and the API in the patch is preferable (possibly modulo removal of the 'executable' option until a need emerges).

My position is very simple:  *Whether or not* other consumers appear, the operation "do whatever is necessary on this platform to give this file the access permissions that it would have received if we had created it in its current location in the most natural way" belongs in OS.File and not in its consumers, because it is security-critical, finicky to get right, and requires detailed understanding of platform conventions to get right.  Moreover it should not be lumped into a generic setPermissions() call, which programmers will expect to do exactly as it is told rather than performing DWIMmery.

Comment 2

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

Comment on attachment 8413204 [details] [diff] [review]
patch v1: Add mode-setting to OS.File

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

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +540,5 @@
> +   */
> +  setPerms: function setPerms(mode) {
> +    return Scheduler.post("File_prototype_setPerms",
> +                          [this._fdmsg, mode], this);
> +  },

I'd prefer something along the lines of

setPermissions: function(options) {
  // ...
}

where options may contain fields unixMode, winMode and possibly more stuff (e.g. makeAccessibleByOthers).

@@ +544,5 @@
> +  },
> +
> +  /**
> +   * Make the file accessible by others.  On Unix, honors the umask.
> +   * FIXME: On Windows, currently does nothing.

Well, what should it do on Windows?

@@ +550,5 @@
> +   * @param {bool} exec   If true, the file should be executable.
> +   */
> +  makeAccessibleByOthers: function makeAccessibleByOthers(exec) {
> +    return Scheduler.post("File_prototype_makeAccessibleByOthers",
> +                          [this._fdmsg, exec], this);

This doesn't match the impedence of the rest of OS.File. If you want a magic call makeAccessibleByOthers, this should rather be an option for `setPermissions`.

Comment 3

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

Note: if you want a review, next time, please ask for it :)

Comment 4

[Zack Weinberg (:zwol)]

Attachment: 1001849-add-mode-setting-to-OS.File.diff

Long time coming, sorry about that, but here is a new proposed patch.  I'll paste the docstring for the revised API here, since that's the most interesting part of the change:

  /**
   * Set the file's access permissions.  Without any options, the
   * permissions are "normalized" to what they would have been if the
   * file had been created in its current directory in the "most
   * typical" fashion for the operating system.  (For instance, mode
   * 0666 & ~umask on POSIX-compliant systems.)  Note that it may not
   * be possible to get the permissions exactly right.
   * FIXME: Currently unimplemented on Windows.
   *
   * @param {*=} options
   * - {number} unixMode    If present, the POSIX file mode is set to exactly
   *                        this value (unless |honorUmask| is also present).
   *                        Throws an exception if present on Windows.
   * - {boolean} honorUmask If |true|, any |unixMode| value is modified by the
   *                        process umask, as open() would have done.
   */
  setPermissions: function setPermissions(options = {}) {

For clarity, calling this with no arguments is equivalent to calling it with
  { unixMode: 0o666, honorUmask: true } on Unix, and (presently) a silent nop
on Windows.  If any options are provided on Windows, it throws an error.  I will file a new bug for "figure out what this should do on Windows and implement it" if this API is approved.

Comment 5

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

(In reply to Zack Weinberg (:zwol) from comment #4)
>   /**
>    * Set the file's access permissions.  Without any options, the
>    * permissions are "normalized" to what they would have been if the
>    * file had been created in its current directory in the "most
>    * typical" fashion for the operating system.  (For instance, mode
>    * 0666 & ~umask on POSIX-compliant systems.)

Ok, fair enough.

>  Note that it may not
>    * be possible to get the permissions exactly right.

Can you elaborate?

>    * FIXME: Currently unimplemented on Windows.

Ok, we can start with this.

Comment 6

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

Comment on attachment 8436604 [details] [diff] [review]
1001849-add-mode-setting-to-OS.File.diff

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

I believe that we're almost there.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +823,5 @@
> +   * - {number} unixMode    If present, the POSIX file mode is set to exactly
> +   *                        this value (unless |honorUmask| is also present).
> +   *                        Throws an exception if present on Windows.
> +   * - {boolean} honorUmask If |true|, any |unixMode| value is modified by the
> +   *                        process umask, as open() would have done.

Since `honorUmask` doesn't make sense under Windows, let's also prefix it by `unix`.

@@ +947,5 @@
> + *                        process umask, as open() would have done.
> + */
> +File.setPermissions = function setPermissions(path, options = {}) {
> +  return Scheduler.post("setPermissions",
> +                        [Type.path.toMsg(path), options], this);

This `this` doesn't make sense.
(nor does the one in the method above, I grant you)

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +238,5 @@
> +      * Set the file's access permission bits. Not implemented for Windows.
> +      */
> +     File.prototype.setPermissions = function setPermissions(options = {}) {
> +       if ("unixMode" in options || "honorUmask" in options) {
> +         throw new Error("setPermissions: not implemented for Windows");

Please don't do that. By design of OS.File, `unix-` prefixed options are simply ignored under Windows. In other words, this method should be a NOOP.

@@ +966,5 @@
> +      * Set the file's access permission bits. Not implemented for Windows.
> +      */
> +     File.setPermissions = function setPermissions(path, options = {}) {
> +       if ("unixMode" in options || "honorUmask" in options) {
> +         throw new Error("setPermissions: not implemented for Windows");

As above.

::: toolkit/components/osfile/tests/xpcshell/test_osfile_async_setPerms.js
@@ +1,1 @@
> +"use strict";

Please add the public domain header.

@@ +1,4 @@
> +"use strict";
> +
> +Components.utils.import("resource://gre/modules/osfile.jsm");
> +Components.utils.import("resource://gre/modules/Task.jsm");

I believe that they are already imported in head.js.

@@ +15,5 @@
> +  do_test_pending();
> +  run_next_test();
> +}
> +
> +function format_mode(mode) {

A few comments would be useful here.

@@ +41,5 @@
> +  return mode & ~_umask;
> +}
> +
> +// Test application to paths.
> +add_task(function test_nonproto() {

`function*`, these days

::: toolkit/components/osfile/tests/xpcshell/xpcshell.ini
@@ +33,5 @@
>  [test_queue.js]
> +[test_constants.js]
> +
> +[test_osfile_async_setPerms.js]
> +skip-if = os == 'win'

Note: I wonder if this works under B2G/Android. Have you tested?

Comment 7

[Zack Weinberg (:zwol)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #6)
> 
> I believe that we're almost there.

Excellent.

> ::: toolkit/components/osfile/modules/osfile_async_front.jsm
> @@ +823,5 @@
> > +   * - {number} unixMode    If present, the POSIX file mode is set to exactly
> > +   *                        this value (unless |honorUmask| is also present).
> > +   *                        Throws an exception if present on Windows.
> > +   * - {boolean} honorUmask If |true|, any |unixMode| value is modified by the
> > +   *                        process umask, as open() would have done.
> 
> Since `honorUmask` doesn't make sense under Windows, let's also prefix it by
> `unix`.

Ok.

> > +  return Scheduler.post("setPermissions",
> > +                        [Type.path.toMsg(path), options], this);
> 
> This `this` doesn't make sense.
> (nor does the one in the method above, I grant you)

Removed.  I wrote a lot of this by copying and modifying nearby code and I don't really understand how it works, so I'm not going to do any unrelated cleanups.

> ::: toolkit/components/osfile/modules/osfile_win_front.jsm
> > +       if ("unixMode" in options || "honorUmask" in options) {
> > +         throw new Error("setPermissions: not implemented for Windows");
> 
> Please don't do that. By design of OS.File, `unix-` prefixed options are
> simply ignored under Windows. In other words, this method should be a NOOP.

That makes me a little nervous -- if someone does use this with a specific mode they probably have a concrete reason for it and would prefer not to have their security constraints silently ignored on a major platform -- but I suppose it's better addressed by making it do something meaningful on Windows. I'll make them both do nothing.

> ::: toolkit/components/osfile/tests/xpcshell/test_osfile_async_setPerms.js
> @@ +1,1 @@
> > +"use strict";
> 
> Please add the public domain header.

Done.

> @@ +1,4 @@
> > +"use strict";
> > +
> > +Components.utils.import("resource://gre/modules/osfile.jsm");
> > +Components.utils.import("resource://gre/modules/Task.jsm");
> 
> I believe that they are already imported in head.js.
> 
> @@ +15,5 @@
> > +  do_test_pending();
> > +  run_next_test();
> > +}
> > +
> > +function format_mode(mode) {
> 
> A few comments would be useful here.

How's this?

/**
 * Helper function for test logging: prints a POSIX file permission mode as an
 * octal number, with a leading '0' per C (not JS) convention.  When the
 * numeric value is 0777 or lower, it is padded on the left with zeroes to
 * four digits wide.
 * Sample outputs:  0022, 0644, 04755.
 */

> @@ +41,5 @@
> > +  return mode & ~_umask;
> > +}
> > +
> > +// Test application to paths.
> > +add_task(function test_nonproto() {
> 
> `function*`, these days

Fixed.  (What does that mean?)

> Note: I wonder if this works under B2G/Android. Have you tested?

https://tbpl.mozilla.org/?tree=Try&rev=60d29c7a6a91 -- oddly, xpcshell orange everywhere *except* B2G/Android (and Windows), which probably does indicate a bug in this patch, although it seems to have been *triggered* by the followup patch in bug 961080.  test_osfile_async_setPerms.js itself is green on all X targets. I will investigate.

Comment 8

[Zack Weinberg (:zwol)]

The xpcshell oranges were caused by a silly mistake in the followup patch.  New try push here:

https://tbpl.mozilla.org/?tree=Try&rev=b7bd88fd50b6

New patch shortly.

Comment 9

[Zack Weinberg (:zwol)]

Attachment: patch v3

I believe this makes all the requested changes.  I have also modified the test to use "0o123"-style octal constants; this syntax does not appear to work in .jsm files, so there are still some "decimal /* octal */" instances there.

Comment 10

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

Comment on attachment 8436983 [details] [diff] [review]
patch v3

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

r+ with trivial changes.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +816,5 @@
> +   * file had been created in its current directory in the "most
> +   * typical" fashion for the operating system.  (For instance, mode
> +   * 0666 & ~umask on POSIX-compliant systems.)  Note that it may not
> +   * be possible to get the permissions exactly right.
> +   * FIXME: Currently unimplemented on Windows.

Nit: I'd replace that last line with: « In the current implementation, the "typical" fashion under Unix is 0666 & ~umask, and this method does nothing under Windows. »

::: toolkit/components/osfile/modules/osfile_unix_front.jsm
@@ +1151,5 @@
>       };
>  
> +     /**
> +      * Helper used by both versions of makeAccessibleByOthers.
> +      * FIXME: Should we detect directories and default to 0777?

Nit: Rather than this `FIXME`, I'd prefer if you opened a bug to discuss the question.

@@ +1156,5 @@
> +      */
> +     function defaultMode(options) {
> +       let mode = 438; /* 0666 */
> +       let unixHonorUmask = true;
> +       if (options.unixMode !== undefined) {

Nit: I would prefer a bit if you could do « if ("unixMode" in options) ». Same thing below.

@@ +1161,5 @@
> +         unixHonorUmask = false;
> +         mode = options.unixMode;
> +       }
> +       if (options.unixHonorUmask !== undefined) {
> +         unixHonorUmask = mode.unixHonorUmask;

That should be `options.unixHonorMask`, shouldn't it?

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +236,5 @@
>  
>       /**
> +      * Set the file's access permission bits. Not implemented for Windows.
> +      */
> +     File.prototype.setPermissions = function setPermissions(options = {}) {

Nit: Can you add a small comment inside along the lines of
  /* Do nothing */
Same below.

::: toolkit/components/osfile/tests/xpcshell/test_osfile_async_setPerms.js
@@ +11,5 @@
> + * the test on Windows.
> + */
> +
> +function run_test() {
> +  do_test_pending();

You don't need to call `do_test_pending`.

@@ +122,5 @@
> +    yield OS.File.remove(path);
> +  }
> +});
> +
> +add_task(do_test_finished);

You don't need this.

Comment 11

[Zack Weinberg (:zwol)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #10)
> 
> Nit: I'd replace that last line with: « In the current implementation, the
> "typical" fashion under Unix is 0666 & ~umask, and this method does nothing
> under Windows. »

OK.  Filed bug 1022816 for the Windows implementation.

> > +      * FIXME: Should we detect directories and default to 0777?
> 
> Nit: Rather than this `FIXME`, I'd prefer if you opened a bug to discuss the
> question.

Filed bug 1022820.

> > +       if (options.unixMode !== undefined) {
> 
> Nit: I would prefer a bit if you could do « if ("unixMode" in options) ».
> Same thing below.

I tried that and I got TypeErrors ("invalid RHS of 'in'" or words to that effect) ... which makes no sense considering that all callers default the argument to {}, but there it is.  Any debugging advice you might have would be appreciated.

> That should be `options.unixHonorMask`, shouldn't it?

The 'u' in 'umask' is for 'user'.

> Nit: Can you add a small comment inside along the lines of
>   /* Do nothing */
> Same below.

OK.

> You don't need to call `do_test_pending`.
> > +add_task(do_test_finished);
> You don't need this.

That was cloned from the setDates test ... I'll experiment with taking it out.

Note that my development box just melted down again (btrfs turns out to have been a Bad Idea) so I might not get to the revisions today.

Comment 12

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

(In reply to Zack Weinberg (:zwol) from comment #11)
> > > +       if (options.unixMode !== undefined) {
> > 
> > Nit: I would prefer a bit if you could do « if ("unixMode" in options) ».
> > Same thing below.
> 
> I tried that and I got TypeErrors ("invalid RHS of 'in'" or words to that
> effect) ... which makes no sense considering that all callers default the
> argument to {}, but there it is.  Any debugging advice you might have would
> be appreciated.

Weird. That happens typically when `options` is `null` or `typeof options != "string"`.

> > That should be `options.unixHonorMask`, shouldn't it?
> 
> The 'u' in 'umask' is for 'user'.

Typo. I meant that you wrote `mode` instead of `options`.

> That was cloned from the setDates test ... I'll experiment with taking it
> out.
> 
> Note that my development box just melted down again (btrfs turns out to have
> been a Bad Idea) so I might not get to the revisions today.

My sympathies.

Comment 13

[Zack Weinberg (:zwol)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #12)
> (In reply to Zack Weinberg (:zwol) from comment #11)
> > > > +       if (options.unixMode !== undefined) {
> > > 
> > > Nit: I would prefer a bit if you could do « if ("unixMode" in options) ».
> > > Same thing below.
> > 
> > I tried that and I got TypeErrors ("invalid RHS of 'in'" or words to that
> > effect) ... which makes no sense considering that all callers default the
> > argument to {}, but there it is.  Any debugging advice you might have would
> > be appreciated.
> 
> Weird. That happens typically when `options` is `null` or `typeof options !=
> "string"`.

I changed it back, and now it works, possibly because of ...

> > > That should be `options.unixHonorMask`, shouldn't it?
> > 
> > The 'u' in 'umask' is for 'user'.
> 
> Typo. I meant that you wrote `mode` instead of `options`.

... this.  Thanks for catching that!  It revealed that the tests weren't testing some of the things they were supposed to test.

> > That was cloned from the setDates test ... I'll experiment with taking it
> > out.

This seems to be enough by itself:

function run_test() {
  run_next_test();
}

... but that cannot be removed.  I think that's what you meant?

Comment 14

[Zack Weinberg (:zwol)]

Attachment: patch v4

With all the requested changes and the knock-on test corrections I mentioned.

Given the schedule I am going to go ahead and push this to inbound.

Comment 15

[Zack Weinberg (:zwol)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/46f584afbed5

Comment 16

[Ed Morley [:emorley]]

Backed out for test_osfile_async_setPerms.js failures on (at least so far) Android:
https://tbpl.mozilla.org/php/getParsedLog.php?id=41442817&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=41443014&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8a0e868c47c2

Comment 17

[Zack Weinberg (:zwol)]

Failure overview: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=46f584afbed5

Frustratingly, this did *not* happen on the try server (see comment 8) which appears to be because "-b d -p all -u xpcshell -t none" *doesn't run xpcshell on Android.*  W. T. F.

Anyhow, here's the proximate cause of failure (https://tbpl.mozilla.org/php/getParsedLog.php?id=41443014&tree=Mozilla-Inbound#error1)

07:22:07     INFO -  OS Agent Received message {"fun":"setPermissions","args":[{"string":"/mnt/sdcard/tests/xpcshell/tmp/test_osfile_async_setPerms_nonproto.tmp"},{"unixMode":2559}],"id":3}
07:22:07     INFO -  OS Agent Calling method setPermissions
07:22:07     INFO -  OS Attempting to declare FFI  chmod
07:22:07     INFO -  OS Function chmod declared
07:22:07     INFO -  OS Controller Message posted
07:22:07     INFO -  OS Agent Error while calling agent method Unix error 1 during operation setPermissions on file /mnt/sdcard/tests/xpcshell/tmp/test_osfile_async_setPerms_nonproto.tmp (Operation not permitted) 

Given that path name, I need to ask y'all whether that's a FAT32 filesystem on /mnt/sdcard, because I happen to know that Linux (the kernel) returns EPERM from chmod() when the filesystem doesn't support POSIX permission bits, and that's the only reason I can think of we could be getting that error in this context.

Comment 18

[Ed Morley [:emorley]]

(In reply to Zack Weinberg (:zwol) from comment #17)
> Failure overview:
> https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=46f584afbed5
> 
> Frustratingly, this did *not* happen on the try server (see comment 8) which
> appears to be because "-b d -p all -u xpcshell -t none" *doesn't run
> xpcshell on Android.*  W. T. F.

On Android, xpcshell is currently only run on opt runs (bug 866795 and bug 940068 are still WIPs; only some suites are enabled), so you need to request opt builds too.

> Anyhow, here's the proximate cause of failure
> (https://tbpl.mozilla.org/php/getParsedLog.php?id=41443014&tree=Mozilla-
> Inbound#error1)
> 
> 07:22:07     INFO -  OS Agent Received message
> {"fun":"setPermissions","args":[{"string":"/mnt/sdcard/tests/xpcshell/tmp/
> test_osfile_async_setPerms_nonproto.tmp"},{"unixMode":2559}],"id":3}
> 07:22:07     INFO -  OS Agent Calling method setPermissions
> 07:22:07     INFO -  OS Attempting to declare FFI  chmod
> 07:22:07     INFO -  OS Function chmod declared
> 07:22:07     INFO -  OS Controller Message posted
> 07:22:07     INFO -  OS Agent Error while calling agent method Unix error 1
> during operation setPermissions on file
> /mnt/sdcard/tests/xpcshell/tmp/test_osfile_async_setPerms_nonproto.tmp
> (Operation not permitted) 
> 
> Given that path name, I need to ask y'all whether that's a FAT32 filesystem
> on /mnt/sdcard, because I happen to know that Linux (the kernel) returns
> EPERM from chmod() when the filesystem doesn't support POSIX permission
> bits, and that's the only reason I can think of we could be getting that
> error in this context.

Not sure; best bet is to ask in #releng :-)

Comment 19

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

(In reply to Zack Weinberg (:zwol) from comment #17)
> Given that path name, I need to ask y'all whether that's a FAT32 filesystem
> on /mnt/sdcard, because I happen to know that Linux (the kernel) returns
> EPERM from chmod() when the filesystem doesn't support POSIX permission
> bits, and that's the only reason I can think of we could be getting that
> error in this context.

Good point. Could you document in the high-level API that this call can fail if applied on a file system that doesn't support permission?

Comment 20

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

Also, we will need to either add a `OS.Error.prototype.becauseSomething` in osfile_{unix, win}_allthreads.jsm to handle EPERM or add EPERM to `becauseAccessDenied`.

Comment 21

[Zack Weinberg (:zwol)]

(In reply to Ed Morley [:edmorley UTC+0] from comment #18)
> (In reply to Zack Weinberg (:zwol) from comment #17)
> > I happen to know that Linux (the kernel) returns
> > EPERM from chmod() when the filesystem doesn't support POSIX permission
> > bits, and that's the only reason I can think of we could be getting that
> > error in this context.
> 
> Not sure; best bet is to ask in #releng :-)

Discussed on #releng and #ateam and indeed, this is the problem.  gbrown ran the following experiment for me on one of the Android testers:

# cd /mnt/sdcard/tests/xpcshell/tmp
# ls
# echo . > ptest.dat
# ls -l ptest.dat
----rwxr-x system   sdcard_rw        2 2014-06-20 13:04 ptest.dat
# chmod 644 ptest.dat
# ls -l ptest.dat
----rwxr-x system   sdcard_rw        2 2014-06-20 13:04 ptest.dat
# chmod 600 ptest.dat
# ls -l ptest.dat
----rwxr-x system   sdcard_rw        2 2014-06-20 13:04 ptest.dat
# chmod 777 ptest.dat
# ls -l ptest.dat
----rwxr-x system   sdcard_rw        2 2014-06-20 13:04 ptest.dat
# chmod 4755 ptest.dat
Unable to chmod ptest.dat: Operation not permitted
# ls -l ptest.dat
----rwxr-x system   sdcard_rw        2 2014-06-20 13:04 ptest.dat
# rm ptest.dat

You can see that only 'chmod 4755' gets an overt failure notification, but none of the requested permission changes have any effect.

As such, and since time is of the essence here if we want to get bug 961080 into 31ESR, I have simply marked this test to be skipped on Android, and I'm going to go ahead and land it again.

(In reply to David Rajchenbach Teller [:Yoric] from comment #19)
> Could you document in the high-level API that this call can fail
> if applied on a file system that doesn't support permission?

I have added this text to the comments above both versions of the function in osfile_async_front.jsm and osfile_unix_front.jsm:

   * This operation is likely to fail if applied to a file that was
   * not created by the currently running program (more precisely,
   * if it was created by a program running under a different OS-level
   * user account).  It may also fail, or silently do nothing, if the
   * filesystem containing the file does not support access permissions.

(In reply to David Rajchenbach Teller [:Yoric] from comment #20)
> Also, we will need to either add a `OS.Error.prototype.becauseSomething` in
> osfile_{unix, win}_allthreads.jsm to handle EPERM or add EPERM to
> `becauseAccessDenied`.

Not required for 961080, so I filed follow-up bug 1028366.

Comment 22

[Zack Weinberg (:zwol)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5714e1eafca
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6426f08a828

(note to sheriffs: if you need to back out a5714e1eafca you must also back out c6426f08a828)

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a5714e1eafca

Comment 24

[Zack Weinberg (:zwol)]

Comment on attachment 8437385 [details] [diff] [review]
patch v4

Approval Request Comment
[Feature/regressing bug #]:
Regression in new download manager (bug 858234)

[User impact if declined]:

On POSIXy operating systems, downloaded files will continue to be accessible only to the user who downloaded them, even if user-global configuration (the 'umask') specifies otherwise.  Per discussion in bug 961080, that is a serious regression in some enterprise installations, where users regularly download
files into shared directories, and expect them to be accessible by others without further action.  One installation refused to upgrade past Firefox 25 because of this regression.

To fix the regression, both this and bug 961080 must be uplifted.

Due to the impact on enterprise users, I am requesting uplift to -beta so that 31ESR will not regress relative to 24ESR.  My apologies for cutting it so close.

[Describe test coverage new/current, TBPL]:

The patch in this bug has comprehensive xpcshell tests on all platforms except Windows and Android.  The new API added by this patch is a stub on Windows (see bug 1022816) which is fine because the regression only affects non-Windows platforms.  Testing on Android is disabled because our Android test workers are configured in a way that makes the test fail spuriously; this is also fine, because the way POSIX file permissions are used on Android is completely different from the way they are used on multi-seat shared-filesystem Unix installations, so it's very unlikely that anyone was affected by the regression on Android, even though it technically does apply there.

The patch in bug 961080 has no automated tests, but one of the affected users reports that it works for him.

[Risks and why]: 

Low risk.  This patch, by itself, adds a small amount of code which is not used for anything.  See bug 961080 for analysis of consequences of using the code for something.

[String/UUID change made/needed]: None

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8437385 [details] [diff] [review]
patch v4

Taking it because the next release is an ESR and it had coverage for a week in m-c. However, next time, please propose it sooner. thanks!

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/61e420e597b0
https://hg.mozilla.org/releases/mozilla-beta/rev/aae3600e0e64

Comment 27

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

My understanding is that this bug goes hand in hand with bug 961080. Bug 961080 describes how to test it manually (has no automated tests), while the current bug is covered by "comprehensive xpcshell tests on all platforms except Windows and Android" according to Zack (apparently this is OK since extra work is pending on Windows, and Android should not be affected). 

Given all of the above, and since the bug is marked as "verifyme", could you let us know what exactly is required from the Manual QA team for verifying this fix?

Comment 28

[Zack Weinberg (:zwol)]

I suppose one could manually verify this patch in isolation by typing OS.File operations into the chrome-JS scratchpad (if that still exists?) but I don't think it's worth the trouble.  I'll let you make the final call, but I think the verification y'all already did on bug 961080 should be enough to call this piece verified as well.

Comment 29

[Bogdan Maris, Desktop QA]

(In reply to Zack Weinberg (:zwol) from comment #28)
> I suppose one could manually verify this patch in isolation by typing
> OS.File operations into the chrome-JS scratchpad (if that still exists?) but
> I don't think it's worth the trouble.  I'll let you make the final call, but
> I think the verification y'all already did on bug 961080 should be enough to
> call this piece verified as well.

Verified by the verification done on bug 961080.