890387 - [OS.File] OS.File.open() options argument does not correctly handle some '0' option values

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[:Irving Reid (No longer working on Firefox)]

Attachment: Allow zero values for many OS.File options

Zero is a legitimate value for many Windows file options, particularly winShare. Also legitimate, though rarely used, for some Unix operations.

One note on this patch: in osfile_unix_front.jsm, is it legitimate for calling code to ask that zero bytes be copied (i.e. options.nbytes == 0)? If so, I don't think the code would handle that case correctly even if we allowed the 0 value to pass through the argument check.

Comment 1

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

Comment on attachment 771474 [details] [diff] [review]
Allow zero values for many OS.File options

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

Thanks for the patch. It's good, with a few minor fixes.

::: toolkit/components/osfile/osfile_win_front.jsm
@@ +253,5 @@
>        * @throws {OS.File.Error} If the file could not be opened.
>        */
>       File.open = function Win_open(path, mode = {}, options = {}) {
> +       let share = "winShare" in options ? options.winShare : DEFAULT_SHARE;
> +       let security = "winSecurity" in options ? options.winSecurity : null;

This one is actually a pointer.

@@ +255,5 @@
>       File.open = function Win_open(path, mode = {}, options = {}) {
> +       let share = "winShare" in options ? options.winShare : DEFAULT_SHARE;
> +       let security = "winSecurity" in options ? options.winSecurity : null;
> +       let flags = "winFlags" in options ? options.winFlags : DEFAULT_FLAGS;
> +       let template = options.winTemplate ? options.winTemplate._fd : null;

This one is actually a pointer.

@@ +379,5 @@
>        * - {bool} ignoreExisting If |true|, do not fail if the
>        * directory already exists.
>        */
>       File.makeDir = function makeDir(path, options = {}) {
> +       let security = "winSecurity" in options ? options.winSecurity : null;

This one is actually a pointer.

Comment 2

[:Irving Reid (No longer working on Firefox)]

Attachment: Allow zero values for many OS.File options, updated with review comments

Carrying forward r=yoric

(In reply to David Rajchenbach Teller [:Yoric] <on workweek, will not follow bugzilla until July 1st> from comment #1)
> Review of attachment 771474 [details] [diff] [review]:
> ::: toolkit/components/osfile/osfile_win_front.jsm
> @@ +253,5 @@
> > +       let security = "winSecurity" in options ? options.winSecurity : null;
> 
> This one is actually a pointer.

Changed back to options.winSecurity || null

> @@ +255,5 @@
> > +       let template = options.winTemplate ? options.winTemplate._fd : null;
> 
> This one is actually a pointer.

All I changed here was the white space around the ternary ? :

> @@ +379,5 @@
> > +       let security = "winSecurity" in options ? options.winSecurity : null;
> 
> This one is actually a pointer.

Changed back to options.winSecurity || null

Comment 3

[:Irving Reid (No longer working on Firefox)]

Removing checkin-needed because of Try test failures on Linux

Comment 4

[:Irving Reid (No longer working on Firefox)]

Attachment: Use "!== undefined" tests instead of "field" in options

I was getting odd failures on Linux, which turned out to be because I was accidentally passing |undefined| as an option value. While it's a bug in the caller to do that, this version of the patch makes those failures less ugly.

Comment 5

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

Comment on attachment 773977 [details] [diff] [review]
Use "!== undefined" tests instead of "field" in options

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

Good for me, that's consistent with the behavior of default arguments.

Comment 6

[:Irving Reid (No longer working on Firefox)]

Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/e80c4863db27

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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