Closed Bug 1285314 Opened 3 years ago Closed 3 years ago

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

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox49 verified, firefox50 verified)

VERIFIED FIXED
mozilla50
Tracking Status
firefox49 --- verified
firefox50 --- verified

People

(Reporter: rpl, Assigned: rpl)

References

Details

(Whiteboard: triaged)

Attachments

(1 file)

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
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
(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.
(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.
(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 on attachment 8768874 [details]
Bug 1285314 - Fix extension id overridden with undefined when missing from the manifest.

https://reviewboard.mozilla.org/r/62910/#review59820
Attachment #8768874 - Flags: review?(aswan) → review+
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
Whiteboard: triaged
Duplicate of this bug: 1286029
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/6bd7cb39abaf
Fix extension id overridden with undefined when missing from the manifest. r=aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6bd7cb39abaf
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee: nobody → 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
Attachment #8768874 - Flags: approval-mozilla-aurora?
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.
Attachment #8768874 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1300304
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.