1306037 - Embedded WebExtensions do not support options_ui manifest properties

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[noitidart]

Attachment: Screenshot showing missing options button.

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20160920155715

Steps to reproduce:

I created embedded webext. I set in manifest.json:

https://github.com/Noitidart/Profilist/blob/master/webextension/manifest.json#L7-L10

	"options_ui": {
		"page": "pages/options.html",
		"open_in_tab": true
	},


Actual results:

No button is shown for options page in about:addons please see attached screenshot.


Expected results:

Options button should show and it should open:
moz-extension://a55063ac-4f0b-4fe8-b3d2-95b8217fb8c8/pages/options.html

Comment 1

[noitidart]

For now I am using:

	"icons": {
		"16": "images/group.svg",
		"48": "images/group.svg",
		"64": "images/group.svg",
		"96": "images/group.svg",
		"128": "images/group.svg"
	},

I'm not sure what sizes to provide there, docs only really state what 48 is used for. Per the docs, I'm not sure if any of the other sizes have any application. I'm thinking 96 is used as 2x where HDPI is present.

Comment 2

[Kris Maglione [:kmag]]

[Tracking Requested - why for this release]:

The transparent embedded WebExtension feature was introduced in Firefox 51, and developers should reasonably expect this feature to work with it.

Comment 3

[Kris Maglione [:kmag]]

*was introduced in Firefox 51 by bug 1269342.

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1306037: Support options_ui in embedded WebExtensions. f?rpl

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

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8795908 [details]
Bug 1306037: Support options_ui in embedded WebExtensions.  f?rpl

https://reviewboard.mozilla.org/r/81874/#review80474

::: testing/specialpowers/content/SpecialPowersObserverAPI.js:623
(Diff revision 1)
> +            // readManifest() will throw if we're loading an embedded
> +            // extension, so don't worry about locale errors in that
> +            // case.

Why does it throw on an embedded extension?  Can we catch just the specific error in that case so other errors don't get swallowed here?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1172
(Diff revision 1)
> +
> +    if (addon.hasEmbeddedWebExtension) {
> +      let uri = NetUtil.newURI("webextension/manifest.json", null, aUri);
> +      let embeddedAddon = yield loadManifestFromWebManifest(uri);
> +      if (embeddedAddon.optionsURL) {
> +        addon.optionsURL = embeddedAddon.optionsURL;

Maybe log a warning or even generate an error here if both halves of the addon have options urls?

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8795908 [details]
Bug 1306037: Support options_ui in embedded WebExtensions.  f?rpl

https://reviewboard.mozilla.org/r/81874/#review80474

> Why does it throw on an embedded extension?  Can we catch just the specific error in that case so other errors don't get swallowed here?

Because the path to the manifest is wrong. This code is redundant in extensions loaded by the add-on manager, and if `readManifest` throws here, we'll get the same error when we try to load the same extension, so we're not losing any important errors here.

Comment 7

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc3137d7d869992394745738791cadfc23a927b2
Bug 1306037: Support options_ui in embedded WebExtensions. r=aswan

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bc3137d7d869

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8795908 [details]
Bug 1306037: Support options_ui in embedded WebExtensions.  f?rpl

Approval Request Comment
[Feature/regressing bug #]: Bug 1269342
[User impact if declined]: Extensions which attempt to provide options to users via the options_ui feature of embedded WebExtensions will fail to do so in versions prior to 52.
[Describe test coverage new/current, TreeHerder]: The related features are well-covered by existing tests, and new tests were added for this change.
[Risks and why]: Low. The changes required to implement this are fairly simple, and should only affect embedded WebExtensions. The changes outside of XPIProvider.jsm are all test-only.
[String/UUID change made/needed]: None.

Comment 10

[Gerry Chang [:gchang]]

Hi Florin, can we have some QA's help to verify this?

Comment 11

[Valentina Virlics [:vvalentina], Ecosystem QA]

Verified this on Windows 7 - x64, using the "Install from File" installation method from about:addons and obtained the following results:

1. Nightly (Version 52.0a1, Build ID 20160928065829)
Embedded web-extension is installed but no "Options" button listed in about:addons -> http://screencast.com/t/wc1DEjgtS2g
Browser console errors: 
- https://pastebin.mozilla.org/8915296
- https://pastebin.mozilla.org/8915297

2. Nightly (Version 52.0a1, Build ID 20161002030227)
Embedded web-extension cannot be installed; corrupt message displayed;
Browser console errors at install: 
- https://pastebin.mozilla.org/8915293 
- https://pastebin.mozilla.org/8915294

Attaching used embedded web-extension. (is the one mentioned at comment #0, but with other versions range)

Kris Maglione [:kmag] : Is the "locales" error from the browser console a result of this fix or I missed something out?

Comment 12

[Valentina Virlics [:vvalentina], Ecosystem QA]

Attachment: Profilist-t.xpi

Comment 13

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

Hi Valentina,
the error related to the locales is due to a different issue: Bug 1206858 " Localizations for packed embedded webextensions searched for in extension root". 

Bug 1206858 has already a patch attached, but the fix is not landed yet, to be able to test the fix related to this issue without waiting for that fix, you can workaround the issue by:

- unpacking the above xpi
- copy the _locales dir into the unpacked add-on root dir
- repack the modified addon into a new xpi file

Comment 14

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

I tried the attached addon with the above workaround applied, but unfortunately there are still a scenario where preferences from the embedded webextension can still fail to be shown in the "about:addons" page:

The reason is that, with the fix applied by the patch attached to this issue, the addon preferences can be successfully rendered only once the embedded webextension has been started, and the attached addon currently doesn't call the `startup()` method if the promise returned by `installNativeMessaging()` is rejected (and it works if the bootstrap.js file is changed so that the embedded webextension is started even if the above promise is rejected).

(If the embedded webextension is not started, the preferences are not shown because during the rendering of the "about:addons" page, the `getURLForExtension` method raises a "Called getURLForExtension on unmapped extension Profilist@jetpack  ExtensionManagement.jsm:121" exception, because the addon uuid is allocated and stored for a webextension only when the it is actually started).

Comment 15

[Valentina Virlics [:vvalentina], Ecosystem QA]

Hi Luca and thanks for your quick reply!

So, we should wait for the fix from https://bugzilla.mozilla.org/show_bug.cgi?id=1206858 to land? will that fix catch the other scenario you found, that fails?

Comment 16

[noitidart]

(In reply to Luca Greco [:rpl] from comment #14)
> I tried the attached addon with the above workaround applied, but
> unfortunately there are still a scenario where preferences from the embedded
> webextension can still fail to be shown in the "about:addons" page:
> 
> The reason is that, with the fix applied by the patch attached to this
> issue, the addon preferences can be successfully rendered only once the
> embedded webextension has been started, and the attached addon currently
> doesn't call the `startup()` method if the promise returned by
> `installNativeMessaging()` is rejected (and it works if the bootstrap.js
> file is changed so that the embedded webextension is started even if the
> above promise is rejected).
> 
> (If the embedded webextension is not started, the preferences are not shown
> because during the rendering of the "about:addons" page, the
> `getURLForExtension` method raises a "Called getURLForExtension on unmapped
> extension Profilist@jetpack  ExtensionManagement.jsm:121" exception, because
> the addon uuid is allocated and stored for a webextension only when the it
> is actually started).

Thanks Luca for testing the attachment so deeply! I think it makes sense that if the webext is not started then the options button does not show. Clicking the options button would open a webext page. So if the webext didn't start, then showing the options button would open an "error" page.

So it should probably be on me to handle it when native messaging fails to install. The addon completely relies on that (options affect across all profiles, so are stored in profiles.ini). If it fails to install, I'll have to give the failure reason to the user. The user can then post an issue on my bug tracker and I can troubleshoot with them.

Comment 17

[Gerry Chang [:gchang]]

Comment on attachment 8795908 [details]
Bug 1306037: Support options_ui in embedded WebExtensions.  f?rpl

Fix a Web Extension related issue which will be in 51. Take it in 51 aurora.

Comment 18

[Gerry Chang [:gchang]]

Track 51+ as this feature will be in 51

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ee406a2f17c1

Comment 20

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

(In reply to ValentinaP from comment #15)
> Hi Luca and thanks for your quick reply!
> 
> So, we should wait for the fix from
> https://bugzilla.mozilla.org/show_bug.cgi?id=1206858 to land? will that fix
> catch the other scenario you found, that fails?

My apologies, the bug linked in my previous comment contains a typo, the right bugzilla issue is Bug 1306858.

Bug 1306858 has now been landed and uplifted to 51 but, as I described in Comment 14 (and noitidart confirmed by Comment 16), the version of the addon currently attached to this issue doesn't start the embedded webextension if the promise returned by installNativeMessaging is rejected, and in this scenario the preferences button is not rendered.

I'm going to attach an xpi with a simple test addon.

Comment 21

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

Attachment: test-options-ui-in-embedded-webextension.xpi

xpi file of a simple test addon with a options_ui page defined in the embedded webextension manifest.

Comment 22

[Valentina Virlics [:vvalentina], Ecosystem QA]

(In reply to Luca Greco [:rpl] from comment #21)
> Created attachment 8798033 [details]
> test-options-ui-in-embedded-webextension.xpi
> 
> xpi file of a simple test addon with a options_ui page defined in the
> embedded webextension manifest.

Thanks a lot, Luca!
Using your add-on I've tested on:
1. Nightly (v. 52.0a1 - Build ID 20161005030211) 
2. Aurora (v. 51.0a2 - Build ID 20161005004013)
For both builds the "Options" button are displayed, but only after page refresh: http://screencast.com/t/ScUGxxChzXrz

Should it be listed directly after install, or it's ok like this?

Comment 23

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

(In reply to ValentinaP from comment #22)
> (In reply to Luca Greco [:rpl] from comment #21)
> > Created attachment 8798033 [details]
> > test-options-ui-in-embedded-webextension.xpi
> > 
> > xpi file of a simple test addon with a options_ui page defined in the
> > embedded webextension manifest.
> 
> Thanks a lot, Luca!
> Using your add-on I've tested on:
> 1. Nightly (v. 52.0a1 - Build ID 20161005030211) 
> 2. Aurora (v. 51.0a2 - Build ID 20161005004013)
> For both builds the "Options" button are displayed, but only after page
> refresh: http://screencast.com/t/ScUGxxChzXrz
> 
> Should it be listed directly after install, or it's ok like this?

the above screencast seems to present the expected behavior when this fix is applied.

Unfortunately if the about:addons page is already loaded in a tab when the addon is installed, the preferences button will not be shown until the about:addons page has been reloaded, for a reason similar to the one described in comment 14 (the about:addons page is opened, it is refreshed once the addon has been installed, but the embedded webextension has not been started yet)

Comment 24

[Marcia Knous [:marcia]]

Tracking - for 52 and I think this fix is covered.

Comment 25

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

This change is not yet covered in the MDN docs related to the "Embedded WebExtensions" feature (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Embedded_WebExtensions), in particular the following details have to be mentioned/changed in the updated docs:

- the "option_ui" from the Embedded WebExtension is now supported (e.g. where the current page says "For example, the add-on's preferences UI will still use the old system", it should become more like "can still use the old system", and, even if it is unrelated, the link in the MDN page related to the above text paragraph currently points to a non-existent section of the same page)

- when specified, the "option_ui" from the Embedded WebExtension manifest will override the Addon Preference UI (at install time, and even if the legacy hybrid addon has its own preferences UI configured in the install.rdf)

- the "option_ui" from the Embedded WebExtension will work only (and in the "about:addons" page the Preferences button related to the addon will be visible) if the embedded webextension has been started (in other words, the "embeddedWebExtension.startup()" method should have been called at least once)

as known issues of the current implementation of the "option_ui" from the Embedded WebExtension:

- if the embedded webextension is not started, in the "about:addons" page, the Preferences button related to the hybrid addon will never be visible (an exception logged is logged in the browser console)

- if the "about:addons" page is already opened in a tab when the hybrid addon with the option_ui in the embedded webextension is installed, the Preferences button will not be visible until the next reload of the "about:addons" page (and the same exception message from the above known issue is logged in the browser console)

Comment 26

[Florin Mezei, QA (:FlorinMezei)]

Based on comment #22, #23 and #25, there doesn't seem to be any additional verification needed here, so marking this as Verified.

Comment 27

[Will Bamberg [:wbamberg]]

I've added(In reply to Luca Greco [:rpl] from comment #25)
> This change is not yet covered in the MDN docs related to the "Embedded
> WebExtensions" feature

I removed the note you referred to, since it seems to be no longer true. I also added:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Embedded_WebExtensions#Preferences

...which I hope covers the current situation. Please let me know if it looks OK.

Comment 28

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

Thanks Will, the updated Preferences section looks great!

There is something that I'd like to mention in the above section if possible (I mean if we can find a formula that doesn't make it more confusing):

if the embedded webextension defines an option_ui page, the `startup` method should be called synchronously during the initial addon load (I mean "without waiting something asynchronous to happen"), so that the Preferences button can immediatelly appear (and work correctly), like it does for a regular webextension.

what do you think?

Comment 29

[Will Bamberg [:wbamberg]]

OK, I've tried to clarify this here: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Embedded_WebExtensions.

Marking as dev-doc-complete, but do please let me know if I need anything else.

Comment 30

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

It looks great, thanks Will!