TypeError: C pointer missing bytes indication.

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

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

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.

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.

"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

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

Improved patch

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`?

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 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.

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

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).

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

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.

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 :)

(strange, marking a duplicate resets the mentor field)

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!

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 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.

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 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.")

> 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.

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

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 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 :)

(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.

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

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