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

RESOLVED INVALID

Status

()

Toolkit
WebExtensions: Frontend
P2
normal
RESOLVED INVALID
9 months ago
8 months ago

People

(Reporter: mattw, Assigned: mattw)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: triaged, URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

9 months ago
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 hidden (mozreview-request)

Comment 2

9 months ago
mozreview-review
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.
(Assignee)

Comment 3

9 months ago
mozreview-review-reply
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

9 months ago
mozreview-review
Comment on attachment 8843362 [details]
Bug 1344285 - Fix check for multiple overrides

https://reviewboard.mozilla.org/r/117124/#review118816
Attachment #8843362 - Flags: review?(mixedpuppy) → review+

Comment 5

9 months ago
mozreview-review-reply
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.
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.
(Assignee)

Comment 7

8 months ago
This bug is no longer valid now that bug 1298950 has been backed out.
Status: NEW → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.