Closed
Bug 1214058
Opened 9 years ago
Closed 9 years ago
Support a simplified JSON add-on update protocol
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
People
(Reporter: kmag, Assigned: kmag)
References
Details
(Keywords: dev-doc-needed)
Attachments
(3 files, 1 obsolete file)
+++ 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.
Assignee | ||
Comment 1•9 years ago
|
||
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.
Attachment #8672938 -
Flags: feedback?(dtownsend)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8672940 -
Flags: feedback?(dtownsend)
Assignee | ||
Updated•9 years ago
|
Attachment #8672938 -
Attachment is obsolete: true
Attachment #8672938 -
Flags: feedback?(dtownsend)
Assignee | ||
Comment 3•9 years ago
|
||
Er, apparently bzupdate does strange things when it can't get a lock on the repository...
Comment 4•9 years ago
|
||
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.
Attachment #8672940 -
Flags: feedback?(dtownsend) → feedback+
Assignee | ||
Comment 5•9 years ago
|
||
(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.
Assignee | ||
Comment 6•9 years ago
|
||
Bug 1214058: Part 1 - Add a simplified JSON-based add-on update protocol. r?Mossop
Attachment #8675830 -
Flags: review?(dtownsend)
Assignee | ||
Comment 7•9 years ago
|
||
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.
Attachment #8675831 -
Flags: review?(dtownsend)
Assignee | ||
Comment 8•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2815f9a19bc1
Assignee | ||
Comment 9•9 years ago
|
||
Added some preliminary documentation: https://developer.mozilla.org/en-US/Add-ons/Updates I'll update the related documentation once this lands.
Keywords: dev-doc-needed
Comment 10•9 years ago
|
||
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?
Attachment #8675830 -
Flags: review?(dtownsend)
Assignee | ||
Comment 11•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Attachment #8675830 -
Flags: review?(dtownsend)
Assignee | ||
Comment 12•9 years ago
|
||
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
Assignee | ||
Comment 13•9 years ago
|
||
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.
Assignee | ||
Comment 14•9 years ago
|
||
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•9 years ago
|
||
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!
Attachment #8675830 -
Flags: review?(dtownsend)
Updated•9 years ago
|
Attachment #8675831 -
Flags: review?(dtownsend)
Comment 16•9 years ago
|
||
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
Assignee | ||
Comment 17•9 years ago
|
||
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.
Assignee | ||
Comment 18•9 years ago
|
||
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.
Updated•9 years ago
|
Iteration: 44.2 - Oct 19 → 44.3 - Nov 2
Assignee | ||
Comment 19•9 years ago
|
||
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
Attachment #8675830 -
Flags: review?(dtownsend)
Assignee | ||
Comment 20•9 years ago
|
||
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.
Attachment #8675831 -
Flags: review?(dtownsend)
Updated•9 years ago
|
Attachment #8675830 -
Flags: review?(dtownsend) → review+
Comment 21•9 years ago
|
||
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•9 years ago
|
||
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
Attachment #8675831 -
Flags: review?(dtownsend) → review+
Updated•9 years ago
|
Iteration: 44.3 - Nov 2 → 45.1 - Nov 16
Assignee | ||
Comment 23•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Iteration: 45.1 - Nov 16 → 44.3 - Nov 2
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
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 25•9 years ago
|
||
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
Flags: needinfo?(kmaglione+bmo)
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.
Assignee | ||
Comment 27•9 years ago
|
||
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•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e61464a21375 https://hg.mozilla.org/mozilla-central/rev/b27e589695ed https://hg.mozilla.org/mozilla-central/rev/39b88d0ca94f
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 29•9 years ago
|
||
bugherder uplift |
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
status-b2g-v2.5:
--- → fixed
Comment 30•9 years ago
|
||
(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
Assignee | ||
Comment 31•9 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•