926691 - [OS.File] |file.write(buffer, {bytes: bytes}| does not work anymore.

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Nils Maier [:nmaier]]

Bug 865387 introduced the following change, causing |options.byteLength| to be always overriden by |buffer.byteLength|:

-    if (isTypedArray(buffer) && (!options || !("bytes" in options))) {
-      // Preserve the reference to |outExecutionDuration| option if it is
-      // passed.
+    if (isTypedArray(buffer)) {
+      // Preserve reference to option |outExecutionDuration|, if it is passed.
       options = clone(options, ["outExecutionDuration"]);
       options.bytes = buffer.byteLength;
     }

Bug 865387 comment 33.

Comment 1

[Nils Maier [:nmaier]]

Attachment: OS.File API must preverse |bytes| options to .readTo() and .write().

Alrighty, re-added the check, also re-added the "options is nullish" check and added a simple xpc-shell test.

Comment 2

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

Comment on attachment 816891 [details] [diff] [review]
OS.File API must preverse |bytes| options to .readTo() and .write().

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

r=me, but I believe that we should keep our files smaller :)

::: toolkit/components/osfile/tests/xpcshell/test_osfile_async_bytes.js
@@ +20,5 @@
> +    try {
> +      // 1. Test write, by supplying {bytes:} options smaller than the actual
> +      // buffer.
> +      yield file.write(new Uint8Array(1<<11), {bytes: 1<<10});
> +      do_check_eq((yield file.stat()).size, 1<<10);

Do you really need your files to be that large?
I know that it's going to do no harm under Unix, but I have no such confidence in Windows.

Comment 3

[Nils Maier [:nmaier]]

1024 bytes is considered large these days?

Comment 4

[Nils Maier [:nmaier]]

Attachment: OS.File API must preverse |bytes| options to .readTo() and .write(). r=yoric

- function*() ;)
- Replaced 1<<11 with 2048 and so on, to make it more clear what the sizes are.

Comment 5

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

For some reason, I had seen 1 << 20. My bad.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/ef3ac303c242

Comment 7

[Nils Maier [:nmaier]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #6)
> https://hg.mozilla.org/integration/fx-team/rev/ef3ac303c242

Sorry, this will cause failures on linux. Was too eager with the checkin-needed after receiving try greens for android, OSX and Windows...

Comment 8

[Carsten Book [:Tomcat]]

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