Non firefox browser_specific_settings entries lead to error during installation

VERIFIED FIXED in Firefox -esr68

Status

defect
P5
normal
VERIFIED FIXED
5 months ago
20 days ago

People

(Reporter: cgrebs, Assigned: joseph.a.jalbert, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla69
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr68 fixed, firefox69 verified)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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

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:

Priority: -- → P3
Keywords: good-first-bug
Priority: P3 → P5

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

Mentor: lgreco

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

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. :)

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

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!

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!

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 :)

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

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

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

Attachment #9067842 - Attachment is obsolete: true
Attachment #9067841 - Attachment is obsolete: true
Attachment #9067185 - Attachment is obsolete: true
Assignee: nobody → joseph.a.jalbert
Status: NEW → ASSIGNED
Keywords: checkin-needed

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

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

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!

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

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

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": "addon@example.com"
},
"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.

Status: RESOLVED → VERIFIED

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:
Attachment #9067997 - Flags: approval-mozilla-esr68?
Flags: in-testsuite+

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.

Attachment #9067997 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

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!

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.

Flags: needinfo?(ismith)

(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?

Flags: needinfo?(ismith) → needinfo?(aswan)

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

Flags: needinfo?(aswan)

68.1esr, not 68.2.

(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.

You need to log in before you can comment on or make changes to this bug.