Reading the contents of an XPI file

Question

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

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

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.