992600 - [OS.File] Writing a typed array in a worker ignores .byteOffset.

Status: RESOLVED WONTFIX

(Toolkit Graveyard :: OS.File, defect)

Description

[Nils Maier [:nmaier]]

Attachment: test.osfile.offset.js

When calling OS.File.write*(typedArrayWithOffset) in a Worker thread, incorrect data is written, as the Worker version of write*() disregards |TypedArray.byteOffset|.
Using the async API on the main thread works as expected, however.

Consider the following buffer and typed array:
    var buf = (new TextEncoder()).encode("123helloworld").buffer;
    var arr = new Uint8Array(buf, 3);
|OS.File.writeAtomic(file, arr)| should write "helloworld" to the file.

However, calling |OS.File.writeAtomic(file, arr)| in a Worker will instead write "123hellowo", as |.byteOffset| is disregarded.

Calling |yield OS.File.writeAtomic(file, arr)| on the main thread does indeed write the expected "helloworld", so the async API is unaffected.

See the attachment for a full scratchpad test case.

Comment 1

[Nils Maier [:nmaier]]

Taking. Already got a working patch and some tests...

Comment 2

[Nils Maier [:nmaier]]

Attachment: Part 1 - Fix .byteOffset handling in OS.File (workers)

Alright, this at least passes locally on OSX and Windows.

When I wrote the tests, I wanted to avoid writing them twice, once with |yield|s for the main thread version, and another one without |yield|s for the worker. So instead I opted to create a more generic solution in |run_in_worker| and |add_dual_task|. Essentially you can run self-contained tasks in a worker now without duplicating code, provided you stick to the features the worker actually supports (|do_check*| and so on). The code will |.toSource()| the task function, pass it to the worker thread, and the worker will then do mini-Task.jsm, i.e. just eat all |yields| and post back the results. I also added test_worker.js to for some self-tests.

Comment 3

[Nils Maier [:nmaier]]

Attachment: Part 2 - Convert some existing test OS.File test do dual ones.

Since Part 1 puts |add_dual_task| in place, lets use that to convert most of the existing OS.File xpc-shell tests to it, so that the code gets actually tested using the async and worker APIs.

As a bonus, I fixed two instances where null-ish |options| threw in the thread, but not in the async API.

Comment 4

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

Comment on attachment 8405700 [details] [diff] [review]
Part 1 - Fix .byteOffset handling in OS.File (workers)

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

A long time ago, I decided to write essentially what you are doing with worker.js. It kind-of-worked, but never enough to be used by anybody outside of myself. I suggest that you do not pursue that part of the patch. It would be much more useful if you could work on improving support for workers in xpcshell.

::: toolkit/components/osfile/tests/xpcshell/worker.js
@@ +14,5 @@
> + * environment is somewhat compatible to a plain xpcshell-test environment, so
> + * that you can use |do_check_*|, |do_print|, |Task.Result|.
> + * Task.jsm support is only rudimentary and does not actually support promises
> + * at all! All it does is strip away |yield|s, which is OK for OS.File, but
> + * might not be OK for other code.

Too fragile for my tastes.

@@ +30,5 @@
> +    if (!worker) {
> +      worker = new ChromeWorker(WORKER_HELPER_URI);
> +    }
> +    worker.onmessage = function(evt) {
> +      if (evt.data.done) {

So that's when the test is complete? If so, please document it.

@@ +34,5 @@
> +      if (evt.data.done) {
> +        if (ok) {
> +          d.resolve(evt.data.result);
> +        }
> +        else {

} else {

@@ +39,5 @@
> +          d.reject(new Error("Tests did not pass"));
> +        }
> +        return;
> +      }
> +      if ("msg" in evt.data) {

So that's when we want to show something on stderr? If so, please document it.

Comment 5

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

Comment on attachment 8405701 [details] [diff] [review]
Part 2 - Convert some existing test OS.File test do dual ones.

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

As mentioned in the other patch – I'm a bit nervous that we will end up with something very ad hoc and that breaks in ways that can only be fixed by the original dev.

Comment 6

[Beth Rennie [:beth] (she/her)]

OSFIle is being replaced with IOUtils and PathUtils.