Bug 1587793 - re-associate manifest with a browser when getting them from cache r=snorp

Addresses the problem of cached manifests having the wrong associated browser.
However, this is only temporary solution, as cached manifests can have different
associated browsers (e.g., if 3x more tabs are open, this will still potentially be racy).

Differential Revision: https://phabricator.services.mozilla.com/D49706

--HG--
extra : moz-landing-system : lando

dom/manifest/Manifest.jsm

@@ -46,7 +46,7 @@ function generateHash(aString) {
 }
 
 /**
- * Trims the query paramters from a url
+ * Trims the query parameters from a url
  */
 function stripQuery(url) {
   return url.split("?")[0];
@@ -70,10 +70,18 @@ class Manifest {
     // However arbitrary urls are not safe file paths so lets hash it.
     const fileName = generateHash(manifestUrl) + ".json";
     this._path = OS.Path.join(MANIFESTS_DIR, fileName);
-    this._browser = browser;
+    this.browser = browser;
   }
 
-  async initialise() {
+  get browser() {
+    return this._browser;
+  }
+
+  set browser(aBrowser) {
+    this._browser = aBrowser;
+  }
+
+  async initialize() {
     this._store = new JSONFile({ path: this._path, saveDelayMs: 100 });
     await this._store.load();
   }
@@ -155,12 +163,13 @@ class Manifest {
  * Manifests maintains the list of installed manifests
  */
 var Manifests = {
-  async initialise() {
-    if (this.started) {
-      return this.started;
+  async _initialize() {
+    if (this._readyPromise) {
+      return this._readyPromise;
     }
 
-    this.started = (async () => {
+    // Prevent multiple initializations
+    this._readyPromise = (async () => {
       // Make sure the manifests have the folder needed to save into
       await OS.File.makeDir(MANIFESTS_DIR, { ignoreExisting: true });
 
@@ -169,21 +178,20 @@ var Manifests = {
       this._store = new JSONFile({ path: this._path });
       await this._store.load();
 
-      // If we dont have any existing data, initialise empty
+      // If we don't have any existing data, initialize empty
       if (!this._store.data.hasOwnProperty("scopes")) {
         this._store.data.scopes = new Map();
       }
-
-      // Cache the Manifest objects creates as they are references to files
-      // and we do not want multiple file handles
-      this.manifestObjs = {};
     })();
 
-    return this.started;
+    // Cache the Manifest objects creates as they are references to files
+    // and we do not want multiple file handles
+    this.manifestObjs = new Map();
+    return this._readyPromise;
   },
 
   // When a manifest is installed, we save its scope so we can determine if
-  // fiture visits fall within this manifests scope
+  // future visits fall within this manifests scope
   manifestInstalled(manifest) {
     this._store.data.scopes[manifest.scope] = manifest.url;
     this._store.saveSoon();
@@ -204,7 +212,9 @@ var Manifests = {
   // tied to the current page
   async getManifest(browser, manifestUrl) {
     // Ensure we have all started up
-    await this.initialise();
+    if (!this._readyPromise) {
+      await this._initialize();
+    }
 
     // If the client does not already know its manifestUrl, we take the
     // url of the client and see if it matches the scope of any installed
@@ -220,14 +230,19 @@ var Manifests = {
     }
 
     // If we have already created this manifest return cached
-    if (manifestUrl in this.manifestObjs) {
-      return this.manifestObjs[manifestUrl];
+    if (this.manifestObjs.has(manifestUrl)) {
+      const manifest = this.manifestObjs.get(manifestUrl);
+      if (manifest.browser !== browser) {
+        manifest.browser = browser;
+      }
+      return manifest;
     }
 
     // Otherwise create a new manifest object
-    this.manifestObjs[manifestUrl] = new Manifest(browser, manifestUrl);
-    await this.manifestObjs[manifestUrl].initialise();
-    return this.manifestObjs[manifestUrl];
+    const manifest = new Manifest(browser, manifestUrl);
+    this.manifestObjs.set(manifestUrl, manifest);
+    await manifest.initialize();
+    return manifest;
   },
 };
 

dom/manifest/test/browser_Manifest_install.js

@@ -28,12 +28,13 @@ add_task(async function() {
 
   await BrowserTestUtils.withNewTab(tabOptions, async function(browser) {
     let manifest = await Manifests.getManifest(browser, manifestUrl);
-    is(manifest.installed, false, "We havent installed this manifest yet");
+    is(manifest.installed, false, "We haven't installed this manifest yet");
 
     await manifest.install(browser);
     is(manifest.name, "hello World", "Manifest has correct name");
     is(manifest.installed, true, "Manifest is installed");
     is(manifest.url, manifestUrl, "has correct url");
+    is(manifest.browser, browser, "has correct browser");
 
     manifest = await Manifests.getManifest(browser, manifestUrl);
     is(manifest.installed, true, "New instances are installed");
@@ -49,4 +50,9 @@ add_task(async function() {
     foundManifest = Manifests.findManifestUrl("http://example.org/");
     is(foundManifest, null, "Does not find manifests outside scope");
   });
+  // Get the cached one now
+  await BrowserTestUtils.withNewTab(tabOptions, async browser => {
+    const manifest = await Manifests.getManifest(browser, manifestUrl);
+    is(manifest.browser, browser, "has updated browser object");
+  });
 });