create a method for directly adding chrome registry entries

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: aswan, Assigned: kmag)

Tracking

55 Branch
mozilla57
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

The only way to add chrome registry entries is by providing a manifest file.  For new-style langpacks we want to programatically add entries.  This bug is to add that capability to nsIChromeRegistry.
Remind me why new-style langpacks need chrome registration at all? Could you use another custom protocol handler instead? Given the long history of dynamic chrome registration (and unregistration) issues, and how we intentionally removed this feature in the past, I'm worried that this would be a step in the wrong direction.
Flags: needinfo?(aswan)
302 to :gandalf
Flags: needinfo?(aswan) → needinfo?(gandalf)
> Remind me why new-style langpacks need chrome registration at all?

Because for a while we will have both old and new entries. In order for the "old" entries to work, they need to be registered in Chrome Registry.
We simplified the process by adding all of them into the manifest.json file and as we move to the new localization API we'll be removing them until there's nothing left.

> Could you use another custom protocol handler instead?

Possibly. I don't know how to register the path to the resources stored in a langpack to L10nRegistry otherwise then through a "resource ${langpackId} /path" entry and then doing `fetch('resource:///${langpackId}');`. Do you have suggestions?

The bottom line is, we'll need hybrid langpacks for a while and we'll need a path to the langpack resources for the new entries as well.

In case you want to see the scope, this is the current manifest.json for the new langpacks: https://bug1365031.bmoattachments.org/attachment.cgi?id=8868470

Do you have a different recommendation for action here?
Flags: needinfo?(gandalf) → needinfo?(benjamin)
I don't really understand all of your constraints, so it's hard to say. My recommendation would be to not use resource: or chrome: at all, but have a protocol handler mozl10n://{locale}/{files} which you control completely.

I'm going to be on vacation for a couple weeks, which might leave you in a bit of a bind: I hope Mossop and froydnj can help provide advice (and reviews) in my absence.
Flags: needinfo?(benjamin)
To comment on this the third time, and third time is charm:

We never expose resource URLs outside of the l10n registry, it's not something developers would see or use.

As we need to load multiple resources per developer-specified path pattern, the abstraction layer of a protocol doesn't fit.

I personally think we might just as well continue to ship a chrome manifest for the content we need in the chrome registry, as long as Firefox still uses that for parts of l10n. At most lint them during the install or AMO steps.
> As we need to load multiple resources per developer-specified path pattern, the abstraction layer of a protocol doesn't fit.

That's ok I believe. The proposal for the protocol (mozl10n:) is not for developers to use, but for L10nRegistry to use.

Developer still specifies just the uri (like they do right now in the l20n branch) like "/browser/menu.ftl"), L10nRegistry combines it with the right locale and basePath from the FileSource and constructs a path for Fetch API to use.

Currently the path is something like "resource://gre/localization/en-US/browser/menu.ftl" or "resource://langpack-pl/localization/pl/browser/menu.ftl".
The protocol proposal suggest that we do sth like "mozl10n://toolkit@mozilla.org/en-US/browser/menu.ftl" or "mozl10n://langpack-pl@mozilla.org/pl/browser/menu.ftl" and that internally goes through either the right resource or jar:|file: split.

> I personally think we might just as well continue to ship a chrome manifest for the content we need in the chrome registry, as long as Firefox still uses that for parts of l10n. At most lint them during the install or AMO steps.

If I understand correctly that's the position :bsmedberg and :mossop take. :kmag believes that to be worse than adding programmatic API to register them and for that reason suggested that he'll write a patch and then we can discuss its merits.
For me there's no big difference between adding the same list of entries to chrome.manifest at build time, or manifest.json.
> The protocol proposal suggest that we do sth like "mozl10n://toolkit@mozilla.org/en-US/browser/menu.ftl" or "mozl10n://langpack-pl@mozilla.org/pl/browser/menu.ftl" and that internally goes through either the right resource or jar:|file: split.

If we'll do our own jar|file split, then the benefit here is that we won't have to register the langpack-pl as a resource entry since we won't use resource protocol.

If we, behind the scenes, go through the resource: then we still need to register it during WebExtension bootstrap, but the benefit is that then the use of resource: becomes an internal matter between L10nRegistry and mozL10n protocol and we can switch away from resource: transparently.
Without mozl10n: the only way to retrieve a localization resource remains to use the resource: protocol and if anything, for whatever reason (tests?), will want to retrieve a resource not via L10nRegistry, we'll have to transition it if we ever decide to move away from resource:.
So, I have to say I like the mozl10n: proposal, even as a API wrapper over resource: for now.
I went with the simplest possible approach here, and only added support for
"locale" and "override" entries, since we don't expect this to stick around
very long.
Attachment #8893653 - Flags: review?(dtownsend)
We do need "resource" entries as well for searchplugins entry as per https://bug1365031.bmoattachments.org/attachment.cgi?id=8868470
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #9)
> We do need "resource" entries as well for searchplugins entry as per
> https://bug1365031.bmoattachments.org/attachment.cgi?id=8868470

Resource entries can be directly registered with the resource: protocol handler.
Comment on attachment 8893653 [details] [diff] [review]
Add a helper for adding dynamic chrome registry entries

Review of attachment 8893653 [details] [diff] [review]:
-----------------------------------------------------------------

I'm fine with this approach but it would be good to have someone with better Mozilla C++ chops than me review the details here.
Attachment #8893653 - Flags: review?(dtownsend) → feedback+
Attachment #8893653 - Flags: review?(nfroyd)
Comment on attachment 8893653 [details] [diff] [review]
Add a helper for adding dynamic chrome registry entries

Review of attachment 8893653 [details] [diff] [review]:
-----------------------------------------------------------------

My knowledge of this stuff is minimal, but I have some questions.

::: toolkit/mozapps/extensions/AddonManagerStartup.cpp
@@ +843,5 @@
> +
> +private:
> +  FileLocation mLocation;
> +  nsTArray<Override> mOverrides;
> +  nsTArray<Locale> mLocales;

Could make these const member variables, though that might have undesirable consequences.

@@ +873,5 @@
> +  if (isInList()) {
> +    remove();
> +
> +    RefPtr<nsChromeRegistry> cr = nsChromeRegistry::GetSingleton();
> +    return cr->CheckForNewChrome();

I am probably missing a lot of context here, but why do we want to do this when every entry is destroyed?  That seems kind of expensive.

@@ +884,5 @@
> +{
> +  static LinkedList<RegistryEntries> sEntries;
> +  return sEntries;
> +}
> +}; // anonymouse namespace

Nit: "anonymous"

@@ +886,5 @@
> +  return sEntries;
> +}
> +}; // anonymouse namespace
> +
> +nsresult

Nit: is this supposed to be NS_IMETHODIMP?

@@ +888,5 @@
> +}; // anonymouse namespace
> +
> +nsresult
> +AddonManagerStartup::RegisterChrome(nsIURI* manifestURI, JS::HandleValue locations,
> +                                    JSContext* cx,  nsIJSRAIIHelper** result)

This isn't called from anywhere in the current patch?

@@ +923,5 @@
> +
> +      vals.AppendElement(NS_ConvertUTF16toUTF8(str));
> +    }
> +
> +    nsCString type = vals[0];

What guarantees that this is going to be in-bounds?

@@ +961,5 @@
> +AddonManagerStartup::Observe(nsISupports* subject, const char* topic, const char16_t* data)
> +{
> +  if (!strcmp(topic, "chrome-manifests-loaded")) {
> +    for (auto entry : GetRegistryEntries()) {
> +      entry->Register();

So entries register themselves when they're dynamically added from chrome, and when we observe chrome-manifests-loaded?  That seems...unusual.  Why would we do both?
Attachment #8893653 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #12)
> @@ +873,5 @@
> > +  if (isInList()) {
> > +    remove();
> > +
> > +    RefPtr<nsChromeRegistry> cr = nsChromeRegistry::GetSingleton();
> > +    return cr->CheckForNewChrome();
> 
> I am probably missing a lot of context here, but why do we want to do this
> when every entry is destroyed?  That seems kind of expensive.

This is basically a mirror of what we do for chrome manifests from
bootstrapped extensions, but for dynamically-generated rather than static
manifests.

The basic problem is that the chrome registry works by building static maps of
resources as it parses manifests. Entries from later manifests can override
entries from earlier manifests, and they do so by simply overwriting the
entries in the static mappings. When we remove a manifest, the only way to
guarantee sanity is to rebuild the resource maps from scratch, using the
remaining inputs.

This isn't a great situation, but it's the one we've already settled on for
mapping resources from bootstrapped extensions, so I don't see much sense in
changing it now. On the upside, this code should be fairly short-lived while
we transition legacy code to use the new locale service model.

> @@ +884,5 @@
> > +{
> > +  static LinkedList<RegistryEntries> sEntries;
> > +  return sEntries;
> > +}
> > +}; // anonymouse namespace
> 
> Nit: "anonymous"

Heh, yeah, I noticed that just after I pushed, and already fixed it locally.

> @@ +888,5 @@
> > +}; // anonymouse namespace
> > +
> > +nsresult
> > +AddonManagerStartup::RegisterChrome(nsIURI* manifestURI, JS::HandleValue locations,
> > +                                    JSContext* cx,  nsIJSRAIIHelper** result)
> 
> This isn't called from anywhere in the current patch?

At the moment, only from tests. It's a dependency for bug 1365709, though,
where we need it in order to register langpack resources.

> @@ +923,5 @@
> > +
> > +      vals.AppendElement(NS_ConvertUTF16toUTF8(str));
> > +    }
> > +
> > +    nsCString type = vals[0];
> 
> What guarantees that this is going to be in-bounds?

Nothing.

> @@ +961,5 @@
> > +AddonManagerStartup::Observe(nsISupports* subject, const char* topic, const char16_t* data)
> > +{
> > +  if (!strcmp(topic, "chrome-manifests-loaded")) {
> > +    for (auto entry : GetRegistryEntries()) {
> > +      entry->Register();
> 
> So entries register themselves when they're dynamically added from chrome,
> and when we observe chrome-manifests-loaded?  That seems...unusual.  Why
> would we do both?

Whenever a bootstrapped manifest or a dynamic entry is removed, we need to
clear the registry and rebuild from existing inputs. For static manifest
files, the chrome registry code takes care of that. For dynamic entries, this
observer handles it.
(In reply to Kris Maglione [:kmag] from comment #13)
> (In reply to Nathan Froyd [:froydnj] from comment #12)
> > @@ +923,5 @@
> > > +
> > > +      vals.AppendElement(NS_ConvertUTF16toUTF8(str));
> > > +    }
> > > +
> > > +    nsCString type = vals[0];
> > 
> > What guarantees that this is going to be in-bounds?
> 
> Nothing.

If we're going to go the trouble of verifying arrayness, stringness, length of subarrays, etc., we should also try to verify that we have a non-empty array here.

> > @@ +961,5 @@
> > > +AddonManagerStartup::Observe(nsISupports* subject, const char* topic, const char16_t* data)
> > > +{
> > > +  if (!strcmp(topic, "chrome-manifests-loaded")) {
> > > +    for (auto entry : GetRegistryEntries()) {
> > > +      entry->Register();
> > 
> > So entries register themselves when they're dynamically added from chrome,
> > and when we observe chrome-manifests-loaded?  That seems...unusual.  Why
> > would we do both?
> 
> Whenever a bootstrapped manifest or a dynamic entry is removed, we need to
> clear the registry and rebuild from existing inputs. For static manifest
> files, the chrome registry code takes care of that. For dynamic entries, this
> observer handles it.

OK, thanks for the explanations.  Some comments to these effects would be welcome here...even if this is going to be short-lived code.

Other than that, I don't think I have any objections to the patch.
With bug 1377911 fixed, and bug 1389407 and bug 1389397 on their way, can we remove `override` entries from this API? That would, if I understand correctly, help with security.
Flags: needinfo?(l10n)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #15)
> With bug 1377911 fixed, and bug 1389407 and bug 1389397 on their way, can we
> remove `override` entries from this API? That would, if I understand
> correctly, help with security.

We won't be exposing this API directly, so the ability to add override entries using it shouldn't affect security one way or the other.

Having already written tests and docs for override entries... I'm reluctant to remove support until we're sure we're done with the API. I've run into too many cases where we're sure we don't need something anymore, until we realize that we do.
Flags: needinfo?(l10n)
Comment on attachment 8896524 [details]
Bug 1384689: Add a helper for adding dynamic chrome registry entries. f=Mossop

https://reviewboard.mozilla.org/r/167778/#review173092
Attachment #8896524 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/be93c09fd3e37112fda7693205b7c1f70675091f
Bug 1384689: Add a helper for adding dynamic chrome registry entries. f=Mossop r=froydnj
Attachment #8896802 - Attachment is obsolete: true
Attachment #8896802 - Flags: review?(nfroyd)
https://hg.mozilla.org/mozilla-central/rev/be93c09fd3e3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Is manual testing required on this bug? If yes, please provide some STR and the proper webextension(if required) or set the “qe-verify-“ flag.
Flags: needinfo?(kmaglione+bmo)
This was just an engineering task that was part of bug 1365031, it doesn't need any manual verification.
Flags: needinfo?(kmaglione+bmo)
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.