Closed Bug 890387 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: OS.File, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: Irving, Assigned: Irving)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attachment #771474 - Flags: review?(dteller)
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.
Attachment #771474 - Flags: review?(dteller) → review+
Summary: OS.File.open() options argument does not correctly handle some '0' option values → [OS.File] OS.File.open() options argument does not correctly handle some '0' option values
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
Attachment #771474 - Attachment is obsolete: true
Attachment #771486 - Flags: review+
Removing checkin-needed because of Try test failures on Linux
Keywords: checkin-needed
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.
Attachment #771486 - Attachment is obsolete: true
Attachment #773977 - Flags: review?(dteller)
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.
Attachment #773977 - Flags: review?(dteller) → review+
https://hg.mozilla.org/mozilla-central/rev/e80c4863db27
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.