Signed add-on installation fails with `getURL may not be called before an `id` or `uuid` has been set

VERIFIED FIXED

Status

()

P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: TheOne, Assigned: aswan)

Tracking

48 Branch
Points:
---

Firefox Tracking Flags

(firefox48- wontfix, firefox49+ verified, firefox50+ verified, firefox51+ fixed)

Details

(Whiteboard: triaged, URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
The installation of `Tibia Online Status` (see url) version 0.0.12 is failing with:

1470765095546	addons.xpi	DEBUG	Download started for https://addons.mozilla.org/firefox/downloads/latest/tibia-online-status/addon-539902-latest.xpi?src=dp-btn-primary to file /Users/awagner/Library/Caches/TemporaryItems/tmp-epb.xpi
1470765095549	addons.xpi	DEBUG	Download of https://addons.mozilla.org/firefox/downloads/latest/tibia-online-status/addon-539902-latest.xpi?src=dp-btn-primary completed.
1470765095574	addons.webextension.<unknown>	WARN	Loading extension 'null': Reading manifest: Error processing background.persistent: Event pages are not currently supported. This will run as a persistent background page.
1470765095578	addons.xpi	WARN	Download of https://addons.mozilla.org/firefox/downloads/latest/tibia-online-status/addon-539902-latest.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: getURL@Extension.jsm:764:13 < loadManifestFromWebManifest<@XPIProvider.jsm:913:24 < TaskImpl_run@Task.jsm:319:40 < Handler.prototype.process@Promise-backend.js:937:23 < this.PromiseWalker.walkerLoop@Promise-backend.js:816:7 < this.PromiseWalker.scheduleWalkerLoop/<@Promise-backend.js:750:11
1470765095583	addons.xpi	DEBUG	downloadFailed: removing temp file for https://addons.mozilla.org/firefox/downloads/latest/tibia-online-status/addon-539902-latest.xpi?src=dp-btn-primary
1470765095583	addons.xpi	DEBUG	removeTemporaryFile: https://addons.mozilla.org/firefox/downloads/latest/tibia-online-status/addon-539902-latest.xpi?src=dp-btn-primary removing temp file /Users/awagner/Library/Caches/TemporaryItems/tmp-epb.xpi

Reproducible in all channels.

Updated

2 years ago
Assignee: nobody → aswan
Priority: -- → P1
Whiteboard: triaged
(Assignee)

Updated

2 years ago
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → affected
Sylvestre, needinfo to you to make sure you see this. andym mentioned this may be a candidate for dotrelease. 

Tracking for 49+ in case we get a verifiable fix. Andy, can you describe the impact a bit more ? I assume this is affecting many addon developers if we are thinking about this as a dot release driver or ridealong.
tracking-firefox48: --- → ?
tracking-firefox49: --- → +
tracking-firefox50: --- → +
tracking-firefox51: --- → +
Flags: needinfo?(sledru)
Flags: needinfo?(amckay)

Comment 2

2 years ago
Its not worthy of a new dot release on its own. I would only consider it as a ride along.
Flags: needinfo?(amckay)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

2 years ago
Comment on attachment 8779505 [details]
Bug 1293721 Handle options_ui properly when id isn't immediately available

This needs a test before landing but with the patch applied I can install the extension from the original report and view its preferences page.  Kris can you confirm this is what you were expecting?
Attachment #8779505 - Flags: feedback?(kmaglione+bmo)
Comment hidden (mozreview-request)

Comment 6

2 years ago
There are 58 webextensions add-ons on AMO that use options_ui, of those about 4 or 5 don't have an applications gecko id.

I recommend we let those developers know and ask them to specify and gecko id in the manifest and get those fixed up. And focus on getting this patch into Aurora.

I'll get a list of those add-ons affected tomorrow so the reviewer team can contact them.

Updated

2 years ago
status-firefox48: affected → wontfix
(Reporter)

Comment 7

2 years ago
Andy, do you think we could even ask for uplifting this to beta?
If not, do you think it makes sense to add a linter warning (or error) as a stopgap until the patch reached release?
Flags: needinfo?(amckay)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8779505 [details]
Bug 1293721 Handle options_ui properly when id isn't immediately available

https://reviewboard.mozilla.org/r/70490/#review67922

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:966
(Diff revision 2)
>  
>    if (manifest.options_ui) {
> -    addon.optionsURL = extension.getURL(manifest.options_ui.page);
> +    // We can't resolve this to a full URL until the Extension has
> +    // been fully loaded.  See bug 1293721 for gory details.
> +    addon.optionsURL = null;
> +    addon._optionsPage = manifest.options_ui.page

Why not just store this in optionsURL?
Comment hidden (mozreview-request)

Comment 10

2 years ago
(In reply to Andreas Wagner [:TheOne] from comment #7)
> Andy, do you think we could even ask for uplifting this to beta?
> If not, do you think it makes sense to add a linter warning (or error) as a
> stopgap until the patch reached release?

Yes we should uplift to beta too.
Flags: needinfo?(amckay)
Ok, if we want it in the dot release, the patch should be ready today.
Flags: needinfo?(sledru)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8779505 [details]
Bug 1293721 Handle options_ui properly when id isn't immediately available

https://reviewboard.mozilla.org/r/70490/#review68102

::: toolkit/mozapps/extensions/test/browser/addons/options_signed/manifest.json:1
(Diff revision 3)
> +{

I'd rather we bundle the META_INF folder and build the XPI than bundle the pre-built XPI, so we don't have to worry about whether it's in sync with the sources or not.

::: toolkit/mozapps/extensions/test/browser/addons/options_signed/manifest.json:7
(Diff revision 3)
> +  "manifest_version": 2,
> +
> +  "name": "Test options_ui",
> +  "description": "Test add-ons manager handling options_ui with no id in manifest.json",
> +  "version": "1.1",
> +  

Nit: Remove whitespace.

::: toolkit/mozapps/extensions/test/browser/browser_webext_options.js:8
(Diff revision 3)
> + */
> +
> +// Given an extension id, open the add-ons manager, navigate to the
> +// given extension and open its options pane, verifying that works.
> +function* openPrefs(id) {
> +  let mgrWindow = yield open_manager("addons://list/extension");

We should probably open the view before we install the add-on, given that it made a difference with an older variant of this patch.
Attachment #8779505 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 13

2 years ago
mozreview-review-reply
Comment on attachment 8779505 [details]
Bug 1293721 Handle options_ui properly when id isn't immediately available

https://reviewboard.mozilla.org/r/70490/#review68102

> I'd rather we bundle the META_INF folder and build the XPI than bundle the pre-built XPI, so we don't have to worry about whether it's in sync with the sources or not.

That sounds like a good idea but that would make the handling of this single extension different from the other 20+ addons in this directory.
How about a follow-up to apply your idea to the whole directory instead of doing it here?
Comment hidden (mozreview-request)

Comment 15

2 years ago
mozreview-review-reply
Comment on attachment 8779505 [details]
Bug 1293721 Handle options_ui properly when id isn't immediately available

https://reviewboard.mozilla.org/r/70490/#review68102

> That sounds like a good idea but that would make the handling of this single extension different from the other 20+ addons in this directory.
> How about a follow-up to apply your idea to the whole directory instead of doing it here?

Hm. I suppose.

Comment 16

2 years ago
mozreview-review
Comment on attachment 8779505 [details]
Bug 1293721 Handle options_ui properly when id isn't immediately available

https://reviewboard.mozilla.org/r/70490/#review68128
Attachment #8779505 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 17

2 years ago
mozreview-review-reply
Comment on attachment 8779505 [details]
Bug 1293721 Handle options_ui properly when id isn't immediately available

https://reviewboard.mozilla.org/r/70490/#review68102

> Hm. I suppose.

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

Comment 18

2 years ago
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/548a0e44482a
Handle options_ui properly when id isn't immediately available r=kmag
(Assignee)

Comment 19

2 years ago
Comment on attachment 8779505 [details]
Bug 1293721 Handle options_ui properly when id isn't immediately available

Approval Request Comment
[Feature/regressing bug #]: bug 1262005
[User impact if declined]: some webextensions (those that use options_ui but do not include an id in the manifest) will fail to load
[Describe test coverage new/current, TreeHerder]: test included in the patch
[Risks and why]: Risks are low, handling of all extension options pages is affected, but covered by existing tests plus new tests
[String/UUID change made/needed]: none
Attachment #8779505 - Flags: approval-mozilla-release?
Attachment #8779505 - Flags: approval-mozilla-beta?
Attachment #8779505 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 20

2 years ago
After this landed in autoland, Kris pointed out that this is going to break compatibility for existing users with fully qualified options URLs in their addons DB.  I'll work on a follow-up to address that.
Keywords: leave-open
(Assignee)

Updated

2 years ago
See Also: → bug 1269454
Andrew, should we take it too beta or should we wait for a second patch?
(Assignee)

Comment 22

2 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #21)
> Andrew, should we take it too beta or should we wait for a second patch?

It shouldn't land in beta without the second patch, I'll have that ready within a few hours.
(Assignee)

Comment 23

2 years ago
Created attachment 8780167 [details] [diff] [review]
follow up: handle pre-existing absolute optionsURL
Attachment #8780167 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 24

2 years ago
Comment on attachment 8779505 [details]
Bug 1293721 Handle options_ui properly when id isn't immediately available

Once the follow up is reviewed, I'll generate a single squashed patch for uplifts, stay tuned.
Attachment #8779505 - Flags: approval-mozilla-release?
Attachment #8779505 - Flags: approval-mozilla-beta?
Attachment #8779505 - Flags: approval-mozilla-aurora?

Updated

2 years ago
Attachment #8780167 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8780167 - Attachment is obsolete: true

Comment 26

2 years ago
mozreview-review
Comment on attachment 8780198 [details]
Bug 1293721 follow up: handle pre-existing absolute optionsURL

https://reviewboard.mozilla.org/r/70940/#review68414
Attachment #8780198 - Flags: review?(kmaglione+bmo) → review+

Comment 27

2 years ago
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/61fbf96a2cfa
follow up: handle pre-existing absolute optionsURL r=kmag
(Assignee)

Comment 28

2 years ago
Created attachment 8780222 [details] [diff] [review]
Handle options_ui properly when id isn't immediately available

This patch is the original two patches squashed, rebased onto aurora,
and then slightly modified to work in aurora.  try run is here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e47c20fd344b
Attachment #8780222 - Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request)

Comment 31

2 years ago
mozreview-review
Comment on attachment 8780198 [details]
Bug 1293721 follow up: handle pre-existing absolute optionsURL

https://reviewboard.mozilla.org/r/70940/#review68538

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:7258
(Diff revisions 1 - 2)
> +        let baseURI = NetUtil.newURI(base);
> +        let result = NetUtil.newURI(addon.optionsURL, null, baseURI).spec;

We should still use `new URL` for this.
Comment hidden (mozreview-request)

Comment 33

2 years ago
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ebbebe3e2aee
follow up: handle pre-existing absolute optionsURL r=kmag
Andy marked it as wontfix for 48, not tracking.
tracking-firefox48: ? → -
(Assignee)

Comment 36

2 years ago
Clearing ni? and marking resolved since the revised follow-up patch landed in m-c yesterday.

There's still the matter of uplifts.  The aurora patch from comment 28 was good on try.  Ping :kmag
Flags: needinfo?(aswan)
(Assignee)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED

Updated

2 years ago
Attachment #8780222 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 37

2 years ago
We've been getting a fair number of complaints about this from release users.  Andy is out today and next week but I discussed this with him verbally before he left and he's on board with uplifting this all the way to release (or with asking for approval to do so to be precise)
status-firefox48: wontfix → affected
The patch is pretty big, not verified. I prefer this ride the trains from 49.
I would prefer to see the addons updated instead.
(Assignee)

Comment 39

2 years ago
Comment on attachment 8780222 [details] [diff] [review]
Handle options_ui properly when id isn't immediately available

See comment 19 for the original uplift request

Try runs for beta and release respectively:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=16862d9216b6
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcf01a32a6dc
Attachment #8780222 - Flags: approval-mozilla-release?
Attachment #8780222 - Flags: approval-mozilla-beta?
Attachment #8780222 - Flags: approval-mozilla-aurora?
Comment on attachment 8780222 [details] [diff] [review]
Handle options_ui properly when id isn't immediately available

Let's take it on all branches. should be in 49beta4.
cf comment #38 for release.
Attachment #8780222 - Flags: approval-mozilla-release?
Attachment #8780222 - Flags: approval-mozilla-release-
Attachment #8780222 - Flags: approval-mozilla-beta?
Attachment #8780222 - Flags: approval-mozilla-beta+
Attachment #8780222 - Flags: approval-mozilla-aurora?
Attachment #8780222 - Flags: approval-mozilla-aurora+
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1269454

Comment 42

2 years ago
Verified as fixed in FF 51.0a1 (2016-08-16) using Mac OS 10.10.4, Win 7 x64 and Win 10 x32:
- normal install: add-on is installed with success and no error is returned 
- temporary install: add-on is installed with success

I've tested using the extension mentioned at comment #0 and the extension mentioned in  https://bugzilla.mozilla.org/show_bug.cgi?id=1283181 (that is marked as duplicate of Bug 1269454)

Question: 
-> after uploading the first add-on with success in about:debugging and try to upload the second one: we can see only the last add-on uploaded - is this an intended behavior?

Based on comment #40, shouldn't this bug be fixed in Beta and Aurora?

Comment 43

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/e6d0063c10d6
status-firefox50: affected → fixed
Needs rebasing for Beta.
Flags: needinfo?(aswan)
status-firefox48: affected → wontfix
(Assignee)

Comment 45

2 years ago
Created attachment 8781583 [details] [diff] [review]
Handle options_ui properly when id isn't immediately available



MozReview-Commit-ID: JQC8rZwUkXG
(Assignee)

Comment 46

2 years ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #44)
> Needs rebasing for Beta.

Gr, rebased patch is failing lint for some dumb reason, I'll get back to this later today and just push it myself with a=sledru if that's okay.
(Assignee)

Updated

2 years ago
Attachment #8781583 - Attachment is obsolete: true
Flags: needinfo?(aswan)
(Assignee)

Comment 47

2 years ago
I had to tweak the patch a bit more for beta, nothing substantial but here's a new try run anyway:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c249678cb57

Will push on green.
(Assignee)

Comment 48

2 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/a7839743d385
status-firefox49: affected → fixed

Comment 49

2 years ago
Verified as fixed on FF49.0b5(19-08-2016 and FF50.0a2(18-08-2016) using Win 7x64  
-normal install, drag&drop or "install from file" for https://addons.mozilla.org/en-US/firefox/addon/tibia-online-status/?src=ss is working as expected and no errors are returned  

Posftix video: http://screencast.com/t/uLQdQY9T6K

Also, please update the status flag for Nightly. Thanks!
(Reporter)

Updated

2 years ago
status-firefox51: affected → fixed
(Reporter)

Updated

2 years ago
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified
status-firefox50: fixed → verified
You need to log in before you can comment on or make changes to this bug.