1214058 - Support a simplified JSON add-on update protocol

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Kris Maglione [:kmag]]

+++ This bug was initially created as a clone of Bug #1192435 +++

(In reply to Kris Maglione [:kmag] from comment #7)
> I'm proposing to implement a JSON protocol, with update manifests in the
> following format, but otherwise identical to the current RDF update protocol:
> 
> {
>   "addon": {
>     "id": "addon@id"
>   },
>   "updates": [
>     {
>       "version": "1.0",
>       "update_link": "https://...",
>       [optional if update_link is https] "update_hash": "sha256:...",
>       [optional, future] "update_link_key": "pinned key for update link as
> in Public-Key-Pins header",
> 
>       [optional] "update_info_url": "https://...",
> 
>       [optional] "multiprocess_compatible": true,
> 
>       [optional] "applications": {
>         "gecko": {
>           [optional] "strict_min_version": "42a1",
>           [optional] "strict_max_version": "*", (any value other than "*"
> implies the strictCompatibility flag)
>         }
>       }
>     }
>   ]
> }
> 
> 
> I chose some of the key names to make the format more similar to our
> manifest.json format than to install.rdf/update.rdf.
> 
> The "update_link_key" property shouldn't be implemented in the first
> version, but I think it would be a good idea plan on allowing pinned keys
> for both the update link in manifest.json and the update links in
> update.json in the future, for people who insist on added security.

(In reply to Dave Townsend [:mossop] from comment #8)
> I would suggest making it possible to include information about multiple
> add-ons in a single file, in case developers want to use a shared json for
> all their add-ons. You could just use an array for that but I think it would
> be nicer for parsing to do something like:
> 
> {
>   "addon1@id": {
>     "updates": [ ... as above ... ],
>   },
>   "addon2@id": {
>     "updates": [ ... as above ... ],
>   }
> }
> 
> I suggest making the updates array a property of the add-on's object because
> it leaves us scope to include additional information about the add-on there.
> For example in the future we could extend this format to replace the current
> metadata ping and be able to pull metadata and update info in one request.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Proposed patch

I added a top-level "addons" property, containing keys for add-on
IDs, mainly in case we decide we need to add something else at the
top level later. I also added an "advisory_max_version" property,
mainly for continuity with the RDF update tests. I'm not sure if
it's actually a good idea.

Everything else should be as discussed.

Comment 2

[Kris Maglione [:kmag]]

Attachment: Support a simplified JSON protocol for add-on updates

Comment 3

[Kris Maglione [:kmag]]

Er, apparently bzupdate does strange things when it can't get a lock on the repository...

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 8672940 [details] [diff] [review]
Support a simplified JSON protocol for add-on updates

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

Definitely f+, any reason you didn't request full review?

::: toolkit/mozapps/extensions/internal/AddonUpdateChecker.jsm
@@ +398,5 @@
> + * @throws if the update manifest is invalid in any way
> + */
> +function parseJSONManifest(aId, aUpdateKey, aRequest) {
> +  if (aUpdateKey) {
> +      throw Components.Exception("Update keys are not supported for JSON update manifests");

\o/

2 space indent please

@@ +406,5 @@
> +    "array": val => Array.isArray(val),
> +    "object": val => val && typeof val == "object" && !Array.isArray(val),
> +  };
> +
> +  function getProperty(aObj, aProperty, aType, aDefault = undefined) {

aDefault doesn't seem to be used

@@ +550,5 @@
> +
> +    let uri = Services.io.newURI(this.url, null, null);
> +    if (uri instanceof Ci.nsIURL && uri.fileExtension.toLowerCase() == "json") {
> +      this.request.overrideMimeType("application/json");
> +    }

Is this necessary? You're handling the parsing yourself now so not sure why you need to set this anymore.

Comment 5

[Kris Maglione [:kmag]]

(In reply to Dave Townsend [:mossop] from comment #4)
> Comment on attachment 8672940 [details] [diff] [review]
> Support a simplified JSON protocol for add-on updates
> 
> Review of attachment 8672940 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Definitely f+, any reason you didn't request full review?

Yeah. There are a lot of tests for corner cases in the RDF 
protocol. I wanted to make sure I was on the right track before 
I checked for all of them.

> @@ +406,5 @@
> > +    "array": val => Array.isArray(val),
> > +    "object": val => val && typeof val == "object" && !Array.isArray(val),
> > +  };
> > +
> > +  function getProperty(aObj, aProperty, aType, aDefault = undefined) {
> 
> aDefault doesn't seem to be used

It should have been returned if the property doesn't exist. I 
need to add more tests for optional properties.

> @@ +550,5 @@
> > +
> > +    let uri = Services.io.newURI(this.url, null, null);
> > +    if (uri instanceof Ci.nsIURL && uri.fileExtension.toLowerCase() == "json") {
> > +      this.request.overrideMimeType("application/json");
> > +    }
> 
> Is this necessary? You're handling the parsing yourself now so not sure why
> you need to set this anymore.

Yeah. I want to handle the manifest as JSON if it has a either 
`.json` extension or a JSON MIME type. I could do the test from 
the callback, rather than here, but for people testing with 
file: URLs, it would default to text/xml, and they'd get 
confusing parse errors in the console, which I'd rather avoid.

Comment 6

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1214058: Part 1 - Add a simplified JSON-based add-on update protocol. r?Mossop

Bug 1214058: Part 1 - Add a simplified JSON-based add-on update protocol. r?Mossop

Comment 7

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1214058: Part 2 - Run add-on update tests against comparable JSON and RDF manifests. r?Mossop

Bug 1214058: Part 2 - Run add-on update tests against comparable JSON and RDF manifests. r?Mossop

I tried to keep the changes to existing tests as minimal as
possible. There were a few exceptions, though:

* test_update_ignorecompat.js was completely broken. I couldn't
  figure out why it was suddenly failing after I changed it to use
  `add_test`, and it turned out that it had been failing all along,
  but in a way that the harness didn't pick up.

* I changed most of the `do_throw` in update callbacks to `ok(false`
  because it took me about an hour to figure out where the test was
  failing when I hit one of them.

* I made some changes to sync `test_update.js` and `test_update_ignorecompat.js`
  where one appeared to have been changed without updating the
  other.

* I made `promiseFindAddonUpdates` a bit more generic, because I was
  planning to convert most of `test_update.js` to use it, rather
  than nested callbacks. I changed my mind a quarter of the way
  through, but decided to keep the changes, since they'll probably
  be useful elsewhere.

Comment 8

[Kris Maglione [:kmag]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2815f9a19bc1

Comment 9

[Kris Maglione [:kmag]]

Added some preliminary documentation:

https://developer.mozilla.org/en-US/Add-ons/Updates

I'll update the related documentation once this lands.

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 8675830 [details]
MozReview Request: Bug 1214058: Part 1 - Add a simplified JSON-based add-on update protocol. r?Mossop

https://reviewboard.mozilla.org/r/22437/#review20195

Looking pretty good so far but I'd like to see some of this addressed/answered before I review more.

::: toolkit/mozapps/extensions/internal/AddonUpdateChecker.jsm:457
(Diff revision 1)
> +  let updates = getRequiredProperty(addon, "updates", "array");

I think we can just say that a missing updates property also means no update info rather than throwing

::: toolkit/mozapps/extensions/internal/AddonUpdateChecker.jsm:472
(Diff revision 1)
> +    let app = getRequiredProperty(applications, "gecko", "object");

This should be optional. If the gecko property doesn't exist then just skip this update entry. To allow for other apps to make use of this format.

::: toolkit/mozapps/extensions/internal/AddonUpdateChecker.jsm:505
(Diff revision 1)
> +        scriptSecurity.checkLoadURIStrWithPrincipal(principal, result.updateURL,

We don't have this in the RDF case, is it necessary here and should we add it there?

::: toolkit/mozapps/extensions/internal/AddonUpdateChecker.jsm:511
(Diff revision 1)
> +      if (result.updateURL &&

I think we can share some of this logic between the two functions.

::: toolkit/mozapps/extensions/internal/AddonUpdateChecker.jsm:566
(Diff revision 1)
> +    if (uri instanceof Ci.nsIURL && uri.fileExtension.toLowerCase() == "json") {

I think relying on file extension can make this a surprise result. Instead just override to text/plain (to stop the XHR from trying to parse to result) and then manually try to parse as JSON or XML regardless of content-type (we've always ignored that previously anyway). If both fail log an error.

::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1543
(Diff revision 1)
> -  function MockWindowsRegKey() {
> +  var MockWindowsRegKey = function MockWindowsRegKey() {

I'm not sure why this is necessary

::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1927
(Diff revision 1)
> +    const DONE = "=== xpcshell test console listener done ===";

Using a DONE message to end isn't necessary here you can just end after the task finishes. In fact you can simplify this a bit like this:

    var promiseConsoleOutput = Task.async(function*(aTask) {
      let messages = []
      let listener = msg => {
        msg instanceof Components.interfaces.nsIScriptError;
        messages.push(msg);
      }
      
      Services.console.registerListener(listener);
      try {
        let result = yield aTask;
        resolve({ messages, result })
      }
      finally {
        Services.console.unregisterListener(listener);
      }
    })

Note I also changed it to resolve to an object which will make code using this a bit more self documenting.

::: toolkit/mozapps/extensions/test/xpcshell/test_json_updatecheck.js:16
(Diff revision 1)
> +Components.utils.import("resource://gre/modules/Task.jsm");

This is already imported in head_addons.js

::: toolkit/mozapps/extensions/test/xpcshell/test_json_updatecheck.js:83
(Diff revision 1)
> +  }, extensionsDir);

Why does this need to install an extension?

Comment 11

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/22437/#review20195

> I think we can just say that a missing updates property also means no update info rather than throwing

Good catch. I meant for that to be optional.

> This should be optional. If the gecko property doesn't exist then just skip this update entry. To allow for other apps to make use of this format.

Yeah, that makes sense.

> We don't have this in the RDF case, is it necessary here and should we add it there?

I was going to file a follow-up bug to add it to the RDF case. It doesn't really matter for XUL add-ons, since their updates are fully-privileged anyway. But for WebExtensions, it could offer a way to gain elevated privileges.

> I think relying on file extension can make this a surprise result. Instead just override to text/plain (to stop the XHR from trying to parse to result) and then manually try to parse as JSON or XML regardless of content-type (we've always ignored that previously anyway). If both fail log an error.

Hmmm... I can't think of any major problems with doing it that way. I'll give it a try.

> I'm not sure why this is necessary

I was getting errors about non-top-level function declarations in strict mode. The tests did still seem to pass, but the errors worried me.

> Using a DONE message to end isn't necessary here you can just end after the task finishes. In fact you can simplify this a bit like this:
> 
>     var promiseConsoleOutput = Task.async(function*(aTask) {
>       let messages = []
>       let listener = msg => {
>         msg instanceof Components.interfaces.nsIScriptError;
>         messages.push(msg);
>       }
>       
>       Services.console.registerListener(listener);
>       try {
>         let result = yield aTask;
>         resolve({ messages, result })
>       }
>       finally {
>         Services.console.unregisterListener(listener);
>       }
>     })
> 
> Note I also changed it to resolve to an object which will make code using this a bit more self documenting.

Are you sure? Console listener callbacks are dispatched asynchronously, and I can't think of any reason we can be sure the promise won't resolve before they've all been handled. The other test helpers that use console listeners also use a DONE message to make sure of that.

> Why does this need to install an extension?

I guess it doesn't. I originally wrote it thinking I'd be testing findUpdates as well, but I wound up skipping that.

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8675830 [details]
MozReview Request: Bug 1214058: Part 1 - Add a simplified JSON-based add-on update protocol. r?Mossop

Bug 1214058: Part 1 - Add a simplified JSON-based add-on update protocol. r?Mossop

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8675831 [details]
MozReview Request: Bug 1214058: Part 2 - Run add-on update tests against comparable JSON and RDF manifests. r?Mossop

Bug 1214058: Part 2 - Run add-on update tests against comparable JSON and RDF manifests. r?Mossop

I tried to keep the changes to existing tests as minimal as
possible. There were a few exceptions, though:

* test_update_ignorecompat.js was completely broken. I couldn't
  figure out why it was suddenly failing after I changed it to use
  `add_test`, and it turned out that it had been failing all along,
  but in a way that the harness didn't pick up.

* I changed most of the `do_throw` in update callbacks to `ok(false`
  because it took me about an hour to figure out where the test was
  failing when I hit one of them.

* I made some changes to sync `test_update.js` and `test_update_ignorecompat.js`
  where one appeared to have been changed without updating the
  other.

* I made `promiseFindAddonUpdates` a bit more generic, because I was
  planning to convert most of `test_update.js` to use it, rather
  than nested callbacks. I changed my mind a quarter of the way
  through, but decided to keep the changes, since they'll probably
  be useful elsewhere.

Comment 14

[Kris Maglione [:kmag]]

I think this addresses most of your comments.

I used your code for promiseConsoleMessages, but kept the DONE message, since I'm pretty sure it's necessary to avoid races.

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 8675830 [details]
MozReview Request: Bug 1214058: Part 1 - Add a simplified JSON-based add-on update protocol. r?Mossop

https://reviewboard.mozilla.org/r/22437/#review20503

::: toolkit/mozapps/extensions/internal/AddonUpdateChecker.jsm:247
(Diff revision 2)
> +      delete aUpdate.updateURL;

You should log an error at this point so developers can see what is going on. Also just return and skip checking if updateURL is null later.

::: toolkit/mozapps/extensions/internal/AddonUpdateChecker.jsm:507
(Diff revision 2)
> +      minVersion: getProperty(app, "strict_min_version", "string", "42.0a0"),

We should put this default min-version in a pref so different apps like Fennec can set it to something else.

::: toolkit/mozapps/extensions/internal/AddonUpdateChecker.jsm:640
(Diff revision 2)
> -      this.notifyError(AddonUpdateChecker.ERROR_PARSE_ERROR);
> +        JSON.parse(request.responseText);

It's a bit wasteful to do this JSON parse (and the later XML parse) and then throw away the result and do it all over again in the parsing functions. Can we just pass the parsed data to the functions?

::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1926
(Diff revision 2)
> +  const DONE = "=== xpcshell test console listener done ===";

Thanks, looks like console listeners became async a few years ago so this is the right thing to do here.

::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1929
(Diff revision 2)
> +  let awaitListener = Promise.defer();

I think we're trying to avoid Promise.defer so just use new Promise.

A couple of minor things but this is looking good!

Comment 16

[Dave Townsend [:mossop]]

Comment on attachment 8675831 [details]
MozReview Request: Bug 1214058: Part 2 - Run add-on update tests against comparable JSON and RDF manifests. r?Mossop

https://reviewboard.mozilla.org/r/22439/#review20511

Nice work thanks for all this. A couple of questions to be answered here.

::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1904
(Diff revision 2)
> +        addon.compatibilityUpdate = true;

This should be false right?

::: toolkit/mozapps/extensions/test/xpcshell/test_bug570173.js:47
(Diff revision 2)
> +      bootstrap: "true",

Why this change?

::: toolkit/mozapps/extensions/test/xpcshell/test_signed_install.js:26
(Diff revision 2)
> +});

This pattern is showing up a lot. How about a function in head_addons to do it. Could define a createHttpServer that gets the server and registers the stop automatically.

::: toolkit/mozapps/extensions/test/xpcshell/test_update.js:63
(Diff revision 2)
> +  let { updateFile, appId } = test;

You can just do this deconstruction in the for line

Comment 17

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/22437/#review20503

> You should log an error at this point so developers can see what is going on. Also just return and skip checking if updateURL is null later.

The script security manager logs an error. I initially overrode that and logged a separate error, but eventually decided that probably wasn't necessary.

> We should put this default min-version in a pref so different apps like Fennec can set it to something else.

Good point. We should probably use the same default value for the manifest.json parser too.

> It's a bit wasteful to do this JSON parse (and the later XML parse) and then throw away the result and do it all over again in the parsing functions. Can we just pass the parsed data to the functions?

It is a bit. As far as I know, the RDF parser can't take a pre-parsed DOM tree, which I guess is why we were doing it this way before. The JSON version certainly could, but it's such a trivial efficiency that I didn't think it was worth using a separate call signature for that parser.

I guess we could just skip the XML parser and go straight to parsing it as RDF, though.

Comment 18

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/22439/#review20511

> This should be false right?

Yep

> Why this change?

I think it was because I wanted to be able to replace the add-on without adding a second restart to the end of the loop.

> This pattern is showing up a lot. How about a function in head_addons to do it. Could define a createHttpServer that gets the server and registers the stop automatically.

Yeah, I was getting a bit worried by how many times I typed out that function. A helper sounds like a good idea.

> You can just do this deconstruction in the for line

I could if https://bugzil.la/449811 was fixed but, alas, the test functions are closures over those variables, and for the moment it doesn't work.

Comment 19

[Kris Maglione [:kmag]]

Comment on attachment 8675830 [details]
MozReview Request: Bug 1214058: Part 1 - Add a simplified JSON-based add-on update protocol. r?Mossop

Bug 1214058: Part 1 - Add a simplified JSON-based add-on update protocol. r?Mossop

Comment 20

[Kris Maglione [:kmag]]

Comment on attachment 8675831 [details]
MozReview Request: Bug 1214058: Part 2 - Run add-on update tests against comparable JSON and RDF manifests. r?Mossop

Bug 1214058: Part 2 - Run add-on update tests against comparable JSON and RDF manifests. r?Mossop

I tried to keep the changes to existing tests as minimal as
possible. There were a few exceptions, though:

* test_update_ignorecompat.js was completely broken. I couldn't
  figure out why it was suddenly failing after I changed it to use
  `add_test`, and it turned out that it had been failing all along,
  but in a way that the harness didn't pick up.

* I changed most of the `do_throw` in update callbacks to `ok(false`
  because it took me about an hour to figure out where the test was
  failing when I hit one of them.

* I made some changes to sync `test_update.js` and `test_update_ignorecompat.js`
  where one appeared to have been changed without updating the
  other.

* I made `promiseFindAddonUpdates` a bit more generic, because I was
  planning to convert most of `test_update.js` to use it, rather
  than nested callbacks. I changed my mind a quarter of the way
  through, but decided to keep the changes, since they'll probably
  be useful elsewhere.

Comment 21

[Dave Townsend [:mossop]]

Comment on attachment 8675830 [details]
MozReview Request: Bug 1214058: Part 1 - Add a simplified JSON-based add-on update protocol. r?Mossop

https://reviewboard.mozilla.org/r/22437/#review20797

::: toolkit/mozapps/extensions/internal/AddonUpdateChecker.jsm:657
(Diff revisions 2 - 3)
>            throw new Error("Update manifest was not valid XML");

Change this message to say it wasn't valid XML or JSON

Comment 22

[Dave Townsend [:mossop]]

Comment on attachment 8675831 [details]
MozReview Request: Bug 1214058: Part 2 - Run add-on update tests against comparable JSON and RDF manifests. r?Mossop

https://reviewboard.mozilla.org/r/22439/#review20799

Comment 23

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4d5d63a03ef3938d95f629a6a9ea31d3e88627d
Bug 1214058: Part 1 - Add a simplified JSON-based add-on update protocol. r=Mossop

https://hg.mozilla.org/integration/mozilla-inbound/rev/90e625ac70b2071f1c2430725892f7c266928521
Bug 1214058: Part 2 - Run add-on update tests against comparable JSON and RDF manifests. r=Mossop

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for xpcshell bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/789a931f4344

https://treeherder.mozilla.org/logviewer.html#?job_id=16765659&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=16765669&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=16765682&repo=mozilla-inbound

Comment 25

[Kris Maglione [:kmag]]

I'm guessing it just needs a clobber. The linux64 xpcshell tests passed on my first try push even without a clobber, though, so I'm not sure how to test this without pushing to inbound again:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5c3201df26c

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Try clobbers by default, so I don't think a try push on its own will tell you either way. Happy for you to re-land with a CLOBBER touch, though.

Comment 27

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e61464a21375344f4cc11b48b962cbc3100c3e60
Bug 1214058: Part 1 - Add a simplified JSON-based add-on update protocol. r=Mossop

https://hg.mozilla.org/integration/mozilla-inbound/rev/b27e589695eda4418a06ffeaa5ac93daf58e2c13
Bug 1214058: Part 2 - Run add-on update tests against comparable JSON and RDF manifests. r=Mossop

https://hg.mozilla.org/integration/mozilla-inbound/rev/39b88d0ca94f85feed1da6b469117525794ed9b3
Bug 1214058: Part 3 - Touch CLOBBER. r=trivial

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e61464a21375
https://hg.mozilla.org/mozilla-central/rev/b27e589695ed
https://hg.mozilla.org/mozilla-central/rev/39b88d0ca94f

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/e61464a21375
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/b27e589695ed
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/39b88d0ca94f

Comment 30

[Dave Townsend [:mossop]]

(In reply to Kris Maglione [:kmag] from comment #25)
> I'm guessing it just needs a clobber. The linux64 xpcshell tests passed on
> my first try push even without a clobber, though, so I'm not sure how to
> test this without pushing to inbound again:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5c3201df26c

Sorry I should have spotted this in review, you got hit by bug 994255

Comment 31

[Kris Maglione [:kmag]]

(In reply to Dave Townsend [:mossop] from comment #30)
> Sorry I should have spotted this in review, you got hit by bug 994255

Ah, interesting. I'll try to remember that.