Experiment names must be lower case

RESOLVED FIXED in Firefox 57

Status

defect
P3
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: bsilverberg, Assigned: ahillier)

Tracking

unspecified
mozilla57

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Based on a bunch of testing I just did to help a colleague, we have come to the conclusion that the name one uses when creating an experiment (i.e., the prefix of the id in the <em:id> entry in install.rdf) cannot be the same as the namespace as defined in the schema.json and api.js files.

When they are both the same, and one loads the experiment add-on, and then subsequently an extension that requests permission to use the experiment, the following error appears in the browser console:

Error opening input stream (invalid filename?): resource://extension-newtabcontent-api/api.js  Extension.jsm:1178

If one changes either the name or the namespace then the problem goes away.

This is contrary to the documentation both at [1] and [2], the latter of which specifically states:

"The first part of the id (before the @) must match the namespace used in api.js and schema.json."

So it seems like this is likely a regression that was introduced at some point.

A further note is that the boilerplate example include the permission string in the schema.json file, but Kris says that is unnecessary, and if that is the case it should be removed from the boilerplate example.

[1] https://webextensions-experiments.readthedocs.io/en/latest/how.html
[2] https://github.com/web-ext-experiments/boilerplate-experiment/blob/master/install.rdf
Assignee

Comment 1

2 years ago
On further testing I don't think that the issue is that the experiment name cannot be the same as the namespace. When the namespace is lowercase only (e.g. newtabcontent) and the name is the same then everything works correctly. It's only if the name and namespace have capital letters (e.g. newTabContent) that the error occurs.

I've noticed that if the name of the experiment and the namespace are different but the name is not wholly lowercase then the same issue occurs (the namespace can be capitalised regardless).

Hence my guess would be that input stream error arises when the experiment name is not wholly lowercase, and the cause is that the path that is being opened is transformed to lowercase (or some similar error with path capitalisation).
I think the problem is that when we register resources for an experiment here:
http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/toolkit/mozapps/extensions/internal/APIExtensionBootstrap.js#26
we use the experiment name as provided, but it should be normalized to lower case first:
http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/netwerk/protocol/res/nsISubstitutingProtocolHandler.idl#23-24

A fix should be as simple as adding `.toLowerCase()` to the definition of `resource`.  Bob, do you want to take a shot at it?
Flags: needinfo?(bob.silverberg)
Summary: The experiment name cannot be the same as the namespace when creating an experiment → Experiment names must be lower case
Reporter

Comment 3

2 years ago
Sure, unless Adam wants to give it a go, as he did most of the follow-up. Do you want to take this bug, Adam?
Flags: needinfo?(ahillier)
Assignee

Comment 4

2 years ago
Sure I'll take it, thanks
Flags: needinfo?(bob.silverberg)
Flags: needinfo?(ahillier)
Comment hidden (mozreview-request)
Reporter

Comment 6

2 years ago
mozreview-review
Comment on attachment 8890055 [details]
Bug 1382819 - Allow non-lowercase names for API extension experiments

https://reviewboard.mozilla.org/r/161114/#review166858

This looks good based on what Andrew said in the bug. Thanks for doing this, Adam.

I assume you tested this locally to verify it fixed the issue? We should really have an automated test for this too, before landing, but I'm not sure how much test coverage we already have for experiments. If you want to see if you can find the existing tests and find a good place to test this that would be great. If you'd like some help locating the tests let me know and I'll see if I can find them.
Attachment #8890055 - Flags: review?(bob.silverberg) → review+
Assignee

Comment 7

2 years ago
mozreview-review-reply
Comment on attachment 8890055 [details]
Bug 1382819 - Allow non-lowercase names for API extension experiments

https://reviewboard.mozilla.org/r/161114/#review166858

Yeah it works locally to fix it. I'll have a look and see what I can find for the tests.
Assignee: nobody → ahillier
Priority: -- → P3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8893815 - Attachment is obsolete: true
Attachment #8893815 - Flags: review?(bob.silverberg)
Reporter

Comment 11

2 years ago
mozreview-review
Comment on attachment 8890055 [details]
Bug 1382819 - Allow non-lowercase names for API extension experiments

https://reviewboard.mozilla.org/r/161114/#review170300

This looks great, thanks for finding and fixing this, Adam. The only comment I have is to add a bit more information to the commit message, but this gets my r+. Please request an final review by a peer (aswan is probably the best choice for this) so this can land.

::: commit-message-4b32e:2
(Diff revision 3)
> +Bug 1382819 - Allow non-lowercase names for API extension experiments
> +

Could you please add a comment to the commit message to explain why you updated the test in the manner you did?
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8890055 - Flags: review?(aswan)

Comment 13

2 years ago
mozreview-review
Comment on attachment 8890055 [details]
Bug 1382819 - Allow non-lowercase names for API extension experiments

https://reviewboard.mozilla.org/r/161114/#review170464

I think you changed a lot more than necessary (just the id needs have some upper case, not the namespace).  If you want to revise it, you could just capitalize the existing name.  But its not necessary, its also fine if you want to land this as is.
Attachment #8890055 - Flags: review?(aswan) → review+

Comment 14

2 years ago
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/535faaf4f99d
Allow non-lowercase names for API extension experiments r=aswan,bsilverberg

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/535faaf4f99d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.