1892669 - Allow multiple function and event definitions using the same name for different manifest version targets

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P3)

Comment 1

[John Bieling]

Attachment: Bug 1892669 - Allow skipping function and event parameters based on min/max_manifest_version entries in the schema definition. r=robwu

This allows to use min/max_manifest_version entries in the definition of
function and event parameters (including extraParameters). Many Manifest
V2/Manifest V3 differences can already be caught using a choices
array in the type definition, but that does not cover all edge cases. For
example:

1. impossible to add non-optional parameters in Manifest V3 only
2. impossible to rename a parameter (needed for documentation)

Comment 2

[Rob Wu [:robwu]]

1. impossible to add non-optional parameters in Manifest V3 only

So you'd like to mark a parameter as required in MV3 but optional in MV2.
Before the patch, every index in the parameters array always matched to a position in the parameter list of the actual implementation.
In the current version of the patch, parameters definitions are dropped from the schema when there is a manifest version mismatch. Such a type definition is only obvious in two situations: a parameter definition at the end, or parameters where there is exactly one parameter definition for each manifest version, with overlapping/equivalent semantics. The latter is the case in bug 1892674. This scenario is so uncommon that I'd rather explore alternatives instead of complicating the schema parser.

Renaming (case 2) is covered below, so the remaining case is the ability to make a parameter required in one manifest but optional in another. Conceptually, I think that it could make sense to consider an option required if the choices list is non-empty, and optional if the choices list does not contain any entries that match the manifest version.

2. impossible to rename a parameter (needed for documentation)

The browser does not really care about the name: the WebExtensions framework does not use it; in contrast, Chrome's generated error messages include the type and name of parameters. If for documentation purposes (with specific example from bug 1892674 in mind), my advice would be to either rename folder to folderId and mention that the non-ID folder type was the previous format and is also supported for backwards compatibility in MV2.

Comment 3

[John Bieling]

The mentioned use cases are just some of the possibilities. In general, it is the wish to support min/max_manifest_version entries in function parameters, just as we can use them in property lists. Another use case is to switch a default from true to false, and have that stored in the schema file and not the implementation.

Documentation: Since schema files support to differ between MV2 and MV3, it would be ideal to also get the correct values for the documentation from the schema files. Having to pick one to use it in both is unfortunate. I would like to avoid to store additional data fields outside of the schema fields to get the documentation right. It is desired to have as much information in the schema file as possible. Downstream projects like TypeScript type definition generators would benefit from that.

I will see if I can get choices to work with id, default and/or optional

Comment 4

[John Bieling]

Attachment: Bug 1892669 - Allow choices to specify extra properties. r=robwu

So far, it is not possible to have a different optional setting for
function parameters in Manifest V2 and Manifest V3.

This is an alternative approach to be able to specify "optional" and
"default" (and also "name") as a choice, and not on the main parameter.

Comment 5

[John Bieling]

I did not update the existing phab, but created a new one, since it is using a different approach.

I understand, that you do not want to break up the fixed relationship between the parameter index used, and the parameters as defined in the schema. The only other approach I found is based on using choices.

The optional skip is handled before the choices are evaluated. In order to see if a manifest-version-based choice has an explicit definition, the patch preprocesses the raw parameter value and checks the available options. If only one option is applicable and has explicit definitions for optional, default and/or name, these values are used for the parameter.

Comment 6

[Rob Wu [:robwu]]

Thanks for the two different iterations with the different approaches. After seeing your comments and examples, I think that I have a final direction that should work.

Conceptually, I think that it would make most sense to specify min_manifest_version and max_manifest_version at the function level (or if you are very adventurous - the namespace level). For example, to change a parameter as required, the syntax would be as follows. Besides the use case here (bug 1892674), I also see an opportunity to use this capability for the userScripts.register API (bug 1875475), where there are conflicting method definitions with the same name (different parameters, different return types, different permission requirements).

{
  "namespace": "myNamespace",
  "functions": [
    {
      "max_manifest_version": 2,
      "name": "myFunc",
      "type": "function",
      "parameters": [{ "name": "arg", "type": "string", "optional": true }]
    },
    {
      "min_manifest_version": 3,
      "name": "myFunc",
      "type": "function",
      "parameters": [{ "name": "arg", "type": "string" }]
    }
  ]
}

I briefly explored the possibility of implementing it, at the namespace level, or at the individual function level.

Doing so at the the namespace level would be complicated. While it is already possible for a namespace to be spread across different definitions, there is generally the assumption that min_manifest_version and max_manifest_version is specified only once (and if specified multiple times, that they are at least consistent). The implementation that does so is basically in Namespace.prototype.addSchema, which collects the list of schema files and overwrites properties such as min_manifest_version, if any.

• The getNamespace method does currently not take a manifest version, so you would need to add it and pass it from the callers. E.g. in Namespace.prototype.getDescriptor, you would have to start passing context.manifestVersion all the way through to getNamespace if you want to do something special at the namespace level. (context class and manifestVersion definition).
• Similarly for Schemas.checkPermissions. In this case, "wrapperFuncs" is actually the Extension instance (which has a manifestVersion field), so in practice you can read wrapperFuncs.manifestVersion

Doing so at the function level would be more feasible. The JSON format could accept multiple definitions with the same name, and these would then be saved in the namespace by Namespace.prototype.init - note that the code looks like this: this.functions.get(fun.name).push(fun)

• In practice, only types have more than one definition. If a schema contains $extend for a type, then it would extend an existing (object) definition through loadType. Note that the actual "parent" type is looked up via this.get(type.$extend);, where this is Namespace extends Map. You can think of it as a lookup from the map, in practice there is some minimal magic going on to lazily initialize the actual schema Namespace.prototype.get, which calls initKey, which reads from this[type].get(key) - this is exactly the this.functions.get(fun.name) array mentioned above.
• As you can see in the lnked initKey function, it stores the definition in the namespace map by that key. In case of types (and loadType), any previous key is read (see previous explanation on $extend) and saved before the overwrite.
• To expand this functionality to functions (and/or events), you would have to do something similar in loadFunction and loadEvent. E.g. by storing it as a kind of linked list (FunctionEntry.fallback -> FunctionEntry -> ...).
• Once you have a mechanism to store it, you need to do something useful with it. That may be the easy part - Currently, when the manifest version of an entry does not match, the entry (e.g. function definition derived from the schema) is ignored (see Namespace.prototype.injectInto and its matchManifestVersion call). Now instead of ignoring, you can read the fallback field until there is a matching entry (and if there is none, continue as before).

Comment 7

[John Bieling]

Attachment: Bug 1892669 - Allow multiple function and event definitions using the same name for different manifest version targets. r=robwu,rpl

This is implementing the approach proposed by Rob Wu. If multiple
function/event definitions with the same name are found in the schema,
there are no longer skipped (actually replaced while parsing, so only
the last one survived), but added as a fallback.

When checking the definition for matching the current manifest version,
the implemenation walks through the provided fallbacks until it finds
the correctly matching definition (or none).

This allows to have completely independent definitions of
functions/events for different manifest versions.

Comment 8

[John Bieling]

Thanks, Rob, for putting so much work in explaining the situation in Comment 6. That does seem to be the most flexible solution, because it allows to specify completely independent definitions for MV2 and MV3. I hope my implementation is as expected.

Comment 9

[John Bieling]

Attachment: Bug 1892669 - Use a private method of Namespace instead of a labeled for-loop. r=rpl

Depends on D209488

Comment 10

[Rob Wu [:robwu]]

FYI: This is the snippet I used to check which values list more than one definition (from comment 7 - "In practice, only types have more than one definition.")

function logNamespace(rootSchema, prefix = "") {
  for (let [ns, o] of rootSchema.entries()) {
    if (prefix && o?.constructor?.name !== "Namespace") continue; // Non-root namespaces can contain non-Namespace
    o.init(); // initialize Namespace
    for (let k of ['types', 'properties', 'functions', 'events']){
      let dm = o[k];
      for (let [dmK, dmV] of dm) {
        if (dmV.length > 1) { console.log(prefix + "." + ns, k, dmK, dmV); }
      }
    }
    for (let [k, v] of o.entries()) {
      if (v?.constructor?.name === "Namespace") { // Nested namespace
        logNamespace(v, prefix + "." + k)
      }
    }
  }
}
logNamespace(ChromeUtils.importESModule("resource://gre/modules/Schemas.sys.mjs").Schemas.rootSchema);

Although the logic above accounts for nested namespaces, and types/properties/functions/events, the output shows that there are no entries with multiple definitions in nested namespaces nor in properties/functions/events. Only in types, as seen below:

Output:

.manifest types WebExtensionManifest   Array(17) [ {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, … ]
.manifest types OptionalPermissionNoPrompt   Array(7) [ {…}, {…}, {…}, {…}, {…}, {…}, {…} ]
.manifest types OptionalPermission   Array(18) [ {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, … ]
.manifest types PermissionPrivileged   Array(5) [ {…}, {…}, {…}, {…}, {…} ]
.manifest types PermissionNoPrompt   Array(9) [ {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…} ]
.manifest types Permission   Array(3) [ {…}, {…}, {…} ]

When the patch attached to this bug lands, and the dependent bugs are resolved, I expect that the result is going to include functions entries.

Comment 11

[John Bieling]

It will have entries like this:

.folders functions get Array [ {…}, {…} ]
​   0: Object { async: "callback", min_manifest_version: 3, name: "get", … }
​   1: Object { async: "callback", max_manifest_version: 2, name: "get", … }

I assume this is as expected? Do you consider this good or bad?

Edit: This is from Thunderbird with all dependent patches applied. If I run this on Firefox tip and the patch for Schemas.sys.mjs, I get the output from Comment 10.

Comment 12

[Rob Wu [:robwu]]

@John That is as expected. At the end of my comment 10, I predicted that Thunderbird will contain "functions" entries.

By design each item will have a non-overlapping manifest version support range, and the implementation will choose the right entry depending on the extension's manifest version.

Additionally, we don't have to worry about incompatibilities between the cached schemas and the application's Schemas.sys.mjs implementation, because these caches are erased when the application updates. This is guaranteed on release, with the implementation at https://searchfox.org/mozilla-central/rev/55944eaee1e358b5443eaedc8adcd37e3fd23fd3/toolkit/xre/nsAppRunner.cpp#5159-5232

And in the unlikely event of the cache not being erased:

• if old schema (one entry without manifest restrictions), but new implementation: API is exposed without manifest version restrictions.
• if new schema, but old implementation: the last entry in the schema overwrites the previous ones without fallback. So the API is hidden from a specific manifest version.
  • this case is what you are seeing on try with condprof: https://phabricator.services.mozilla.com/D209488#7237330
  • on condprof the issue happens because the caches weren't cleared (thus the old Schemas.sys.mjs implementation continued to be used). The cache is not cleared because the logic in nsAppRunner.cpp that I mentioned before does not trigger in xpcshell runner, for details see the penultimate paragraph of https://bugzilla.mozilla.org/show_bug.cgi?id=1769184#c3

Comment 13

[John Bieling]

Ah great, so this is not a blocking issue. Thanks for your explanations.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3be6c6131109
Allow multiple function and event definitions using the same name for different manifest version targets. r=robwu,rpl

Comment 15

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/3be6c6131109