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

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[Andreas Wagner [:TheOne] [use NI]]

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: [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
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.

Comment 1

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 2

[Andy McKay]

Its not worthy of a new dot release on its own. I would only consider it as a ride along.

Comment 3

[Andrew Swan [:aswan]]

Attachment: Bug 1293721 Handle options_ui properly when id isn't immediately available

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

Comment 4

[Andrew Swan [:aswan]]

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?

Comment 5

[Andrew Swan [:aswan]]

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

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

Comment 6

[Andy McKay]

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.

Comment 7

[Andreas Wagner [:TheOne] [use NI]]

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?

Comment 8

[Kris Maglione [:kmag]]

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 9

[Andrew Swan [:aswan]]

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

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

Comment 10

[Andy McKay]

(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.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Ok, if we want it in the dot release, the patch should be ready today.

Comment 12

[Kris Maglione [:kmag]]

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.

Comment 13

[Andrew Swan [:aswan]]

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 14

[Andrew Swan [:aswan]]

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

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

Comment 15

[Kris Maglione [:kmag]]

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

[Kris Maglione [:kmag]]

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

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

Comment 17

[Andrew Swan [:aswan]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/548a0e44482a
Handle options_ui properly when id isn't immediately available r=kmag

Comment 19

[Andrew Swan [:aswan]]

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

Comment 20

[Andrew Swan [:aswan]]

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.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Andrew, should we take it too beta or should we wait for a second patch?

Comment 22

[Andrew Swan [:aswan]]

(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.

Comment 23

[Andrew Swan [:aswan]]

Attachment: follow up: handle pre-existing absolute optionsURL

Comment 24

[Andrew Swan [:aswan]]

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.

Comment 25

[Andrew Swan [:aswan]]

Attachment: Bug 1293721 follow up: handle pre-existing absolute optionsURL

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

Comment 26

[Kris Maglione [:kmag]]

Comment on attachment 8780198 [details]
Bug 1293721 follow up: handle pre-existing absolute optionsURL

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61fbf96a2cfa
follow up: handle pre-existing absolute optionsURL r=kmag

Comment 28

[Andrew Swan [:aswan]]

Attachment: 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

Comment 29

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

Backed out for leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=1779478&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/7b97d5f70bb6

Comment 30

[Andrew Swan [:aswan]]

Comment on attachment 8780198 [details]
Bug 1293721 follow up: handle pre-existing absolute optionsURL

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

Comment 31

[Kris Maglione [:kmag]]

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 32

[Andrew Swan [:aswan]]

Comment on attachment 8780198 [details]
Bug 1293721 follow up: handle pre-existing absolute optionsURL

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

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ebbebe3e2aee
follow up: handle pre-existing absolute optionsURL r=kmag

Comment 34

[Sylvestre Ledru [:Sylvestre]]

Andy marked it as wontfix for 48, not tracking.

Comment 35

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

https://hg.mozilla.org/mozilla-central/rev/548a0e44482a
https://hg.mozilla.org/mozilla-central/rev/ebbebe3e2aee

Comment 36

[Andrew Swan [:aswan]]

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

Comment 37

[Andrew Swan [:aswan]]

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)

Comment 38

[Sylvestre Ledru [:Sylvestre]]

The patch is pretty big, not verified. I prefer this ride the trains from 49.
I would prefer to see the addons updated instead.

Comment 39

[Andrew Swan [:aswan]]

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

Comment 40

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 42

[Valentina Virlics [:vvalentina], Ecosystem QA]

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

[Ryan VanderMeulen [:RyanVM]]

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

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Beta.

Comment 45

[Andrew Swan [:aswan]]

Attachment: Handle options_ui properly when id isn't immediately available

MozReview-Commit-ID: JQC8rZwUkXG

Comment 46

[Andrew Swan [:aswan]]

(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.

Comment 47

[Andrew Swan [:aswan]]

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.

Comment 48

[Andrew Swan [:aswan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a7839743d385

Comment 49

[Valentina Virlics [:vvalentina], Ecosystem QA]

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!