1542351 - Non firefox browser_specific_settings entries lead to error during installation

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P5)

Description

[Christopher Grebs [:cgrebs]]

Attachment: Error message when installing an add-on with that property via about:debugging

An Add-on that adds the "edge" property to the "browser_specific_settings" in manifest.json leads to an error during installation.

According to https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/browser_specific_settings#Microsoft_Edge_properties this should be very well supported and users seem to try to make use of this kind of interoperability: https://github.com/mozilla/addons/issues/851#issuecomment-480259306

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Based on the API schema definition for the browser_specific_settings manifest property it looks that the reason for the install error is that we currently don't allow any additional properties in browser_specific_settings (besides "gecko" which is explicitly defined in the schema)

In some of the other manifest properties we explicitly include an "additionalProperties": { "$ref": "UnrecognizedProperty" } property to make the validation less strict about the presence of property that the schema may not know about (e.g. as we do here for the "background" manifest property:

• https://searchfox.org/mozilla-central/rev/8e0ea968308dd692ba5394b14e584234acf7d4ca/toolkit/components/extensions/schemas/manifest.json#120

Comment 2

[Caitlin Neiman [:caitmuenster]]

If this is your first contribution, please refer to https://wiki.mozilla.org/WebExtensions/Contribution_Onramp for how to get started.

Comment #1 describes what needs to be done.

Mentor: :rpl

Comment 3

[joseph.a.jalbert]

Hello! I would like to fix this bug. I am familiar w/ the WebExtensions APIs. I plan on submitting a patch soon-ish (next few days), just need to figure out the workflow/testing/ etc.
-Joe

Comment 4

[Caitlin Neiman [:caitmuenster]]

Hey Joe! Welcome! Please feel free to needinfo :rpl, the mentor for this bug, if you have any questions. We'll formally assign you to the bug once you have a patch up in Phabricator. :)

Comment 5

[Myeongjun Go]

Hello Joe.
If you don't try to solve this bug, Can I try to work on it?

Comment 6

[joseph.a.jalbert]

Hi, Myeongjun,
Yes of course, feel free. It took me quite some time to get the build working on my machine, and since then I've been too busy to work on it. Go for it!

Comment 7

[joseph.a.jalbert]

Hi again Myeongjun,
I actually have some free time now and so I'm going to try working on this. Please don't let this stop you though if you have already begun. It should be a very small fix anyway. If you get the patch submitted first, good for you!

Comment 8

[Myeongjun Go]

Thanks for reply!!
I don't knew if you try to work it.
I have worked this issue. But I think that you are first. You interested in this issue already and you have tried to work.
I ask you again joe :)

Comment 9

[joseph.a.jalbert]

Thank you Meyongjun. I have patched the bug and plan on submitting my patch today

Comment 10

[joseph.a.jalbert]

Attachment: Bug 1542351: Allow unrecognized properties in 'browser_specific_settings' manifest property for add-ons r=rpl

Comment 11

[Myeongjun Go]

I add comment that I think to Phabricator.
You can refer to my comment.

Comment 12

[joseph.a.jalbert]

Thank you for the feedback! I am updating the patch based on your suggestions

Comment 13

[joseph.a.jalbert]

Attachment: Bug 1542351: Add specific 'edge' property to 'browser_specific_settings' manifest property r=rpl

Depends on D32391

Comment 14

[joseph.a.jalbert]

Attachment: Bug 1542351: Unit test for installing add-on with MS Edge browser specific property r=rpl

Depends on D32746

Comment 15

[joseph.a.jalbert]

Attachment: Bug 1542351: Allow 'edge' and non-gecko browser specific settings in the extensions schema manifest r=rpl

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65a152806f06
Allow 'edge' and non-gecko browser specific settings in the extensions schema manifest r=rpl

Comment 17

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/65a152806f06

Comment 18

[Caitlin Neiman [:caitmuenster]]

Yay! Congrats on landing the patch, Joseph! 🎉 Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you!

Comment 19

[joseph.a.jalbert]

Thanks Caitlin! Yes, I would love to join to mozillians. Here is my profile url: https://mozillians.org/en-US/u/joseph.a.jalbert/

Comment 20

[Caitlin Neiman [:caitmuenster]]

Fabulous! You are vouched. Welcome onboard! We look forward to seeing you around the project. ✨

Comment 21

[Madalin Cotetiu]

Verified in Win7x64 and Ubuntu 14.0.4 FF70.0a1(20190709153742) and 69.0b3
I have created a test extension and in the manifest I have added the following lines:
"browser_specific_settings": {
"gecko": {
"id": "[e-mail]"
},
"edge": {
"browser_action_next_to_addressbar": true
}
}

The extension was successfully installed via about:debugging and no errors or warnings were displayed in the console.

Marking bug as verified fixed.

Comment 22

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 9067997 [details]
Bug 1542351: Allow 'edge' and non-gecko browser specific settings in the extensions schema manifest r=rpl

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: :mkaply pointed out this issue as something that we should nominate for uplift to 68ESR, as it would prevent Firefox from denying installation of extensions that contain a browser_specific_settings.edge property (and/or settings specific to other browsers).

The browser_specific_settings manifest property was already meant to be used by other browsers (to include their own specific manifest options) and so the Firefox JSON schema validations should have never considered unknown properties included in that section as "blocking validation errors".

The MDN docs for the browser_specific_settings manifest property also mention the edge property explicitly and it is part (along with the browser_specific_settings.gecko property) of the example we are providing on MDN: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/browser_specific_settings#Examples

• User impact if declined: An addon developer will not be able to include in the browser_specific_settings any properties related to other browsers (edge in particular) on an extension targeting both Firefox and another browser, despite the MDN docs are explicitly suggesting its usage.
• Fix Landed on Version: 69
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Very small change, only applied to the WebExtensions API schema (to relax the validation and only raise warning for unknown properties included in the browser_specific_settings manifest.json property).

The patch includes a new automated test, and the fix has been also explicitly verified by QA on Firefox 69.

• String or UUID changes made by this patch:

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9067997 [details]
Bug 1542351: Allow 'edge' and non-gecko browser specific settings in the extensions schema manifest r=rpl

Fixes a bug which can cause WebExtension interop issues. Approved for 68.1esr.

Comment 24

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

https://hg.mozilla.org/releases/mozilla-esr68/rev/649260006f28

Comment 25

[Irene Smith]

I have added the following notice to the page:

Warning: Adding Edge-specific properties to the manifest causes an error in Firefox 68 which can prevent an extension from installing. This is a known bug ({{bug(1542351)}}) in Firefox 68. It has been fixed in Firefox 69 and the fix will be uplifted to Firefox 68.

Please add a needs-info to this bug when the fix has been uplifted to 68 so I can remove the notice.

Thanks!

Comment 26

[Andrew Swan [:aswan]]

It looks like it has been uplifted to ESR so it will appear in ESR 68.2. I don't believe this fix will be uplifted to 68 release.

Comment 27

[Irene Smith]

(In reply to Andrew Swan [:aswan] from comment #26)

It looks like it has been uplifted to ESR so it will appear in ESR 68.2. I don't believe this fix will be uplifted to 68 release.

So the message should just state that it is an error and remove the comment about the fix being uplifted to 68 release?

Comment 28

[Andrew Swan [:aswan]]

I think a concise note that this didn't work prior to Firefox 69 / ESR 68.2 sounds reasonable.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

68.1esr, not 68.2.

Comment 30

[Irene Smith]

(In reply to Andrew Swan [:aswan] from comment #28)

I think a concise note that this didn't work prior to Firefox 69 / ESR 68.2 sounds reasonable.

I have changed the warning to:

Warning: Adding Edge-specific properties to the manifest caused an error prior to Firefox 69 which can prevent the extension from installing.

Thanks for responding so quickly.

Comment 31

[carlos]

Does this mean browser_specific_settings.gecko_android would also be uncompatible prior to Firefox 69 / ESR 68.2?

Comment 32

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to carlos from comment #31)

Does this mean browser_specific_settings.gecko_android would also be uncompatible prior to Firefox 69 / ESR 68.2?

yes, on such an old Firefox version an extension with a browser_specific_settings that those Firefox versions didn't know about would be triggering a manifest validation error instead of a manifest validation warning as in all more recent versions.

As a more general side note, which is unrelated to extensions compatibility specifically, it is really not advisable to navigate the web with a browser engine that old, security related fixes are not going to be backported that far in the past and it would potentially expose the user to security issues.

As an additional side note, questions about extensions development posted on a closed bug are more likely to go unnoticed, discourse.mozilla.org would be a better place to post questions (e.g. https://discourse.mozilla.org/c/add-ons/35 for addons related questions, and https://discourse.mozilla.org/c/add-ons/android/9393 for Firefox for Android extensions development specifically). On discourse, in addition to engineers from the Add-ons team, our DevRels are monitoring the activity and then collect and answer questions, and the other developers from the extensions developers community are also pretty active.