928239 - [OS.File] Unix non-splice fallback (pump_userland) is broken

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Nils Maier [:nmaier]]

Will throw "Unexpected exception options is undefined", as the |read()/_read()| calls do not specify any options, but |_read()| actually will check options since bug 865387, but pump_userland fails to provide an |options| argument.

Comment 1

[Nils Maier [:nmaier]]

Attachment: Fix pump_userland fallback in OS.File.copy.

Not sure what |options| to |read()/write()| should actually be. Settled for |read(..., options)| and |write(..., {})|.

The test might not actually test the code path if the system |copyfile| API. This is OK (expect that it wastes some time).

Comment 2

[Nils Maier [:nmaier]]

Green try: https://tbpl.mozilla.org/?tree=Try&rev=8b06fd6b37f1

Comment 3

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

Comment on attachment 818842 [details] [diff] [review]
Fix pump_userland fallback in OS.File.copy.

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

Wouldn't it be simpler to just add default values for options in |_read|, etc.?

::: toolkit/components/osfile/modules/osfile_unix_front.jsm
@@ +551,5 @@
>             } else {
>               dest = File.open(destPath, {trunc:true});
>             }
> +           if (options.unixUserland) {
> +             result = pump_userland(source, dest, options);

Is that for testing purposes?

Comment 4

[Nils Maier [:nmaier]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> Comment on attachment 818842 [details] [diff] [review]
> Fix pump_userland fallback in OS.File.copy.
> 
> Review of attachment 818842 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Wouldn't it be simpler to just add default values for options in |_read|,
> etc.?

I thought you (or whoever wrote _read/_write) had a reason not to add default values. But yeah, default parameters would be fine, I guess.

> ::: toolkit/components/osfile/modules/osfile_unix_front.jsm
> @@ +551,5 @@
> >             } else {
> >               dest = File.open(destPath, {trunc:true});
> >             }
> > +           if (options.unixUserland) {
> > +             result = pump_userland(source, dest, options);
> 
> Is that for testing purposes?

Well, yes and no... I added it for testing purposes, but the API lets you control most low-level stuff anyway, so I'm not sure if some people might find it useful for other purposes as well.

Comment 5

[Nils Maier [:nmaier]]

Attachment: Fix pump_userland fallback in OS.File.copy.

Using default values now, and added @option unixUserland comment.

Comment 6

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

Comment on attachment 818996 [details] [diff] [review]
Fix pump_userland fallback in OS.File.copy.

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

WFM, but even better if you add the following stuff.

::: toolkit/components/osfile/tests/xpcshell/test_osfile_async_copy_unix.js
@@ +23,5 @@
> +      yield file.write(new Uint8Array(2048), {bytes: 1024});
> +    } finally {
> +      yield file.close();
> +    }
> +    yield OS.File.copy(source, dest);

It might be a good idea to check that copying creates a file with the same data.
(this will also let us get rid of the copy test in test_osfile_async.xul)

@@ +54,5 @@
> +      yield file.write(new Uint8Array(2048), {bytes: 1024});
> +    } finally {
> +      yield file.close();
> +    }
> +    yield OS.File.copy(source, dest, {unixUserland: true});

Same here.

Comment 7

[Nils Maier [:nmaier]]

Attachment: Fix pump_userland fallback in OS.File.copy + move copy test.

How about this version then? I moved the copy/move test into the xpcshell test.

Comment 8

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

Comment on attachment 819046 [details] [diff] [review]
Fix pump_userland fallback in OS.File.copy + move copy test.

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

Looks good, thanks.

::: toolkit/components/osfile/tests/xpcshell/test_osfile_async_copy.js
@@ +47,5 @@
> +        promise.resolve(result);
> +      }
> +  });
> +  return promise.promise;
> +};

Note for self: we'll probably want to move this to head.js at some point.

@@ +63,5 @@
> +let reference_compare_files = function reference_compare_files(a, b) {
> +  let a_contents = yield reference_fetch_file(a);
> +  let b_contents = yield reference_fetch_file(b);
> +  // Not using do_check_eq to avoid dumping the whole file to the log.
> +  do_check_true(a_contents === b_contents);

Triple equals? I guess this works because we have strings, but could you please add a comment to that effect?

@@ +69,5 @@
> +
> +/**
> + * Test to ensure that OS.File.copy works.
> + */
> +function* test_copymove(options = {}) {

Sorry, the JS team just decided to deactivate |function*| until some bugs have been fixed. You'll need to return to |function|.

Comment 9

[Nils Maier [:nmaier]]

Attachment: Fix pump_userland fallback in OS.File.copy. r=yoric

Nits picked. Carrying over r=yoric.

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec99b2269e95

Comment 11

[Carsten Book [:Tomcat]]

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