1355334 - Exception in XPIProvider when manifest validation fails

Status: RESOLVED FIXED

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

Description

[Andrew Swan [:aswan]]

When attempting to install a webextension that fails manifest validation we now get this uncaught exception:

A coding exception was thrown and uncaught in a Task.

Full message: TypeError: manifest is undefined
Full stack: loadManifestFromWebManifest<@resource://gre/modules/addons/XPIProvider.jsm:966:16
...

I think this was introduced by a combination of bug 1330349 and bug 1344590.  Prior to either of those bugs, loadManifestFromWebManifest() checked extension.errors before looking at the returned manifest.  Bug 1330349 added code that looks at manifest.theme before checking whether the manifest is valid, but this worked by accident (since ExtensionData.loadManifest() used to return the non-normalized parsed manifest json if validation failed).  But 1344590 changes loadManifest() to sensibly return undefined if validation fails, so now the code added with bug 1330349 blows up.

Comment 1

[Andrew Swan [:aswan]]

Attachment: Bug 1355334 Bail out immediately on manifest errors when parsing a webextension

Review commit: https://reviewboard.mozilla.org/r/128730/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128730/

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8856805 [details]
Bug 1355334 Bail out immediately on manifest errors when parsing a webextension

https://reviewboard.mozilla.org/r/128730/#review131240

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:990
(Diff revision 1)
>    }
>  
>    let addon = new AddonInternal();
>    addon.id = bss.id;
>    addon.version = manifest.version;
> -  addon.type = "webextension" + (theme ? "-theme" : "");
> +  addon.type = "webextension" + (!!manifest.theme ? "-theme" : "");

Nit: No need for `!!`. Don't we have an ESLint rule for this?

Comment 3

[Andrew Swan [:aswan]]

Comment on attachment 8856805 [details]
Bug 1355334 Bail out immediately on manifest errors when parsing a webextension

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128730/diff/1-2/

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b3a538e17a1
Bail out immediately on manifest errors when parsing a webextension r=kmag

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for eslint failure:

https://hg.mozilla.org/integration/autoland/rev/5e84ca460bb091a068998cf65aee74dca49132ed

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6b3a538e17a16daefed8f46f15e3da393ce1897a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=90592484&repo=autoland

> TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/mozapps/extensions/internal/XPIProvider.jsm:1075:24 | 'theme' is not defined.

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8856805 [details]
Bug 1355334 Bail out immediately on manifest errors when parsing a webextension

https://reviewboard.mozilla.org/r/128730/#review131592

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:969
(Diff revision 2)
>    let manifest = yield extension.loadManifest();
> -  let theme = !!manifest.theme;
>  
> +  // If there were any errors loading the extension, bail out now.
> +  if (extension.errors.length) {
> +    throw new Error("Extension is invalid");

Urgh. Mozreview. Sorry, apparently I lost a comment.

We need to do this after `initAllLocales`, or locale errors won't cause the installation to fail. We could also do it *before* the locale initialization, but then developers would have to fix all manifest errors before they learn about locale errors, which would be unfortunate.

I thought we had tests for this, but I guess nothing fails after these changes?

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8856805 [details]
Bug 1355334 Bail out immediately on manifest errors when parsing a webextension

https://reviewboard.mozilla.org/r/128730/#review131668

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:969
(Diff revision 2)
>    let manifest = yield extension.loadManifest();
> -  let theme = !!manifest.theme;
>  
> +  // If there were any errors loading the extension, bail out now.
> +  if (extension.errors.length) {
> +    throw new Error("Extension is invalid");

But after your recent changes, ExtensionData.manifest is null (or undefined, i don't remember which) if normalization fails so we can't check locales, that requires the default_locale property from the manifest...

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8856805 [details]
Bug 1355334 Bail out immediately on manifest errors when parsing a webextension

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128730/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4045456954bd
Bail out immediately on manifest errors when parsing a webextension r=kmag

Comment 10

[Kris Maglione [:kmag]]

We just need to check whether `this.manifest` is null from the `defaultLocale` getter.

Comment 11

[Andrew Swan [:aswan]]

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

Comment 12

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] from comment #10)
> We just need to check whether `this.manifest` is null from the
> `defaultLocale` getter.

This comment crossed mid-air with a push of a different fix.
We can change it in a followup if you like, but I think that saying you can't call initAllLocales() if you don't have a valid manifest is a reasonable thing...

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/4045456954bd