1344285 - Multiple overrides error thrown even when extension has only one override.

Status: RESOLVED INVALID

(WebExtensions :: Frontend, defect, P2)

Description

[Matthew Wein [:mattw]]

This line always evaluates to true: http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-url-overrides.js#79. Therefore, the error is thrown regardless of how many overrides the extension has. The code should instead be evaluating manifest.chrome_url_overrides, and should throw only if it contains more than one non-null value.

Comment 1

[Matthew Wein [:mattw]]

Attachment: Bug 1344285 - Fix check for multiple overrides

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

Comment 2

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8843362 [details]
Bug 1344285 - Fix check for multiple overrides

https://reviewboard.mozilla.org/r/117124/#review118740

::: browser/components/extensions/ext-url-overrides.js:81
(Diff revision 1)
> -  if (Object.keys(overrides).length > 1) {
> +  let hasOverride = false;
> -    extension.manifestError("Extensions can override only one page.");
> -  }
> -
>    for (let page of Object.keys(overrides)) {
>      if (manifest.chrome_url_overrides[page]) {

can we just check the number of keys in manifest.chrome_url_overrides?

Do we have a test for this?  If not please add one, if we do, wondering why it didn't fail and it probably needs to be fixed.

Comment 3

[Matthew Wein [:mattw]]

Comment on attachment 8843362 [details]
Bug 1344285 - Fix check for multiple overrides

https://reviewboard.mozilla.org/r/117124/#review118740

> can we just check the number of keys in manifest.chrome_url_overrides?
> 
> Do we have a test for this?  If not please add one, if we do, wondering why it didn't fail and it probably needs to be fixed.

We can't just check the number of keys in `manifest.chrome_url_overrides` because the schema adds in the missing keys and normalizes them to `null`.
 
We do have tests for this - http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_url_overrides_all.js#77. We just don't have a test to make sure this error doesn't throw when only one override is requested. Do you think we should add one for that case, and if so, do you know how we would write one that does that?

Comment 4

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8843362 [details]
Bug 1344285 - Fix check for multiple overrides

https://reviewboard.mozilla.org/r/117124/#review118816

Comment 5

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8843362 [details]
Bug 1344285 - Fix check for multiple overrides

https://reviewboard.mozilla.org/r/117124/#review118740

> We can't just check the number of keys in `manifest.chrome_url_overrides` because the schema adds in the missing keys and normalizes them to `null`.
>  
> We do have tests for this - http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_url_overrides_all.js#77. We just don't have a test to make sure this error doesn't throw when only one override is requested. Do you think we should add one for that case, and if so, do you know how we would write one that does that?

Object.values(manifest.chrome_url_overrides).filter(v => v != null).length !== 1

But on reflection what you did is fine too.  I see that it wasn't an actual failure, just console output, so I'm fine with the test as is.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Bug 1341458 is actually backing out the home override patch, so this probably doesn't need to land unless Bug 1341458 doesn't land prior to uplift.

Comment 7

[Matthew Wein [:mattw]]

This bug is no longer valid now that bug 1298950 has been backed out.