558834 - Support targetPlatforms from install manifests

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[Dave Townsend [:mossop]]

We currently read the information but then don't put it into the database or make compatibility judgements based on it.

Comment 2

[Dave Townsend [:mossop]]

Attachment: DB patch rev 1

This adds DB support for the targetPlatform and skinnable properties from install.rdf and also adds a size property to the DB that is calculated at install time. I want to do all these in one go so it is only one schema update. I've also fixed a problem where switching between different DB schemas will throw away any updated compatibility information from updates. This means that right now downgrading from schema 2 to a version running schema 1 will lose that info, upgrading will retain it which I think is fine.

Comment 3

[Dave Townsend [:mossop]]

Attachment: targetPlatform support rev 1

This adds a new property to addons, isPlatformCompatible that is based on the data in targetPlatforms and uses it to app enable/disable add-ons as appropriate.

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 454619 [details] [diff] [review]
DB patch rev 1

Actually cancel that, one other property we're going to add while we're here.

Comment 5

[Dave Townsend [:mossop]]

Attachment: DB patch rev 2

This is the previously mentioned DB patch with an added property sourceURI for addons that holds the URI that the XPI file came from if known. I'm going to add tests for this in bug 575463 which will also rename AddonInstall.sourceURL to AddonInstall.sourceURI for consistency with Addon.getResourceURI.

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 454709 [details] [diff] [review]
DB patch rev 2

Gah, wrong patch

Comment 7

[Dave Townsend [:mossop]]

Attachment: DB patch rev 2

Comment 8

[Dave Townsend [:mossop]]

*sigh* and please ignore the hunk in AddonManager.jsm, it slipped in while debugging and I've removed it locally.

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 454710 [details] [diff] [review]
DB patch rev 2

Ok cancel this again, Blair has pointed out that we want some more info in the DB, will upload a new patch tomorrow.

Comment 10

[Dave Townsend [:mossop]]

Attachment: DB patch rev 3

Hopefully the last set of DB changes. This now also includes an infoURI property for add-ons which will hold the release notes for installed versions (i.e. when we detect a new version and see its updateInfoURL then download and install we keep the info URL to be viewed with the new version at a later time). Feeding that property in is covered in bug 576089

Comment 11

[Robert Strong (they/them - no direct email)]

Comment on attachment 455264 [details] [diff] [review]
DB patch rev 3

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>...
>@@ -450,63 +463,125 @@ function loadManifestFromRDF(aUri, aStre
>       WARN("Ignoring duplicate targetApplication entry for " + targetAppInfo.id +
>            " in install manifest");
>       continue;
>     }
>     seenApplications.push(targetAppInfo.id);
>     addon.targetApplications.push(targetAppInfo);
>   }
> 
>-  addon.targetPlatforms = getPropertyArray(ds, root, "targetPlatform");
>+  // Note that we don't need to check for duplicate targetPlatform entries since
>+  // the RDF service coalesces them for us.
>+  let targetPlatforms = getPropertyArray(ds, root, "targetPlatform");
>+  addon.targetPlatforms = [];
>+  targetPlatforms.forEach(function(aPlatform) {
>+    let platform = {
>+      os: null,
>+      abi: null
>+    };
>+
>+    let pos = aPlatform.indexOf("_");
>+    if (pos >= 0) {
nit: != -1 or flip flop the code below and use == -1

>+      platform.os = aPlatform.substring(0, pos);
>+      platform.abi = aPlatform.substring(pos + 1);
>+    }
>+    else {
>+      platform.os = aPlatform;
>+    }
>+
>+    addon.targetPlatforms.push(platform);
>+  });
>...
> /**
>  * Loads an AddonInternal object from an add-on extracted in a directory.
>  *
>  * @param  aDir
>  *         The nsIFile directory holding the add-on
>  * @return an AddonInternal object
>  * @throws if the directory does not contain a valid install manifest
>  */
> function loadManifestFromDir(aDir) {
>+  function getFileSize(aFile) {
>+    if (aFile.isSymlink())
>+      return 0;
>+
>+    if (!aFile.isDirectory())
>+      return aFile.fileSize;
>+
>+    let size = 0;
>+    let entries = aFile.directoryEntries.QueryInterface(Ci.nsIDirectoryEnumerator);
>+    let entry;
>+    while (entry = entries.nextFile)
>+      size += getFileSize(entry);
>+    entries.close();
>+    return size;
>+  }
>+
>   let file = aDir.clone();
>   file.append(FILE_INSTALL_MANIFEST);
>   if (!file.exists() || !file.isFile())
>     throw new Error("Directory " + dir.path + " does not contain a valid " +
>                     "install manifest");
> 
>   let fis = Cc["@mozilla.org/network/file-input-stream;1"].
>             createInstance(Ci.nsIFileInputStream);
>   fis.init(file, -1, -1, false);
>   let bis = Cc["@mozilla.org/network/buffered-input-stream;1"].
>             createInstance(Ci.nsIBufferedInputStream);
>   bis.init(fis, 4096);
> 
>   try {
>     let addon = loadManifestFromRDF(Services.io.newFileURI(file), bis);
>     addon._sourceBundle = aDir.clone().QueryInterface(Ci.nsILocalFile);
>+    addon.size = getFileSize(aDir);
>     return addon;
>   }
>   finally {
>     bis.close();
>     fis.close();
>   }
> }
> 
>+function loadManifestFromZipReader(aZipReader) {
Please add a javadoc comment for this function

>+  let zis = aZipReader.getInputStream(FILE_INSTALL_MANIFEST);
>+  let bis = Cc["@mozilla.org/network/buffered-input-stream;1"].
>+            createInstance(Ci.nsIBufferedInputStream);
>+  bis.init(zis, 4096);
>+
>+  try {
>+    let uri = buildJarURI(aZipReader.file, FILE_INSTALL_MANIFEST);
>+    let addon = loadManifestFromRDF(uri, bis);
>+    addon._sourceBundle = aZipReader.file;
>+
>+    addon.size = 0;
>+    let entries = aZipReader.findEntries(null);
>+    while (entries.hasMore())
>+      addon.size += aZipReader.getEntry(entries.getNext()).realSize;
>+
>+    return addon;
>+  }
>+  finally {
>+    bis.close();
>+    zis.close();
>+  }
>+}
Please verify that the zip can be deleted on Windows with this code since Windows file in use has prevented us in the past from doing so.

>@@ -2363,17 +2438,18 @@ var XPIProvider = {
>       AddonManagerPrivate.notifyAddonChanged(aAddon.id, aAddon.type, false);
>   }
> };
> 
> const FIELDS_ADDON = "internal_id, id, location, version, type, internalName, " +
>                      "updateURL, updateKey, optionsURL, aboutURL, iconURL, " +
>                      "defaultLocale, visible, active, userDisabled, appDisabled, " +
>                      "pendingUninstall, descriptor, installDate, updateDate, " +
>-                     "applyBackgroundUpdates, bootstrap";
>+                     "applyBackgroundUpdates, bootstrap, skinnable, size, " +
>+                     "sourceURI, infoURI";
I think I'm ok with infoURI though I keep thinking that it should be releaseNoteURI. I leave that up to you.

>@@ -2791,34 +2897,41 @@ var XPIDatabase = {
>                                   "type TEXT, internalName TEXT, updateURL TEXT, " +
>                                   "updateKey TEXT, optionsURL TEXT, aboutURL TEXT, " +
>                                   "iconURL TEXT, defaultLocale INTEGER, " +
>                                   "visible INTEGER, active INTEGER, " +
>                                   "userDisabled INTEGER, appDisabled INTEGER, " +
>                                   "pendingUninstall INTEGER, descriptor TEXT, " +
>                                   "installDate INTEGER, updateDate INTEGER, " +
>                                   "applyBackgroundUpdates INTEGER, " +
>-                                  "bootstrap INTEGER, UNIQUE (id, location)");
>+                                  "bootstrap INTEGER, skinnable INTEGER, " +
>+                                  "size INTEGER, sourceURI TEXT, infoURI TEXT," +
s/infoURI TEXT,"/infoURI TEXT, "/

Comment 12

[Robert Strong (they/them - no direct email)]

Comment on attachment 455264 [details] [diff] [review]
DB patch rev 3

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>...
>@@ -4774,30 +4918,38 @@ function createWrapper(aAddon) {
> 
> /**
>  * The AddonWrapper wraps an Addon to provide the data visible to consumers of
>  * the public API.
>  */
> function AddonWrapper(aAddon) {
>   ["id", "version", "type", "isCompatible",
>    "providesUpdatesSecurely", "blocklistState", "appDisabled",
>-   "userDisabled"].forEach(function(aProp) {
>+   "userDisabled", "skinnable", "size"].forEach(function(aProp) {
>      this.__defineGetter__(aProp, function() aAddon[aProp]);
>   }, this);
> 
>   ["optionsURL", "aboutURL"].forEach(function(aProp) {
>     this.__defineGetter__(aProp, function() {
>       return aAddon.active ? aAddon[aProp] : null;
>     });
>   }, this);
> 
>   ["installDate", "updateDate"].forEach(function(aProp) {
>     this.__defineGetter__(aProp, function() new Date(aAddon[aProp]));
>   }, this);
> 
>+  ["sourceURI", "infoURI"].forEach(function(aProp) {
>+    this.__defineGetter__(aProp, function() {
>+        if (!aAddon[aProp])
>+        return null;
nit: indentation if off

>+      return NetUtil.newURI(aAddon[aProp]);
>+    });
>+  }, this);
>+

Now onto the tests

Comment 13

[Robert Strong (they/them - no direct email)]

Comment on attachment 455264 [details] [diff] [review]
DB patch rev 3

r=me with comments addressed

Comment 14

[Robert Strong (they/them - no direct email)]

Comment on attachment 454621 [details] [diff] [review]
targetPlatform support rev 1

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>@@ -4736,16 +4745,50 @@ AddonInternal.prototype = {
>     return !!(this.updateKey || !this.updateURL ||
>               this.updateURL.substring(0, 6) == "https:");
>   },
> 
>   get isCompatible() {
>     return this.isCompatibleWith();
>   },
> 
>+  get isPlatformCompatible() {
>+    if (this.targetPlatforms.length == 0)
>+      return true;
>+
>+    let matchedOS = false;
>+
>+    // If any targetPlatform matches the OS and contains an ABI then we will
>+    // only match a targetPlatform that contains both the current OS and ABI
>+    let needsABI = false;
>+
>+    // Some platforms do not specify an ABI, test against null in that case.
>+    let abi = null; 
nit: trailing whitespace

Comment 15

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/8b3d47a7e2b9
http://hg.mozilla.org/mozilla-central/rev/fbb118bcd786

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with 

If an add-on is not supported by a platform the user wants to install it on, we do not show any failure notice. I have added this issue as another todo entry on bug 577048.

Regarding the schema update, do we really have to expose the schemaVersion to the prefs.js? No-one else, i.e. places.sqlite or signons.sqlite, is doing that.

Comment 17

[Dave Townsend [:mossop]]

(In reply to comment #16)
> Verified fixed with 
> 
> If an add-on is not supported by a platform the user wants to install it on, we
> do not show any failure notice. I have added this issue as another todo entry
> on bug 577048.
> 
> Regarding the schema update, do we really have to expose the schemaVersion to
> the prefs.js? No-one else, i.e. places.sqlite or signons.sqlite, is doing that.

signons and places store their schema version in the database, but we avoid loading the database on startup when possible so we have to hold the schema version in preferences in order to detect it is outdated without loading the database.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

That makes sense then when looking at perf improvements for start-up. Thanks Dave.