1285314 - Exception raised on browserAction creation for temporary installed addons without an id in the manifest

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect)

Description

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

STR:
- install as temporary addon an addon, its manifest contains a browserActor but doesn't have a explicitly assigned it

Expected result:
- the addon is installed correctly and the browserAction is visible in the toolbar

Actual result:
- the addon is installed but there is no browserAction visible in the toolbar
- the following exception is raised in the console (if we fix temporarely the "console is undefined issue" in the event-emitter module, which hide the real exception and its stacktrace):

TypeError: id is null: global.makeWidgetId@chrome://browser/content/ext-utils.js:27:3
BrowserAction@chrome://browser/content/ext-browserAction.js:26:18
@chrome://browser/content/ext-browserAction.js:233:23
emit@resource://devtools/shared/event-emitter.js:164:13
emit@resource://gre/modules/Extension.jsm:230:29
runManifest@resource://gre/modules/Extension.jsm:1358:9
startup/<@resource://gre/modules/Extension.jsm:1430:14
promise callback*startup@resource://gre/modules/Extension.jsm:1406:12
startup@resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/WebExtensionBootstrap.js:18:3
this.XPIProvider.callBootstrapMethod@resource://gre/modules/addons/XPIProvider.jsm:4782:9
this.XPIProvider.installTemporaryAddon<@resource://gre/modules/addons/XPIProvider.jsm:3988:5
TaskImpl_run@resource://gre/modules/Task.jsm:319:40
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:714:7
verifyDirSignedState/</callback.verifySignedDirectoryFinished@resource://gre/modules/addons/XPIProvider.jsm:1783:9

Comment 1

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

It seems that the addon has already been assigned an id (which is coming from addonData.id):

http://searchfox.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#1042

but it is overridden with an undefined value, in the Extension constructor:

http://searchfox.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#894

Comment 2

[Kris Maglione [:kmag]]

(In reply to Luca Greco [:rpl] from comment #1)
> It seems that the addon has already been assigned an id (which is coming
> from addonData.id):
> 
> http://searchfox.org/mozilla-central/source/toolkit/components/extensions/
> Extension.jsm#1042
> 
> but it is overridden with an undefined value, in the Extension constructor:
> 
> http://searchfox.org/mozilla-central/source/toolkit/components/extensions/
> Extension.jsm#894

It will only be overwritten if the manifest contains an `applications.gecko` object, which I suppose should probably still be fixed.

I think that the real problem you're running into, though, is bug 1284407.

Comment 3

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

Attachment: Bug 1285314 - Fix extension id overridden with undefined when missing from the manifest.

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

Comment 4

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

(In reply to Kris Maglione [:kmag] from comment #2) 
> It will only be overwritten if the manifest contains an `applications.gecko`
> object, which I suppose should probably still be fixed.

in the attached patch I've added a check that override the id only if it has not been assigned yet and it is not undefined in the manifest.

> I think that the real problem you're running into, though, is bug 1284407.

No, it's not. I've installed a temporary addon as a directory, the id is generated and then overwritten.

Comment 5

[Kris Maglione [:kmag]]

(In reply to Luca Greco [:rpl] from comment #4)
> (In reply to Kris Maglione [:kmag] from comment #2) 
> > I think that the real problem you're running into, though, is bug 1284407.
> 
> No, it's not. I've installed a temporary addon as a directory, the id is
> generated and then overwritten.

OK. I couldn't reproduce this problem when installing from a directory.

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8768874 [details]
Bug 1285314 - Fix extension id overridden with undefined when missing from the manifest.

https://reviewboard.mozilla.org/r/62910/#review59820

Comment 7

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

Push to try (which looks good, it has some oranges which seems totally unrelated):

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6f91f927c412bff2f7b47ee771dcaf4794b0bdf&selectedJob=23559413

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/6bd7cb39abaf
Fix extension id overridden with undefined when missing from the manifest. r=aswan

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6bd7cb39abaf

Comment 11

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

Comment on attachment 8768874 [details]
Bug 1285314 - Fix extension id overridden with undefined when missing from the manifest.

Approval Request Comment
[Feature/regressing bug #]: Bug 1285314

[User impact if declined]:
The patch fix a regression which happens for webextensions add-ons without an explicit addon id in the manifest.json, in particular on webextensions add-ons with a manifest which:
- defines a browser_action,
- do not have an extension id in the manifest
- but specifies its applications.gecko.strict_min_version/strict_max_version
An exception is raised on the browserAction creation and the addon will fail to load and it will fail to work correctly.
The above issue will have a greater impact once more addons are going to be submitted without an extension id in the manifest.

[Describe test coverage new/current, TreeHerder]:
No new tests are added by this patch, the existent testing coverage ensures that the patch doesn't break the expected behavior when the id is assigned explicitly through the manifest.json properties. 
- try push on aurora:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2573ef1d20f9

[Risks and why]:
The change applied has been kept small to keep the risks as low as possible. The change has an impact restricted to webextensions add-ons, in particular webextensions add-ons without an id specified in their manifest.json, and (as described above) the existent testing coverage ensures that the patch doesn't break the expected behavior when the id is assigned explicitly through the manifest.json properties.

[String/UUID change made/needed]: none

Comment 12

[Gerry Chang [:gchang]]

Comment on attachment 8768874 [details]
Bug 1285314 - Fix extension id overridden with undefined when missing from the manifest.

This patch fixes a webextensions add-ons regression. Take it in aurora.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7a7e5f80e25e

Comment 14

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce the initial issue on Firefox 50.0a1 (2016-07-02) under Windows 10 64-bit.

Verified fixed on Firefox 50.0a2 (2016-08-25) and Firefox 49 beta 6 (20160822111414) under Windows 10 64-bit. The “console is not defined” browser console error is no longer displayed for a webextenion which does not have an extension id in the manifest but specifies its applications.gecko.strict_min_version/strict_max_version.