5
\$\begingroup\$

I wrote this code to list contents of an XPI file and then read the contents in it.

I was thinking of these areas of improvement:

  1. I put the zr.open in the try because if the file does not exist it will throw exception, and if zip reader is left open it usually crashes the add-on.
  2. I'm reusing the input stream create instance. How is it not safe? Is there an async version of this?
  3. Should I close the input stream instance after I am all done or each time after init?
  4. Is Cu.forceGC needed? I'm not sure I thought it might be.
  5. My exception checking is weird; I check the .name I'm not sure if this is the recommended way
var zr = Cc["@mozilla.org/libjar/zip-reader;1"].createInstance(Ci.nsIZipReader);
Cu.import('resource://gre/modules/osfile.jsm');
Cu.import('resource://gre/modules/FileUtils.jsm');

var reusableStreamInstance = Cc['@mozilla.org/scriptableinputstream;1'].createInstance(Ci.nsIScriptableInputStream);

var pathToXpiToRead = OS.Path.join(OS.Constants.Path.profileDir, 'extensions', 'PortableTester@jetpack.xpi');
var nsiFileXpi = new FileUtils.File(pathToXpiToRead);

try {
  zr.open(nsiFileXpi); //if file dne it throws here
  var entries = zr.findEntries('*');
  while (entries.hasMore()) {
    var entryPointer = entries.getNext(); //just a string of "zip path" (this means path to file in zip, and it uses forward slashes remember)
    var entry = zr.getEntry(entryPointer); // should return true on `entry instanceof Ci.nsIZipEntry`
    if (!entry.isDirectory) {
        var inputStream = zr.getInputStream(entryPointer);
        reusableStreamInstance.init(inputStream);
        var fileContents = reusableStreamInstance.read(entry.realSize);
        console.log('contenst of file=', fileContents);
    } else {
        console.log('is directory, no stream to read');
    }
  }
} catch (ex) {
  console.warn('exception occured = ', ex);
  if (ex.name == 'NS_ERROR_FILE_NOT_FOUND') {
    Services.ww.activeWindow.alert('XPI at path does not exist!\n\nPath = ' + pathToXpiToRead);
  }
} finally {
  zr.close();
  console.log('zr closed');
  //Cu.forceGC(); //im not sure shoud i do this here?
}
\$\endgroup\$
7
\$\begingroup\$

I put the zr.open in the try because if the file does not exist it will throw exception, and if zip reader is left open it usually crashes the add-on.

Crash? I wouldn't know about that. But yes, it should be closed. But then again, only .close() it after it has actually been opened.

try {
  zr.open();
  try {
    // do some stuff
  }
  // another catch possible as well. 
  finally {
    zr.close();
  }
}
catch (ex) {
  // handle
}

var reusableStreamInstance = Cc['@mozilla.org/scriptableinputstream;1'].createInstance(Ci.nsIScriptableInputStream);

I'm resuing the input stream create instnace, how is it not safe?

The stream wrappers, or XPCOM in general really, usually have a contract that you should call .init() only once, unless stated otherwise. nsIScriptableInputStream does not state otherwise, and while it just works(tm) to call .init() again and again right now, that might change in the future.

Better create a stream wrapper per wrapped stream.

Should I close the input stream instance after i am all done or everytime after init

Yes.

Is Cu.forceGC needed? I'm not sure I thought it might be.

If you're not sure, then it is not needed. If you know it is "needed", check if there is another way. Manually calling the garbage collector does not only impact performance, it is - or at least used to be - a sure way to crash when calling it too often/too randomly.

My exception checking is weird, I check the .name I'm not sure if this is the recommended way

Actually, you can check ex.result for nsIExceptions, even within the catch.

catch (ex if ex.result == Components.results.NS_ERROR_FILE_NOT_FOUND) {
  Services.ww.activeWindow.alert('XPI at path does not exist!\n\nPath = ' + pathToXpiToRead);
}
catch (ex) {
  console.warn('exception occured = ', ex);
}

My take

var {classes: Cc, interfaces: Ci, results: Cr, Constructor: CC, utils: Cu } =
  Components;

let ZipFileReader =
  CC("@mozilla.org/libjar/zip-reader;1", "nsIZipReader", "open");
let ScriptableInputStream =
  CC("@mozilla.org/scriptableinputstream;1", "nsIScriptableInputStream", "init");

function handleEntry(name) {
  try {
    let entry = this.getEntry(name);
    if (entry.isDirectory) {
      console.log(name + " is directory, no stream to read");
      return false;
    }
    let stream = new ScriptableInputStream(this.getInputStream(name));
    try {
      // Use readBytes to get binary data, read to read a (null-terminated) string
      let contents = stream.readBytes(entry.realSize);
      console.log("Contents of " + name, contents);
    }
    finally {
      stream.close();
    }
    return true;
  }
  catch (ex) {
    console.warn("Failed to read " + name);
  }
  return false;
}

try {
  var xpi = Services.dirsvc.get("ProfD", Ci.nsIFile);   
  "extensions/noverflow@sdrocking.com.xpi".
    split("/").forEach(p => xpi.append(p));

  let reader = new ZipFileReader(xpi);
  try {
    let entries = reader.findEntries('*');
    while (entries.hasMore()) {
      let name = entries.getNext();
      if (handleEntry.call(reader, name)) {
        console.debug("Handled entry " + name);
      }
    }
  }
  finally {
    reader.close();
  }
}
catch (ex if ex.result == Cr.NS_ERROR_FILE_NOT_FOUND) {
  Services.ww.activeWindow.alert("XPI at path does not exist!\n\nPath = " + xpi.path);
}
catch (ex) {
  console.warn("exceptional exception", ex);
}

Edit I moved xpi into the try-catch block, so it doesn't pseudo-leak (becomes a global variable, i.e. this.xpi or window.xpi).

Nested try-blocks

As you asked for this in the comments... First lets see what happens with this:

try {
  try {
    throw new Error("oops");
  }
  finally {
    console.log("finally");
  }
}
catch (ex) {
  console.error("outer", ex.message);
}

Result

"finally"
"outer" "oops"

Now, if we already caught the exception in the inner try-block by adding a catch block

try {
  try {
    throw new Error("oops");
  }
  catch (ex) {
    console.error("inner", ex.message);
  }
  finally {
    console.log("finally");
  }
}
catch (ex) {
  console.error("outer", ex.message);
}

Result

"inner" "oops"
"finally"

And now, lets re-throw the error.

try {
  try {
    throw new Error("oops");
  }
  catch (ex) {
    console.error("inner", ex.message);
    throw ex;
  }
  finally {
    console.log("finally");
  }
}
catch (ex) {
  console.error("outer", ex.message);
}

Result

"inner" "oops"
"finally"
"outer" "oops"

So there you have it. Any given exception will be caught only once, by the nearest enclosing catch-block, unless it is re-thrown. Of course any new exceptions raised in "inner" block (because code in catch-block way do something that throws ;) will be caught by the "outer" block.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much I just learned about CC and new Scripta... so cool. Never understood that. But why did you use let? Any special reason? Can I use var just fine in place of all those lets? People tell me to avoid let till it's standardized as the standard is landing a little different than how moz currently does let. Also learned some fat arrow and I see you append it, so only using / for just typing path, for cross platform reasons. Interesting how this within handleEntry is pointing to the reader. So interesting to see how someone else would do you're code. \$\endgroup\$ – Noitidart Jul 12 '14 at 23:02
  • \$\begingroup\$ Learned a lot on that excpetion handling too, and really like the "exceptional exception" message hahaha. The Cr.NS_.... was the way I was thinking was recommended but I couldn't remember how to compare if it was NS_... I had forgot the Cr. before it. Thanks \$\endgroup\$ – Noitidart Jul 12 '14 at 23:07
  • \$\begingroup\$ Question man: We have a block of try-catch-finally and within the try we have the let reader = new and then a try-finally block. If this try-finally block throws an exception will it catch in the exceptional exception catch? If we made this try-finally a try-catch-finally with the catch being console.log('sub catch'), would it catch twice or just once? \$\endgroup\$ – Noitidart Jul 12 '14 at 23:18
  • 1
    \$\begingroup\$ I addressed the try stuff in the answer, and discuss let in the answer to your other question \$\endgroup\$ – nmaier Jul 13 '14 at 1:16
  • \$\begingroup\$ Wow so awesome thank you man this is superb. I cpoied pasted that to MDN :: try..catch with credit to you in commit comments: "added section on nested try blocks. full credit due to @nmaier: codereview.stackexchange.com/a/56824/49106" \$\endgroup\$ – Noitidart Jul 13 '14 at 1:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.