1032419 - Make the manifest cache more robust

Status: RESOLVED WONTFIX

(Core Graveyard :: DOM: Apps, defect)

Description

[[:fabrice] Fabrice Desré]

We hit a race condition in bug 1023796 where the manifest cache was flushed while being used in an async task, because a memory-pressure event was delivered.

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: manifest-cache.patch

We could move it to its own jsm but it's so small...

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8448238 [details] [diff] [review]
manifest-cache.patch

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

(In reply to Fabrice Desré [:fabrice] from comment #0)
> We hit a race condition in bug 1023796 where the manifest cache was flushed
> while being used in an async task, because a memory-pressure event was
> delivered.

Where's the race?

_readManifests assigns the manifest to elem.manifest via one of two code paths, and neither contain any asynchronous operation between the conditional that checks the cache for the presence of the ID and retrieval of the manifest from the cache by ID:

Here's what it looks like when we have a cache hit:

        if (!manifestCache[id]) {
          // code that doesn't execute
        }

        elem.manifest = manifestCache[id];

With a miss, there's an asynchronous operation, but we test manifestCache[id] after that operation and then immediately retrieve the manifest from the cache:

        if (!manifestCache[id]) {
            // do some stuff, including an asynchronous operation
            if (manifestCache[id]) {
              break;
            }
          }
        }

        elem.manifest = manifestCache[id];

In neither case can flushing the cache cause manifestCache[id] to be deleted between the conditional and the assignment.  Nor does flushing the cache affect the manifest objects themselves, nor any other object's reference to those objects.  A cache flush simply assigns this._manifestCache to a new object:

  this._manifestCache = {};

And that was true even before vingtetun's fix for bug 1023796.  So I don't see how this solves a problem.

Nevertheless, I've reviewed the code assuming it's actually needed, so you can fix it up and land it if I'm really missing something.

::: dom/apps/src/Webapps.jsm
@@ +155,5 @@
> +  let cache = {};
> +  let lockCount = 0;
> +  let pendingActions = [];
> +
> +  return {

Hmm, I wonder if a proxy <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy> would simplify this API.

@@ +160,5 @@
> +    set: function(aKey, aObj) {
> +      cache[aKey] = aObj;
> +    },
> +    has: function(aKey) {
> +      return (cache[aKey] !== undefined);

Nit: I would return (aKey in cache) to make fewer assumptions about the possible values for keys in the cache.

@@ +172,5 @@
> +        return;
> +      }
> +      if (cache[aKey] !== undefined) {
> +        delete cache[aKey];
> +      }

Nit: you should be able to simply delete cache(aKey) here, which'll succeed whether or not the property is defined (and regardless of its value).

@@ +176,5 @@
> +      }
> +    },
> +    flush: function() {
> +      if (lockCount != 0) {
> +        pendingActions.push({ action: "flush" });

Nit: I'd call "action" something like "type" to avoid the repetitive aAction.action references in unlock.

@@ +193,5 @@
> +          if (aAction.action == "remove") {
> +            this.remove(aAction.key);
> +          } else if (aAction.action == "flush") {
> +            this.flush();
> +          }

This never clears the pendingActions queue, so all the actions will be repeated the next time the cache is locked and then unlocked!

You could shift each item as you process the queue:

  while (let action = pendingActions.shift()) {
    …
  }

But probably it'd be better to assign pendingActions to a new array literal after the loop:

  pendingActions = [];

Also, if the loop hits a flush, then the rest of the queue is irrelevant, so the loop might as well break, which it can do if it uses for…of instead of Array.foreach:

  for (action of pendingActions) {
    …
    } else if (action.action == "flush") {
      this.flush();
      break;
    }
  }

@@ +197,5 @@
> +          }
> +        });
> +      }
> +    }
> +  }

Nit: append a semicolon to this closing brace, which completes the return statement.

@@ +2738,5 @@
>    /**
>      * Asynchronously reads a list of manifests
>      */
>  
> +  _manifestCache: createCache(),

Nit: since this is the only createCache caller, I would inline createCache here.

@@ +2743,3 @@
>  
>    _readManifests: function(aData) {
> +    this._manifestCache.lock(); // Prevent flushes during async operations.

This doesn't need to happen until inside the task, before it starts to loop over aData and access this._manifestCache.

@@ +2769,2 @@
>        }
> +      this._manifestCache.unlock();

This needs to ensure that the cache will get unlocked, even if there's an error while reading the manifests.  To do that, surround the code that might fail in a try block, then put the unlock call in a finally block, i.e.:

  _readManifests: function(aData) {
    return Task.spawn(function*() {
      this._manifestCache.lock(); // Prevent flushes during async operations.
      try {
        for (let elem of aData) {
          let id = elem.id;

          if (!this._manifestCache.has(id)) {
            // the manifest file used to be named manifest.json, so fallback on this.
            let baseDir = this.webapps[id].basePath == this.getCoreAppsBasePath()
                            ? "coreAppsDir" : DIRECTORY_NAME;

            let dir = FileUtils.getDir(baseDir, ["webapps", id], false, true);

            let fileNames = ["manifest.webapp", "update.webapp", "manifest.json"];
            for (let fileName of fileNames) {
              let manifest =
                yield AppsUtils.loadJSONAsync(OS.Path.join(dir.path, fileName));
              if (manifest) {
                this._manifestCache.set(id, manifest);
                break;
              }
            }
          }

          elem.manifest = this._manifestCache.get(id);
        }
      } finally {
        this._manifestCache.unlock();
      }
      return aData;
    }.bind(this)).then(null, Cu.reportError);
  },

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Core Graveyard / DOM: Apps is inactive. Closing all bugs in this component.