Bug 1595625 - Create only one shared directory object per address book; r=mkmelin
authorGeoff Lankow <geoff@darktrojan.net>
Sun, 10 Nov 2019 23:53:21 +1300
changeset 37533 18938b699f1861ff962b763d90967bb02353f5a1
parent 37532 9ffcd0110d4155b71896110755152b8e0708397e
child 37534 c9fb9ef25bbbe9a72f81fae1959d12e8f9a53ada
push id396
push userclokep@gmail.com
push dateMon, 06 Jan 2020 23:11:57 +0000
reviewersmkmelin
bugs1595625
Bug 1595625 - Create only one shared directory object per address book; r=mkmelin
mailnews/addrbook/jsaddrbook/AddrBookDirectory.jsm
mailnews/addrbook/test/unit/test_jsaddrbook_inner.js
mailnews/addrbook/test/unit/xpcshell-jsaddrbook.ini
--- a/mailnews/addrbook/jsaddrbook/AddrBookDirectory.jsm
+++ b/mailnews/addrbook/jsaddrbook/AddrBookDirectory.jsm
@@ -82,42 +82,22 @@ AddrBookDirectory.prototype = {
         if (list.URI == uri) {
           this.__proto__ = list;
           return;
         }
       }
       throw Cr.NS_ERROR_UNEXPECTED;
     }
 
-    this.__proto__ = bookPrototype;
-
-    if (!this.dirPrefId) {
-      let filename = uri.substring("jsaddrbook://".length);
-      for (let child of Services.prefs.getChildList("ldap_2.servers.")) {
-        if (
-          child.endsWith(".filename") &&
-          Services.prefs.getStringPref(child) == filename
-        ) {
-          this.dirPrefId = child.substring(
-            0,
-            child.length - ".filename".length
-          );
-          break;
-        }
-      }
-      if (!this.dirPrefId) {
-        throw Cr.NS_ERROR_UNEXPECTED;
-      }
-      // Make sure we always have a file. If a file is not created, the
-      // filename may be accidentally reused.
-      let file = FileUtils.getFile("ProfD", [filename]);
-      if (!file.exists()) {
-        file.create(Ci.nsIFile.NORMAL_FILE_TYPE, 0o644);
-      }
+    let fileName = uri.substring("jsaddrbook://".length);
+    if (fileName.includes("/")) {
+      fileName = fileName.substring(0, fileName.indexOf("/"));
     }
+    this.__proto__ =
+      directories.get(fileName) || new AddrBookDirectoryInner(fileName);
   },
 };
 
 // Keep track of all database connections, and close them at shutdown, since
 // nothing else ever tells us to close them.
 
 var connections = new Map();
 var closeObserver = {
@@ -177,42 +157,74 @@ function closeConnectionTo(file) {
   if (connection) {
     return new Promise(resolve => {
       connection.asyncClose({
         complete() {
           resolve();
         },
       });
       connections.delete(file.path);
+      directories.delete(file.leafName);
     });
   }
   return Promise.resolve();
 }
 
+// One AddrBookDirectoryInner exists for each address book, multiple
+// AddrBookDirectory objects (e.g. queries) can use it as their prototype.
+
+var directories = new Map();
+
 /**
  * Prototype for nsIAbDirectory objects that aren't mailing lists.
  *
  * @implements {nsIAbCollection}
  * @implements {nsIAbDirectory}
  */
-var bookPrototype = {
+function AddrBookDirectoryInner(fileName) {
+  for (let child of Services.prefs.getChildList("ldap_2.servers.")) {
+    if (
+      child.endsWith(".filename") &&
+      Services.prefs.getStringPref(child) == fileName
+    ) {
+      this.dirPrefId = child.substring(0, child.length - ".filename".length);
+      break;
+    }
+  }
+  if (!this.dirPrefId) {
+    throw Cr.NS_ERROR_UNEXPECTED;
+  }
+
+  // Make sure we always have a file. If a file is not created, the
+  // filename may be accidentally reused.
+  let file = FileUtils.getFile("ProfD", [fileName]);
+  if (!file.exists()) {
+    file.create(Ci.nsIFile.NORMAL_FILE_TYPE, 0o644);
+  }
+
+  directories.set(fileName, this);
+  this._inner = this;
+  this._fileName = fileName;
+}
+AddrBookDirectoryInner.prototype = {
+  _uid: null,
   _nextCardId: null,
   _nextListId: null,
   get _prefBranch() {
     if (!this.dirPrefId) {
       throw Cr.NS_ERROR_NOT_AVAILABLE;
     }
     return Services.prefs.getBranch(`${this.dirPrefId}.`);
   },
   get _dbConnection() {
     let file = FileUtils.getFile("ProfD", [this.fileName]);
     let connection = openConnectionTo(file);
 
-    delete this._dbConnection;
-    Object.defineProperty(this, "_dbConnection", {
+    delete this._inner._dbConnection;
+    Object.defineProperty(this._inner, "_dbConnection", {
       enumerable: true,
       value: connection,
       writable: false,
     });
     return connection;
   },
   get _lists() {
     let selectStatement = this._dbConnection.createStatement(
@@ -242,44 +254,44 @@ var bookPrototype = {
         localId: cardStatement.row.localId,
       });
     }
     cardStatement.finalize();
     return results;
   },
 
   _getNextCardId() {
-    if (this._nextCardId === null) {
+    if (this._inner._nextCardId === null) {
       let value = 0;
       let selectStatement = this._dbConnection.createStatement(
         "SELECT MAX(localId) AS localId FROM cards"
       );
       if (selectStatement.executeStep()) {
         value = selectStatement.row.localId;
       }
-      this._nextCardId = value;
+      this._inner._nextCardId = value;
       selectStatement.finalize();
     }
-    this._nextCardId++;
-    return this._nextCardId.toString();
+    this._inner._nextCardId++;
+    return this._inner._nextCardId.toString();
   },
   _getNextListId() {
-    if (this._nextListId === null) {
+    if (this._inner._nextListId === null) {
       let value = 0;
       let selectStatement = this._dbConnection.createStatement(
         "SELECT MAX(localId) AS localId FROM lists"
       );
       if (selectStatement.executeStep()) {
         value = selectStatement.row.localId;
       }
-      this._nextListId = value;
+      this._inner._nextListId = value;
       selectStatement.finalize();
     }
-    this._nextListId++;
-    return this._nextListId.toString();
+    this._inner._nextListId++;
+    return this._inner._nextListId.toString();
   },
   _getCard({ uid, localId = null }) {
     let card = new AddrBookCard();
     card.directoryId = this.uuid;
     card._uid = uid;
     card.localId = localId;
     card._properties = this._loadCardProperties(uid);
     return card.QueryInterface(Ci.nsIAbCard);
@@ -454,31 +466,28 @@ var bookPrototype = {
     let oldValue = this.dirName;
     this.setLocalizedStringValue("description", value);
     MailServices.ab.notifyItemPropertyChanged(this, "DirName", oldValue, value);
   },
   get dirType() {
     return 101;
   },
   get fileName() {
-    if (!this._fileName) {
-      this._fileName = this._prefBranch.getStringPref("filename", "");
-    }
     return this._fileName;
   },
   get UID() {
     if (!this._uid) {
       if (this._prefBranch.getPrefType("uid") == Services.prefs.PREF_STRING) {
-        this._uid = this._prefBranch.getStringPref("uid");
+        this._inner._uid = this._prefBranch.getStringPref("uid");
       } else {
-        this._uid = newUID();
-        this._prefBranch.setStringPref("uid", this._uid);
+        this._inner._uid = newUID();
+        this._prefBranch.setStringPref("uid", this._inner._uid);
       }
     }
-    return this._uid;
+    return this._inner._uid;
   },
   get URI() {
     return this._uri;
   },
   get position() {
     return this._prefBranch.getIntPref("position", 1);
   },
   get uuid() {
new file mode 100644
--- /dev/null
+++ b/mailnews/addrbook/test/unit/test_jsaddrbook_inner.js
@@ -0,0 +1,69 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+const { AddrBookDirectory } = ChromeUtils.import(
+  "resource:///modules/AddrBookDirectory.jsm"
+);
+
+add_task(async () => {
+  let a = new AddrBookDirectory();
+  a.init("jsaddrbook://abook.sqlite");
+
+  let b = new AddrBookDirectory();
+  b.init("jsaddrbook://abook.sqlite/?fakeQuery");
+
+  // Different objects, same prototype.
+  notEqual(a, b);
+  equal(a.__proto__, b.__proto__);
+
+  let inner = a.__proto__;
+  equal(inner._fileName, "abook.sqlite");
+
+  // URI should be cached on the outer object.
+  ok(a.hasOwnProperty("_uri"));
+  ok(b.hasOwnProperty("_uri"));
+  ok(!inner.hasOwnProperty("_uri"));
+  equal(a._uri, "jsaddrbook://abook.sqlite");
+  equal(b._uri, "jsaddrbook://abook.sqlite/?fakeQuery");
+
+  // Query should be cached on the outer object.
+  ok(!a.hasOwnProperty("_query"));
+  ok(b.hasOwnProperty("_query"));
+  ok(!inner.hasOwnProperty("_query"));
+  ok(!a.isQuery);
+  ok(b.isQuery);
+  equal(b._query, "fakeQuery");
+
+  // UID should be cached on the inner object.
+  a.UID;
+  ok(!a.hasOwnProperty("_uid"));
+  ok(!b.hasOwnProperty("_uid"));
+  ok(inner.hasOwnProperty("_uid"));
+  equal(a.UID, b.UID);
+
+  // Database connection should be created on first access, and shared.
+  ok(!a.hasOwnProperty("_dbConnection"));
+  ok(!b.hasOwnProperty("_dbConnection"));
+  ok(!inner.hasOwnProperty("_dbConnection"));
+  ok(inner.__proto__.hasOwnProperty("_dbConnection"));
+
+  a._dbConnection;
+  ok(!a.hasOwnProperty("_dbConnection"));
+  ok(!b.hasOwnProperty("_dbConnection"));
+  ok(inner.hasOwnProperty("_dbConnection"));
+
+  // Calling _getNextCardId should increment a shared value.
+  equal(a._getNextCardId(), 1);
+  equal(a._getNextCardId(), 2);
+  equal(b._getNextCardId(), 3);
+  equal(a._getNextCardId(), 4);
+
+  // Calling _getNextListId should increment a shared value.
+  equal(b._getNextListId(), 1);
+  equal(b._getNextListId(), 2);
+  equal(a._getNextListId(), 3);
+  equal(b._getNextListId(), 4);
+});
--- a/mailnews/addrbook/test/unit/xpcshell-jsaddrbook.ini
+++ b/mailnews/addrbook/test/unit/xpcshell-jsaddrbook.ini
@@ -1,16 +1,17 @@
 [DEFAULT]
 dupe-manifest =
 head = head_jsaddrbook.js
 tail =
 support-files = data/*
 tags = jsaddrbook
 
 [include:xpcshell.ini]
+[test_jsaddrbook_inner.js]
 [test_migration1.js]
 head = head_migration.js
 [test_migration2.js]
 head = head_migration.js
 [test_migration3.js]
 head = head_migration.js
 [test_migration4.js]
 head = head_migration.js