1023685 - [OS.File] writeAtomic does not support writing encoded zero-byte strings

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Alex Vincent [:WeirdAl]]

Attachment: testcase as patch

TypeError: C pointer missing bytes indication.

Comment 1

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

This should be easy to fix. Marking as good first bug.

The problem is here: http://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_shared_front.jsm?from=osfile_shared_front.jsm#92,126

On line 126, we write `options.bytes || undefined` as if it meant "if `options` contains a field `bytes`, then use the value of `options.bytes`, otherwise use `undefined`". However, when `options.bytes` is 0, this is not the case and we end up using `undefined` instead of 0.

So, the only thing to do here is to change this line so as to use:
- `options.bytes` if `options` contains a field `bytes`;
- `undefined` if `options` does not contain a field `bytes`.

The right way to check if `options` contains a field `bytes` is
  "bytes" in options

Comment 2

[andy]

Attachment: patch for the bug

Checking whether the property exists in the object is unnecessary, as in JS `{}.blah` returns `undefined` anyway.

Comment 3

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

Comment on attachment 8446739 [details] [diff] [review]
patch for the bug

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

We are in strict mode, so this will cause a warning.

Comment 4

[andy]

Why is that? It works fine in this JSFiddle: http://jsfiddle.net/VGn62/, without any warnings.

    "use strict";
    console.log({}.nonExistantProperty);

(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> Comment on attachment 8446739 [details] [diff] [review]
> patch for the bug
> 
> Review of attachment 8446739 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We are in strict mode, so this will cause a warning.

Comment 5

[Manish Goregaokar [:manishearth]]

"use strict" doesn't necessarily mean anything when used in web code; the browser doesn't have to actually apply the strict mode rules to it. Firefox browser-level code does, though :)

Perhaps hasOwnProperty[1] will help here?

 [1]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/hasOwnProperty

Comment 6

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

Are you planning to continue working on this bug, andy?

Comment 7

[andy]

Attachment: (better) patch for the bug

Improved patch

Comment 8

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

Comment on attachment 8459352 [details] [diff] [review]
(better) patch for the bug

This patch is empty. Are you sure you didn't forget to call `hg qref`?

Comment 9

[andy]

Attachment: (fixed again) patch for the bug

Oops. Fixed, sorry about that (and for the very, very late responses; I've been travelling a lot recently and haven't had much time to organize everything).

Comment 10

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

Comment on attachment 8459513 [details] [diff] [review]
(fixed again) patch for the bug

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

This looks good.

I'm waiting for a few things before accepting this patch:
- you should change the commit message to something along the lines of "Bug 1023685 - OS.File.writeAtomic does not support writing encoded zero-byte strings;r=yoric";
- I would like a test to ensure that the problem is solved.

Apparently, we do not have tests that test only OS.File.writeAtomic. It would be great if you could add such a test in directory toolkit/components/osfile/tests/xpcshell. Take a look at the other tests in the directory and don't hesitate to ask if you have any question.

Comment 11

[Alex Vincent [:WeirdAl]]

Regarding the test:  would it be adequate to combine the testcase as patch that I submitted in comment 0?  :)

Comment 12

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

Good point. WeirdAl's test is perfectly fine for this purpose. Andy, can you make sure that WeirdAl's test passes with your patch? You will need to import his patch on top of yours (or below yours, that shouldn't change anything).

To launch his test

./mach xpcshell-test toolkit/components/osfile/tests/xpcshell/test_osfile_writeAtomic_zerobytes.js

We can either land the tests and the fix as two distinct patches or as a single one, I have no preference (if WeirdAl agrees to lose ownership of his patch, as he mentioned over IRC).

Comment 13

[Alex Vincent [:WeirdAl]]

Do what you will with the testcase patch; ownership isn't an issue for me.  I have no preference either; the only reason to really combine the two patches might be because I didn't include a commit comment on mine.

But I would suggest running all the tests in the directory, *not* just mine.  (Run mine separately to make sure it does run, of course - that the patch applied cleanly, and that the test passes.)

./mach xpcshell-test toolkit/components/osfile/tests/xpcshell

Comment 14

[Alex Vincent [:WeirdAl]]

Oh, and make sure you formally request review on both.  At this point it's probably a formality, but it's an important distinction from feedback requests.

Comment 15

[Jan Keromnes [:janx]]

Andy, are you still working on this bug? We ended up running into this issue from another part of the code, and we would like to see it fixed.

I can help you, or even steal the bug if you don't care about it anymore :)

Comment 17

[Jan Keromnes [:janx]]

(strange, marking a duplicate resets the mentor field)

Comment 18

[Iaroslav Sheptykin]

Hi, everybody!

Just as a heads-up, I will try to finish the work started by andy. Here is how I want to do it:
1. Apply both patches
2. Run tests
3. If tests pass I will resubmit the testing patch for the review.
4. If tests don't pass I will work on the code. I will squash all patches into one at the end and submit for the review.

Please, stop me if this plan has flows!

Comment 19

[Iaroslav Sheptykin]

Attachment: bug 1023685 - tests the bugfix

Hello, David!

I have carried out the steps described in comment 18. The patch from Andy and the test Alex work great together. All xpcshell tests pass on my setup (see comment 13). Andy's patch is already attached and verified, I applied it without modifications so it should be safe to commit it. In order to create a patch for the test I had to include some name into the header as per convention. I did not want to fake and place Alexe's name without his permission. So I put mine there, which can be easily corrected I believe. I didnt have to modify the code inside the test patch, so it is the same as in the patch submitted by Alex earlier. 

Please let me know if I can do anything else to close this bug.

Comment 20

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

Comment on attachment 8492164 [details] [diff] [review]
bug 1023685 - tests the bugfix

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

Thanks for the patch, this looks good, with a few minor changes below. As far as I am concerned, you wrote the patch, so I have no issue with your name being on it, even if it is based on someone else's STR. Since you put the bug number in the commit message, all the history of that bug may be found here, on Bugzilla, so you are not hiding anybody's previous work.

However, we often browse the list of changes for a given date range, so being more precise in the commit message would be helpful, e.g.: "Bug 1023685 - Testing OS.File.writeAtomic with a 0 bytes string (based on STR from Alex Vincent);r=yoric".

If you could take the opportunity to to apply the feedback of comment 10 to the other patch, we could finally get this landed.

::: toolkit/components/osfile/tests/xpcshell/test_osfile_writeAtomic_zerobytes.js
@@ +1,1 @@
> +let SHARED_PATH;

Could you add the license? Normally, you can copy and paste it from any other test in that directory.

Also, please add `"use strict";` as your first line of code. This helps catch errors.

@@ +13,5 @@
> +
> +  let decoder = new TextDecoder();
> +  let bin = yield OS.File.read(SHARED_PATH);
> +  let string2 = decoder.decode(bin);
> +  do_check_eq(string2, string1);

Can you use the (more recent) `Assert.equal(x, y, "Something that will appear in the log")` instead of the older `do_check_eq`?

@@ +16,5 @@
> +  let string2 = decoder.decode(bin);
> +  do_check_eq(string2, string1);
> +});
> +
> +add_task(do_test_finished);

You don't need this line.

@@ +19,5 @@
> +
> +add_task(do_test_finished);
> +
> +function run_test() {
> +  do_test_pending();

You don't need this line.

Comment 21

[Iaroslav Sheptykin]

Attachment: OS.File.writeAtomic now support writing encoded zero-byte strings

Hi, David! Thanks for the feedback! I modified the test according to your comments. All tests pass in my setup. 
I also addressed comment 10 by changing the commit message of Andy's patch and combining it with the test patch. Therefore, if this patch passes your review, the other packages can be marked as obsolete. I managed to preserve Andy's authorship of the patch (using git am), and referenced Alex Vincent, as you suggested in comment 20.

Comment 22

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

Comment on attachment 8493046 [details] [diff] [review]
OS.File.writeAtomic now support writing encoded zero-byte strings

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

Looks good to me, with a trivial change below.
Let's see if this passes tests:  https://tbpl.mozilla.org/?tree=Try&rev=316e270ff023

::: toolkit/components/osfile/tests/xpcshell/test_osfile_writeAtomic_zerobytes.js
@@ +17,5 @@
> +
> +  let decoder = new TextDecoder();
> +  let bin = yield OS.File.read(SHARED_PATH);
> +  let string2 = decoder.decode(bin);
> +  Assert.equal(string2, string1, "Checking if writeAtomic supports writing encoded zero-byte strings (bug 1023685)");

Nit: I would have put this as a comment near the top of this file. Here, I would have put
`Assert.equal(string2, string1, "Read the expected (empty) string.")

Comment 23

[Iaroslav Sheptykin]

> Nit: I would have put this as a comment near the top of this file. Here, I
> would have put
> `Assert.equal(string2, string1, "Read the expected (empty) string.")

Alright! I wasn't sure about the purpose of the message within Assert.equal. This makes sense.

Comment 24

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

Tests look good. Once you have finished your trivial change, we can mark this patch checkin-needed.

Comment 25

[Iaroslav Sheptykin]

Attachment: Bug 1023685 - OS.File.writeAtomic now supports writing encoded zero-byte strings (based on STR from Alex Vincent)

David, I have updated the patch according to your remark in comment 22. I marked the patch as + to indicate that it's been already reviewed (hope this is correct). To my understanding of comment 24, I also added flag "checkin" referring to you as you mentioned.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8493738 [details] [diff] [review]
Bug 1023685 - OS.File.writeAtomic now supports writing encoded zero-byte strings (based on STR from Alex Vincent)

In the future, please just use the checkin-needed bug keyword at the top. It plays more nicely with our bug marking tools :)

Comment 27

[Iaroslav Sheptykin]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #26)

> In the future, please just use the checkin-needed bug keyword at the top. It
> plays more nicely with our bug marking tools :)

Oh, sorry, my fault. I misunderstood comment 24 due to lack of experience. I will just wait for David to take the right action.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89c56a513e58

I tweaked the commit message a bit to better follow the guidelines linked below. Thanks everybody for the patches!
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/89c56a513e58

Comment 30

[Jan Keromnes [:janx]]

Oh, I didn't notice the fix. Nice job Iaroslav!