initial version of an install and uninstall hook #778
Conversation
|
The gory stack trace I get from this test case is: |
|
Let me know if you want a full review pass over this, I'm assuming it's still too WIP for me to run through besides getting that test to pass. |
|
@mnoorenberghe helped point me towards some examples in mozilla-central, and I've added a commit that generates a |
|
@Osmose - this works for me now. My moz-central repo was completely screwy. I blame the ghost of steve jobs. |
| }, reason => { | ||
| ok(false, `=== Failed to uninstall addon ${reason}`); | ||
| }); | ||
| dump(`SIGNAL: ${gTestPath}`); |
There was a problem hiding this comment.
You have a left over debug line here.
crankycoder
force-pushed
the
features/driver-xpi-install
branch
from
b386e32
to
444db37
Compare
| dir.append("normandy.xpi"); | ||
| const xpiUrl = Services.io.newFileURI(dir).spec; | ||
| var addonId = await driver.installAddon(xpiUrl); | ||
| await driver.uninstallAddon(addonId); |
There was a problem hiding this comment.
This should actually test that the add-on was installed successfully somehow.
There was a problem hiding this comment.
The installAddon method returns the addon ID only if the installation is successful. The uninstall process would also fail if the addon ID was invalid, or if no installed addon could be found with that addonID. Is that not sufficient or do you want this to check the list of installed addons explicitly?
There was a problem hiding this comment.
If this ever fails (partially or fully), I expect checking explicitly that addonId is not undefined (or whatever else installAddon returns if installation didn't work) and/or that there's an installed add-on with the returned ID, will result in more understandable failure messaging than "uninstallAddon threw an error" (because the install failed).
More generally, I'm surprised the ID would be what this returns, and that's all that it returns - the ID is available without installing the add-on...
Also, late nit: let please. :-)
There was a problem hiding this comment.
I think we should actually just do a quick check to see that the add-on ID we expect is in the add-on list, and then isn't after uninstall, just to be thorough. If that's difficult, then at least a comment explaining the return values would help make this test more readable.
| * file:// and downloadable URLs. | ||
| */ | ||
|
|
||
| var installObjectCallback = (installObject, resolve, reject) => { |
There was a problem hiding this comment.
let, here and elsewhere. Though this also doesn't actually seems to use its binding (ie this) and so maybe you just want:
function installObjectCallback(installObject, resolve, reject) {
...
}| installAddon(url) { | ||
| /* Install an addon. Note that the AddonManager can only handle | ||
| * file:// and downloadable URLs. | ||
| */ |
There was a problem hiding this comment.
Spacing and indenting for this docstring seems off.
| var installObjectCallback = (installObject, resolve, reject) => { | ||
| installObject.install(); | ||
| installObject.addListener({ | ||
| onInstallEnded: (addonInstall, addon) => { |
There was a problem hiding this comment.
onInstallEnded(addonInstall, addon) {
...
}and the same below, please.
| }); | ||
| }; | ||
|
|
||
| function installObjectPromise(installObject) { |
There was a problem hiding this comment.
All this does is wrap installObjectCallback, which seems to have no other callers. Just put the contents of installObjectCallback straight into the promise function in here, then you don't need to pass reject/resolve around, either.
|
|
||
| function getInstallForURLPromise(url) { | ||
| return new Promise(function(resolve, reject) { | ||
| AddonManager.getInstallForURL(url, resolve, 'application/x-xpinstall'); |
There was a problem hiding this comment.
Can this throw, and/or can the callback be invoked with an error status? In either of those cases, I guess it should call reject() ? I would expect getInstallForURL to not always succeed...
There was a problem hiding this comment.
The callback has listeners attached to it which get onInstallFailed invoked. I call a reject() function in that case to signal that the installation failed.
| }); | ||
| }; | ||
|
|
||
| // Return a promise |
There was a problem hiding this comment.
Does this just want to be an async function?
There was a problem hiding this comment.
I don't think so - I'd rather be consistent with the use of promises instead of a mix of async functions and promise objects so that callers know what to expect.
| }); | ||
| }); | ||
| } | ||
| return uninstallPromise(addonId); |
There was a problem hiding this comment.
No point having an inner function here, just start the function with return new Promise(...). And again, maybe this just wants to be an async function?
There was a problem hiding this comment.
I changed this to be an immediately invoked function as I need to capture the addonId within the Promise using a closure.
| // Test that we can install an XPI from any URL | ||
| const dir = getChromeDir(getResolvedURI(gTestPath)); | ||
| dir.append("fixtures"); | ||
| dir.append("normandy.xpi"); |
There was a problem hiding this comment.
Can you check with someone more up-to-date with this stuff (than me) - maybe @Mossop - how this works with add-on signature checks?
There was a problem hiding this comment.
If normandy.xpi is unsigned then this will only work in automation in Firefox 55 and higher.
There was a problem hiding this comment.
@Mossop normandy.xpi was indeed unsigned.. I've updated the xpi so that it's signed now.
| installAddon(url) { | ||
| function getInstallForURLPromise(installUrl) { | ||
| return new Promise(function(resolve, reject) { | ||
| AddonManager.getInstallForURL(installUrl, resolve, "application/x-xpinstall"); |
There was a problem hiding this comment.
Note that getInstallForURL returns a promise, no need to create your own.
| * Return a promise that resolves to a success messsage if | ||
| * addon uninstall is successful. | ||
| */ | ||
| uninstallAddon(addonId) { |
There was a problem hiding this comment.
Making this an async function and using the promise returned by getAddonByID would make this a lot cleaner.
|
I could be wrong, because async functions are pretty new beasts, but my understanding is that there's no observable difference from the caller's perspective between an async function and a non-async function that returns a promise -- running either without |
Yep. The current guidelines for the shield-recipe-client is to prefer async/await functions over Promises where possible. The driver uses Promises directly mostly because it's using Promises from the sandbox to allow for sandbox code to access |
There was a problem hiding this comment.
Thanks for this! The others have already mentioned making these methods async/await, and because they're called by methods in the sandbox, you'll want to return promises that the sandbox can use. Luckily, there's a method on the SandboxManager class called wrapAsync that can be used to wrap an async function and return the right kind of promise for the sandbox. Something like this:
return {
installAddon: sandboxManager.wrapAsync(async function installAddon(installUrl) {
await blah;
return foo;
}, {cloneInto: true, cloneArguments: true}),
};I'm not sure if cloneInto and cloneArguments (see the docstring on wrapAsync) are necessary though, as the parameters going in and coming out seem to be strings. @Mossop: Are strings a thing we should be cloning when they pass a sandbox boundary?
The only other thing I noticed was that we could use some tests for failure states for these methods. We could use sinon to mock out addListener and uninstall on the Addon class' prototype, and just use a fake add-on ID to test if getAddonByID fails.
|
I don't think I can restructure the installAddon API as an async function. async functions just decorate a function so that the resolve method of a Promise is wrapped around the return value of the top level function.
The problem here is that the addon object's The decorator in the async function won't wrap the return value of the listener object because the final return value we care about is from within If we still really want to have the top level If you see a way around this - let me know. I haven't been able to figure out a nicer way to clean up this mess of callback functions. I'm going to try adding the |
Not as far as I know |
Note that you getInstallForURL also returns a promise, you don't need to use the callback if that is cleaner. |
|
You can still explicitly return a promise from an async function, try this in a console (with a window, so not node.js): async function a() { return new Promise(r => setTimeout(() => r(5), 1000)) }
a().then(arg => console.log(arg))will print 5 (not 'promise' or something like that). So, (untested) async installAddon(url) {
let installObj = await AddonManager.getInstallForURL(url, null, "application/x-xpinstall");
installObj.install();
return new Promise((resolve, reject) => installObj.addListener({
onInstallEnded(addonInstall, addon) {
resolve(addon.id);
},
onInstallFailed(addonInstall) {
reject(`AddonInstall error code: [${addonInstall.error}]`);
},
}));
}seems like it would work? |
crankycoder
force-pushed
the
features/driver-xpi-install
branch
from
7513345
to
fa14f41
Compare
Osmose
force-pushed
the
features/driver-xpi-install
branch
from
bc00ff8
to
86326df
Compare
|
CircleCI failure is #812, we can rebase once it's fixed. |
| onInstallFailed(addonInstall) { | ||
| reject(`AddonInstall error code: [${addonInstall.error}]`); | ||
| }, | ||
| onDownloadFailed(install) { |
There was a problem hiding this comment.
Nit: unused trailing argument. I'd expect eslint to complain about this...
| // Installing kicks off an asychronous process which tries to read the manifest.json | ||
| // to startup the addon. Note that onInstallEnded is triggered too early | ||
| // which is why we need this delay. | ||
| await new Promise(resolve => SimpleTest.executeSoon(resolve)); |
There was a problem hiding this comment.
Should we wait for something specific, e.g. an observer notification generated by the test add-on once install has actually happened? Otherwise this feels like it's waiting to go intermittent on infra...
There was a problem hiding this comment.
More generally, it feels odd that the addon manager doesn't provide something 'better' to wait for, where install is 'properly' finished, instead of only sort of... Maybe @Mossop can suggest something?
There was a problem hiding this comment.
I think this is really a problem with extensions/internal/XPIInstall.jsm and the way that checkPrompot fires off an async function right away. Using the observer doesn't work right now because the onInstallEnded listeners are triggered prior to the install actually finishing up.
| })); | ||
|
|
||
| add_task(withDriver(Assert, async function uninstallInvalidAddonId(driver) { | ||
| // Test that we can install an XPI from any URL |
There was a problem hiding this comment.
This comment is the same everywhere, and doesn't reflect the test. Please update it to remove confusion.
| const invalidAddonId = "not_a_valid_xpi_id@foo.bar"; | ||
| try { | ||
| await driver.uninstallAddon(invalidAddonId); | ||
| ok(false, `Uninstall returned an unexpected null for success`); |
There was a problem hiding this comment.
It would be helpful if this failure message more explicitly indicated why we expected failure, rather than just that we didn't expect success...
…nto the testcase to verify that the addon was properly installed by checking the value of the addonId
…e the closure around the Promise that wraps AddonManager.getAddonByID
…view * Reordered the listener and install() invocation to avoid possible race * double check the addonID after installation * added a timeout to let the XPI installation complete properly.
…en installing an addon added testcase to uninstall invalid addon id
dropped "return null"
crankycoder
force-pushed
the
features/driver-xpi-install
branch
from
86326df
to
bbd1bdb
Compare
crankycoder
force-pushed
the
features/driver-xpi-install
branch
from
bbd1bdb
to
dbb2b5a
Compare
|
@gijsk can we skip on #778 (comment) for now? To fix the race condition between install and uninstall correctly seems to be out of scope for this particular patch. |
We can skip it , but please file a bug against the add-on manager in bugzilla and reference it in a comment in the code. |
crankycoder
dismissed
gijsk’s stale review
Filed bug https://bugzilla.mozilla.org/show_bug.cgi?id=1373671 to address the race condition between install and uninstall of XPI
|
Osmose pointed out I hadn't formally approved since the last push of code - I approve! :) |
This tries to close off #748 by adding an install and uninstall hook into the normandy driver.
This patch does not currently pass test cases though - the uninstall of the XPI file causes an error somewhere down inside the Extensions.jsm module.
I could stand to use a second pair of eyes on this to see why this is breaking so horribly.
My fixture data for the XPI installer is the simplest possible webextension - just a manifest.json file with no JS code, icons or anything else. Just the manifest zipped into a file with an XPI extension.