1269454 - Temporary install of web extension with options_ui.page fails

Status: RESOLVED DUPLICATE of bug 1293721

(WebExtensions :: Untriaged, defect)

Description

[Andrew Swan [:aswan]]

If a webextension has a ui_page and does not have an id in its manifest, installing it temporarily fails with the exception "getURL may not be called before `id` or `uuid` has been set".
The problem is that code in the manifest parsing (XPIProvider.loadManifestFromWebManifest in particular) tries to use getURL() to construct the options page URL, but getURL() relies on the extension uuid which is not assigned until later.

Proposed fix is to delay construction of the options page URL until it is needed.

Comment 1

[drugan]

Actually, an extension having an id in its manifest along with this entry

"options_ui": {"page": "options.html"}

and officially listed on AMO has neither [Preferences] button in Add-ons Manager Extensions list nor any preferences seen when you click on the extension More link. At the same time options.html could be successfully opened by background.js script through chrome.windows.create() and have all the functionality it is intended for.

Tested in Firefox 45, 46, 47, 48, 49 versions.

Comment 2

[Andrew Swan [:aswan]]

(In reply to drugan from comment #1)
> Actually, an extension having an id in its manifest along with this entry
> 
> "options_ui": {"page": "options.html"}
> 
> and officially listed on AMO has neither [Preferences] button in Add-ons
> Manager Extensions list nor any preferences seen when you click on the
> extension More link. At the same time options.html could be successfully
> opened by background.js script through chrome.windows.create() and have all
> the functionality it is intended for.
> 
> Tested in Firefox 45, 46, 47, 48, 49 versions.

This sounds like a separate issue, can you open a new bug and include the extension as an attachment please.

Comment 3

[drugan]

> This sounds like a separate issue, can you open a new bug and include the
> extension as an attachment please.

Done: https://bugzilla.mozilla.org/show_bug.cgi?id=1271895

Comment 4

[Andrew Swan [:aswan]]

Attachment: Bug 1269454 Make options_ui work with temporary addons

I started out trying to do something more general, following the
pattern we used for the debug global to associate a base URL with
the addon instance.  But I ran into problems -- the copying of
addon properties from AddonInternal to DBAddonInternal and the
ascyhronous nature of AddonManager.getInstanceByID() made that
approach a mess.  The solution in this patch is ad hoc, but I think
its the lesser of two evils.

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

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8756602 [details]
Bug 1269454 Make options_ui work with temporary addons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55274/diff/1-2/

Comment 6

[Kris Maglione [:kmag]]

Why can't we just assign the ID earlier?

Comment 7

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] from comment #6)
> Why can't we just assign the ID earlier?

Well this is the method that reads the manifest we don't know until we've read the manifest whether we'll need to assign a temporary ID or not.

We could speculatively allocate one before reading the manifest and pass it down in case we need one.  That feels worse to me, we'll sometimes be doing work we don't need to do.  Moreover (and this is more philosophical), I don't think a method to load the manifest should have side effects but as it stands right now, it has the side effect of allocating the local uuid for the extension (when we call getURL).  Nothing actually breaks because of that but it feels wrong, that should be happening later, ie when we install or activate the extension.

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8756602 [details]
Bug 1269454 Make options_ui work with temporary addons

https://reviewboard.mozilla.org/r/55274/#review64316

I don't think that this is a good approach. If we need to resolve the URI this late, then we should either add a method to the add-on path service to get the extension's root URL, or use some sort of observer to tell the add-on manager what the full options URL is.

Comment 11

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/55274/#review64316

> I don't think that this is a good approach.

This is not a helpful comment without some explanation.  But regardless, what is the "add-on path service"?

Comment 12

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

as an alternative approach to be able to resolve the option url late in the extension loading, how about the following:

- if the addon doesn't have an id:
  - set the optionsURL to a blank page
  - hook a function to the options ui attribute using extensions.on: e.g. extensions.on("manifest_options_ui", (...) => { ... } to resolve the URI properly, retrieve the private addon wrapper and through it set the optionsURL to the addon internal object

The "extensions.on" is already used in our internals for similar reasons (http://searchfox.org/mozilla-central/search?q=extensions.on%28%22manifest_&path=)

and it looks like a simpler approach then changing the AddonPathService for this purpose.

How it sounds to you, guys?

Comment 13

[Andrew Swan [:aswan]]

That seems good in principle but Management (aka "extensions") isn't exported and we would need to subscribe to these events from AddonManager (or XPIProvider to be precise).  We could export it, but I think it was meant to be internal to just webextensions..

I think in the longer run, we are going to want to give the add-ons manager access to Extension instances (for the management.get* methods for instance).  This patch doesn't do that in a fully general way but should be easy to adapt when we do that.

Comment 14

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

(In reply to Andrew Swan [:aswan] from comment #13)
> That seems good in principle but Management (aka "extensions") isn't
> exported and we would need to subscribe to these events from AddonManager
> (or XPIProvider to be precise).  We could export it, but I think it was
> meant to be internal to just webextensions..
 
Actually what I meant was: to use `extensions.on` in a `ext-*.js` file (where it is already available e.g. even a new `ext-optionsPage.js` if none of the existent is a good pick for this handler), and from there get the private addon wrapper, e.g. like the ext-backgroundPage.js currently does to set the debug global at https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ext-backgroundPage.js#101

Comment 15

[Andrew Swan [:aswan]]

Comment on attachment 8756602 [details]
Bug 1269454 Make options_ui work with temporary addons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55274/diff/2-3/

Comment 16

[Andrew Swan [:aswan]]

I don't love any of the proposed alternatives.  The AddonPathService was a new one for me but I can't actually find any in-tree uses of it other than tests.  It also doesn't do anything like what we need, but it doesn't seem like a great place to be adding new code.
The event/observer approach also doesn't seem right, if those events come from within the webextensions code, they'll be triggered every time the extension is loaded but we just care about getting the options page one time when the extension is installed.

I've attached an even simpler version of the patch with less funny special-case stuff in XPIProvider.jsm.

Comment 17

[Kris Maglione [:kmag]]

Sorry, I lost track of this.

(In reply to Andrew Swan [:aswan] from comment #11)
> This is not a helpful comment without some explanation.

There are a few reasons.

1) It changes the contract of the bootstrap.js startup function in a way that
I don't think really makes sense. I've also been seriously considering
allowing those functions to return promises to notify the add-on manager when
their main startup/shutdown code is finished, and this would conflict with
that.

2) It couples the WebExtension runtime code more tightly with the AOM code
than I'd like, and in ways that we've generally avoided doing so far.

3) It gives us multiple code paths for resolving options URLs, and multiple
ways they may wind up stored in the extensions DB.

Also, honestly, it just feels like a hack.

> But regardless, what is the "add-on path service"?

It's a service that maps internal URLs to an extension ID, which could easily
be extended to resolve paths within an extension to a canonical URL.

(In reply to Andrew Swan [:aswan] from comment #16)
> The event/observer approach also doesn't seem right, if those events come
> from within the webextensions code, they'll be triggered every time the
> extension is loaded but we just care about getting the options page one time
> when the extension is installed.

We already dispatch observer notifications when when add-on detail panes are
opened, and existing extensions use them to manage their inline options. We
could easily do the same to provide the AOM with the resolved URL of the
options page.

Comment 18

[FayneAldan]

This bug also seems to be affecting preliminarily reviewed addons on AMO.
"1470670883856	addons.xpi	WARN	Download of https://addons.mozilla.org/firefox/downloads/file/480060/plaza-4.7.1-fx+an.xpi?src=dp-btn-primary failed: Error: getURL may not be called before an `id` or `uuid` has been set (resource://gre/modules/Extension.jsm:764:13) JS Stack trace: [e-mail]:764:13 < loadManifestFromWebManifest<@XPIProvider.jsm:913:24 < [e-mail]:319:40 < [e-mail]:937:23 < [e-mail]:816:7 < this.PromiseWalker.scheduleWalkerLoop/<@Promise-backend.js:750:11"

Comment 19

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] from comment #17)
> 1) It changes the contract of the bootstrap.js startup function in a way that
> I don't think really makes sense. I've also been seriously considering
> allowing those functions to return promises to notify the add-on manager when
> their main startup/shutdown code is finished, and this would conflict with
> that.

This would only conflict superficially, the promise could resolve to the extension instance.

> 2) It couples the WebExtension runtime code more tightly with the AOM code
> than I'd like, and in ways that we've generally avoided doing so far.

Have you looked at the more recent patch?  There's still some coupling, but it is less than before.

> 3) It gives us multiple code paths for resolving options URLs, and multiple
> ways they may wind up stored in the extensions DB.

I think that's the nature of the problem here, we can't resolve the options URL until the extension is installed.  Unless we change when and how we get the options URL for all types of extensions, I think we're stuck with the multiple paths.

> Also, honestly, it just feels like a hack.

That's fair.  But I think that landing this as-is and revisiting the question of coupling between AOM and webextensions with future patches (I don't think it will be long)

> > The event/observer approach also doesn't seem right, if those events come
> > from within the webextensions code, they'll be triggered every time the
> > extension is loaded but we just care about getting the options page one time
> > when the extension is installed.
> 
> We already dispatch observer notifications when when add-on detail panes are
> opened, and existing extensions use them to manage their inline options. We
> could easily do the same to provide the AOM with the resolved URL of the
> options page.

Sorry I didn't follow this, can you point me to an example?

Comment 20

[Kris Maglione [:kmag]]

(In reply to Andrew Swan [:aswan] from comment #19)
> (In reply to Kris Maglione [:kmag] from comment #17)
> > We already dispatch observer notifications when when add-on detail panes are
> > opened, and existing extensions use them to manage their inline options. We
> > could easily do the same to provide the AOM with the resolved URL of the
> > options page.
> 
> Sorry I didn't follow this, can you point me to an example?

http://searchfox.org/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/toolkit/mozapps/extensions/content/extensions.js#3350
https://dxr.mozilla.org/addons/source/11378/resources/addon-sdk/lib/sdk/l10n/prefs.js
http://searchfox.org/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/addon-sdk/source/lib/sdk/preferences/native-options.js#39

Comment 21

[Rob Wu [:robwu]]

Comment on attachment 8756602 [details]
Bug 1269454 Make options_ui work with temporary addons

https://reviewboard.mozilla.org/r/55274/#review68218

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4000
(Diff revision 3)
>      AddonManagerPrivate.callAddonListeners("onInstalling", addon.wrapper,
>                                             false);
> -    XPIProvider.callBootstrapMethod(addon, file, "startup",
> +    let startupResult = XPIProvider.callBootstrapMethod(addon, file, "startup",
> -                                    BOOTSTRAP_REASONS.ADDON_ENABLE);
> +                                                        BOOTSTRAP_REASONS.ADDON_ENABLE);
> +    if (addon.type == "webextension") {
> +      addon.optionsURL = startupResult.optionsURL;

`XPIProvider.callBootstrapMethod` may return without returning a value. If that happens then this line will throw.
If you want to keep using the return value in the current or next iteration of your patch, use `let startUpResult = ... || {};` or `startupResult && startupResult.optionsURL` to fix this.