Closed Bug 1309249 Opened 8 years ago Closed 8 years ago

OS.File lz4 errors should print out the name of the file

Categories

(Toolkit Graveyard :: OS.File, defect)

Product:
Toolkit Graveyard
Component:
OS.File
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: Yoric, Assigned: sumi29, Mentored)

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(1 file, 1 obsolete file)

Bug 1309249: Add filepath to OS.File.read errors for lz4 decompression;
58 bytes, text/x-review-board-request
Yoric
: review+
Details 
As demonstrated by bug 1309235, when we use `OS.File.read` to read the contents of a compressed file, compression-related error message do not specify the name of the file causing the issue. Adding the name of the file would make debugging easier.

To do this, we should

1. modify function `decompressFileContent` in https://dxr.mozilla.org/mozilla-central/source/toolkit/components/lz4/lz4.js#105 so that all error messages display the contents of `options.path`;

2. modify method `AbstractFile.read` in http://searchfox.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_shared_front.jsm#329 so that we set the field `path` of `options` with the corresponding path ;

3. write a test.
Attached patch lz4-filename.patch (obsolete) — — Splinter Review 

    
    

    
  
Not sure on how to add to the existing test (worker_lz4.js) for that function - this is my first time writing/modifying an xpcshell test so please bear with me on that front.

    
    

    
  
Comment on attachment 8801587 [details] [diff] [review]
lz4-filename.patch

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

Thanks for the patch! It's pretty much what's needed (minus the tests for the moment), although I'd like a few changes detailed below.

For the test, you could indeed look at worker_lz4.js to test the in-memory compression. I'd prefer, though, if you added the test in https://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/tests/xpcshell/test_compression.js#13-30, so that we can test it with actual files. For this purpose

1. create an empty file using `yield OS.File.writeAtomic(some_path, some_empty_array, {compression: "lz4"})`
2. try to decompress it with a call to `yield OS.File.read(some_path, {compression: "lz4"})`
3. this should throw an error, so catch it and check that the `message` field contains `some_path`.

You can repeat this with a file containing random bytes or a good header but random contents. For the moment, though, let's just do the first test.

Also, a few things you need to know:

- By convention, your commit message should look like "Bug 1309249 - Making sure that OS.File.read errors print out the name of the file in case of lz4 error;r?Yoric" (you can add more lines after that if you wish to add more details). We use this convention to make sure that we can find easily where and when (and why) patches were added and who checked that the patch was correct (in this case, "Yoric", i.e. me).

- Next time, don't forget to explicitly request a review for the patch, otherwise, there are chances that nobody will know to look for it. To do so, set the "review" flag to someone's name (in this case, ":Yoric", i.e. still me). You can find the review flag in link "details" of your patch or when you're uploading the patch. If you don't know who should review the patch, Bugzilla will suggest a few names.

- I'm marking this patch as "feedback+" (or "f+"), which means "I like it, but we need a few changes before we can accept it".

- You're now officially in charge of fixing this bug :)

Of course, don't hesitate to ask questions if you need! I'm generally reachable over IRC (nickname: Yoric) or, of course, on this bug (use "need info from" with my nickname to make sure that I'll receive the message).

::: toolkit/components/lz4/lz4.js
@@ +105,5 @@
>  function decompressFileContent(array, options = {}) {
>    let bytes = SharedAll.normalizeBufferArgs(array, options.bytes || null);
>    if (bytes < HEADER_SIZE) {
> +    throw new LZError("decompress", "becauseLZNoHeader", "Buffer is too short (no header)" +
> +                      " - Data: " + options.path || array);

Good. It would be a bit more readable with backquotes:

`Buffer is too short (no header): - Data: ${ options.path || array}`

@@ +113,5 @@
>    let expectMagicNumber = new DataView(array.buffer, 0, MAGIC_NUMBER.byteLength);
>    for (let i = 0; i < MAGIC_NUMBER.byteLength; ++i) {
>      if (expectMagicNumber.getUint8(i) != MAGIC_NUMBER[i]) {
> +      throw new LZError("decompress", "becauseLZWrongMagicNumber", "Invalid header (no magic number" +
> +                      " - Data: " + options.path || array);

Same here.

@@ +141,5 @@
>                                        outputBuffer, outputBuffer.byteLength,
>                                        decompressedBytes.address());
>    if (!success) {
> +    throw new LZError("decompress", "becauseLZInvalidContent", "Invalid content:Decompression stopped at " + decompressedBytes.value +
> +                      " - Data: " + options.path || array);

Same here.

::: toolkit/components/osfile/modules/osfile_shared_front.jsm
@@ +326,4 @@
>      let buffer = file.read(bytes, options);
>      if ("compression" in options) {
>        if (options.compression == "lz4") {
> +        options.path = path;

Let's make this

options = Object.create(options); // Make sure we don't modify `options` accidentally.
options.path = path;

        Attachment #8801587 -
        Flags: feedback+
Assignee: nobody → sumi29

    
    

    
      
  


  

    
    

    
  
@Yoric:
Thanks for the detailed feedback! This isn't my first bug, but this is my first time touching this part of the codebase so I appreciate all the help.
I modified an existing testpoint to check for the file path as well. For the other errors that need to be tested, I could not figure out exactly which conditions would cause those errors - I tried creating an empty array for the 'becauseLZNoHeader' message but I couldn't hit the error, and I have no idea how to create invalid content for 'becauseLZInvalidContent'.

Thanks once again for bearing with my naivete!

        Attachment #8801587 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Sumit Tiwari from comment #5)
> @Yoric:
> Thanks for the detailed feedback! This isn't my first bug, but this is my
> first time touching this part of the codebase so I appreciate all the help.
> I modified an existing testpoint to check for the file path as well. For the
> other errors that need to be tested, I could not figure out exactly which
> conditions would cause those errors - I tried creating an empty array for
> the 'becauseLZNoHeader' message but I couldn't hit the error, and I have no
> idea how to create invalid content for 'becauseLZInvalidContent'.

Mmmh... That's weird. Could you put your code on pastebin or gist so that I can see the problem with the empty array?

For `becauseLZInvalidContent`, I would:
* create an `Uint8Array` of size, say, 256;
* set the first values manually to [109, 111, 122, 76, 122, 52, 48, 0] (i.e. "mozLz4a\0", which is what we write at the beginning of every mozlz4 file);
* fill everything else with arbitrary numbers (0 should do the trick);
* write that file with `yield OS.File.writeAtomic(some_path, some_array)` (no other option);
* attempt to read it back with `yield OS.File.read(some_path, { compression: "lz4" })`.


> Thanks once again for bearing with my naivete!

Hey, thanks for the patches :)

    
    

    
  
Comment on attachment 8801597 [details]
Bug 1309249: Add filepath to OS.File.read errors for lz4 decompression;

https://reviewboard.mozilla.org/r/86268/#review84984

Looks good to me. Now waiting for the remaining tests.

        Attachment #8801597 -
        Flags: review?(dteller)

    
    

    
  
@Yoric:
Pinged you on IRC to talk about the tests, but you were away so I am posting a Pastebin link to my tests here instead.

https://pastebin.mozilla.org/8919633

Please let me know what exactly it is that I am doing incorrectly.

    
    

    
  
I have annotated your code here:

https://pastebin.mozilla.org/8919930
Flags: needinfo?(sumi29)

    
    

    
      
  

    
    

    
  
@Yoric:
I had to make some more small changes to the tests, even after removing the compression option, to get them to actually hit the errors that we are interested in.
For the small header case, a non-empty array (an array of 0s, for example) above the size of 32 wouldn't work, so I created an array of size 8. For writing invalid content, I just wrote a non-empty, without a header, to a file without compression. 

Please let me know if the tests are incorrect in any way.

    
    

    
  
Comment on attachment 8801597 [details]
Bug 1309249: Add filepath to OS.File.read errors for lz4 decompression;

https://reviewboard.mozilla.org/r/86268/#review85834

Looks good to me.

Could you add one last test with the correct header but only 0s afterward?

::: toolkit/components/osfile/tests/xpcshell/test_compression.js:72
(Diff revision 2)
> +  do_check_true(!!exn);
> +  // Check the exception message (and that it contains the file name)
> +  do_check_true(exn.message.indexOf(`Buffer is too short (no header) - Data: ${ path }`) != -1);
> +});
> +
> +add_task(function test_invalid_content() {

That's actually testing for invalid header, rather than invalid content, so you should probably change the name of that function.

        Attachment #8801597 -
        Flags: review?(dteller)

    
    

    
  
@Yoric:
The testcase with correct header followed by only 0s doesn't fail at all, regardless of whether you specify the compression or not (the testcase is the same as in my last pastebin link). 

The 'becauseLZInvalidContent' case is really hard to check for - it seems that we aren't locking it down anywhere at all. DXR only fetches one result - and that's the very file that we are modifying. It seems that this particular error would be the result of an error on the C side, in lz4.c (in LZ4_decompress_generic). There are a whole bunch of cases there which would throw this error, but I don't know how to hit any of them.

    
    

    
  
Got it. Could you try with a bunch of 1s instead of a bunch of 0s? If that doesn't work, let's drop that testcase and just add a comment explaining why we drop it.

    
    

    
      
  

    
    

    
  
Comment on attachment 8801597 [details]
Bug 1309249: Add filepath to OS.File.read errors for lz4 decompression;

https://reviewboard.mozilla.org/r/86268/#review86216

Ok, if that works, I'm happy with the result (with a trivial name change, see below).

Thanks for the patch :)

Do you know how to handle the next steps?

::: toolkit/components/osfile/tests/xpcshell/test_compression.js:72
(Diff revisions 2 - 3)
>    do_check_true(!!exn);
>    // Check the exception message (and that it contains the file name)
>    do_check_true(exn.message.indexOf(`Buffer is too short (no header) - Data: ${ path }`) != -1);
>  });
>  
> -add_task(function test_invalid_content() {
> +add_task(function test_array_ones() {

Can you keep name `test_invalid_content`?

        Attachment #8801597 -
        Flags: review?(dteller) → review+

    
    

    
      
  

    
    

    
  
Comment on attachment 8801597 [details]
Bug 1309249: Add filepath to OS.File.read errors for lz4 decompression;

https://reviewboard.mozilla.org/r/86268/#review86218

Thanks :)

    
    

    
  
@Yoric:
Yes, it does work - thank you for coming up with that idea! I would have never thought of it myself.

I am afraid I don't know the next steps. Would you mind explaining? Thanks a lot for all the help!

    
    

    
  
So, the next steps are:

1. launching the unit tests to confirm that nothing is broken;
2. once the results are green, land the code.

I have just asked our unit tests server to launch the relevant unit tests, to confirm that everything is fine. You'll see the results here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6862888af42 (and on MozReview). In the future, you can do it from the "Automation" tab of MozReview.

Once this is done, we'll land the code. I don't know whether you can do it or it needs to be someone with higher commit level (e.g. me). Can you tell me whether the "Automation" tab contains a (non-greyed-out) item   "Land Commits"?

    
    

    
  
All the options under the 'Automation' tab are greyed out for me. I have fixed a bunch of trivial and non-trivial issues over the past couple of months, but I guess I need to wait a little more. 

Thanks for the information David, I truly appreciate it.

    
    

    
  
No problem I'll handle the landing.

Note that once you're ready to move to a higher-level commit level, you'll need to request it. You can read more about that here: https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/ .

    
    

    
  
Ok, tests were green, I have requested landing.

    
    

    
  
Pushed by dteller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9f023e0cc669
Add filepath to OS.File.read errors for lz4 decompression;r=Yoric

    
    

    
  
Thank you so much for all the help and feedback David; people like you truly make the community. :)

    
    

    
  
Hey, you did all the work :)

Thanks for the patch!
Flags: needinfo?(sumi29)

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/9f023e0cc669
Status: NEW → RESOLVED
Closed: 8 years ago

          status-firefox52:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

    
  
Product: Toolkit → Toolkit Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: