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 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 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 on attachment 8843362 [details] Bug 1344285 - Fix check for multiple overrides https://reviewboard.mozilla.org/r/117124/#review118816

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.

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