793093 - Allow registering of arbitrary handlers for chrome.manifest directives

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

We have a couple of cases where we want to handle new types of directives in chrome.manifest:
* registering widget manifests in bug 770135
* registering search plugins in bug 598697

But there are various roadblocks to doing that:
* The only way to add a handler is to hard-code it in ManifestParser.cpp
* Handlers need to be in C++, or at least have some custom C++ handler that talks to JS via XPCOM
* There's no way for a handler to tell when a directive is unregistered (for restartless add-ons)

So, my proposal is:


Have new consumers register as a handler for a specific directive via Components.manager.addManifestHandler() / removeManifestHandler(). When a chrome.manifest is parsed and we find a directive that isn't handled by the built-in handler look for a handler that was registered via addManifestHandler(). Call that handler's handleManifestDirective() function, passing in the directive name, its tokens, and the file location of the chrome manifest file (this is important to be able to associate a directive with a specific add-on, and ideally we'd get the location the root manifest not any of the sub-manifests loaded due to the "manifest" directive).

At startup when parsing manifests, custom handlers won't have had a chance to register themselves yet. Also, restartless add-ons enabled during runtime that register their own handlers will want to see what directive have already been parsed. So for any directive that isn't handled, we store it somewhere and have them accessible via Components.manager.unhandledManifestDirectives - with a way to mark them as handled (thus removing them from that list). Will need to store *all* directives that aren't handled by the built-in handlers, and put them into the unhandledManifestDirectives list when their handler is registered, so restartless add-ons can be upgraded during runtime.

We'd also need to deal with unregistering directives from restartless add-ons when they get disabled/uninstalled. At the moment manifests aren't "unloaded", nsComponentManager just throws everything away and reloads it all - it never knows what was removed. We'd need to add that, by comparing before/after snapshots of the directives list (only those not handled by built-in handlers).



An alternative approach would be to do all that in the Add-ons Manager. But that has two major downsides:
* We'd need to read (and parse) chrome.manifest files twice - that's a bunch of unnecessary IO, especially on startup
* Would only work for manifests from add-ons - wouldn't be able to use it for manifests that are part of the app (or add-ons), which is desirable for both of the above use-cases

Comment 1

[Ben Bucksch (:BenB)]

Sounds great.

I'd just not hang it off something as central as Components.manager, but make it an XPCOM service nsIManifestParser.addHandler() or something.

Are you going to implement this? Because we'd like to get bug 598697 in soon.

Comment 2

[Ben Bucksch (:BenB)]

> So for any directive that isn't handled, we store it somewhere and have them accessible via
> Components.manager.unhandledManifestDirectives - with a way to mark them as handled
> (thus removing them from that list). Will need to store *all* directives that aren't
> handled by the built-in handlers, and put them into the unhandledManifestDirectives
> list when their handler is registered, so restartless add-ons can be upgraded during runtime.

Not sure I understand what you wrote, but I'd store them and then (automatically) call the handler immediately after it registered itself. That means the handler doesn't have to care about this whole problem.

Comment 3

[Benjamin Smedberg]

I'm a little excited and a lot worried about this proposal. In particular, the manifests were originally supposed to be one-time entities. We've cobbled support for manifests in restartless addons together for only chrome things (not components); when a manifest gets removed by restartless addon uninstall, we throw away all the chrome registry mappings and read them again. I tend to think that it probably makes more sense to have an "undo" feature (in the chrome registry and in these future handlers) rather than throwing things away and redoing everything. I don't know whether we should do that per-line or per-manifest. Some other issues to consider:

* It's not clear to me what the use-case is for addons being able to add new manifest directives. I think we should only support core code adding new directives. So there should never be a case where we encounter a "foo" directive and then later load the foo-handler. And we don't have to implement/test removeManifestHandler() or unhandledManifestDirectives.
* This means handlers can provide a clear grammar to the parser at registration time, indicating the number of expected arguments so that the unified condition handling will still work.
* This means we can continue to report unrecognized directives parse errors at parse time.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Ben Bucksch (:BenB) from comment #1)
> I'd just not hang it off something as central as Components.manager, but
> make it an XPCOM service nsIManifestParser.addHandler() or something.

I mostly suggested Components.manager because addBootstrappedManifestLocation/removeBootstrappedManifestLocation was added to that somewhat recently.


> Are you going to implement this? Because we'd like to get bug 598697 in soon.

Not my top-most priority, but yea. FWIW, I'd consider this a hard-requirement for fixing bug 598697.


(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> we throw away all the chrome registry mappings and read them
> again. I tend to think that it probably makes more sense to have an "undo"
> feature (in the chrome registry and in these future handlers) rather than
> throwing things away and redoing everything.

Yea, I think fixing that everywhere properly would be much better. But (at least to me) it's also a lot scarier, and a lot more invasive. At least with implementing something like comment 0, we'd get the benefits for any new types of directives we add, and can potentially convert over all the existing ones gradually afterwards.


> * It's not clear to me what the use-case is for addons being able to add new
> manifest directives. I think we should only support core code adding new
> directives. So there should never be a case where we encounter a "foo"
> directive and then later load the foo-handler. And we don't have to
> implement/test removeManifestHandler() or unhandledManifestDirectives.

I guess the use-case for add-ons handling directives is a bit weak. I had thought of add-ons providing additional components (not in the XPCOM component sense) to other add-ons - from past experience, we lack a lightweight way of doing that. But mostly, it just doesn't seem like much more additional effort to support it.

Without add-ons handling directives, removeManifestHandler() wouldn't be needed; and possibly the API-side of unhandledManifestDirectives, given comment 2. 

But since these new handlers would be loaded based on "component" (or whatever) directives in manifests, handlers would be registered at some stage during/after parsing (not before). So it still needs to keep track of any encountered unhandled directives at least until after all the manifests are parsed on startup.


> * This means handlers can provide a clear grammar to the parser at
> registration time, indicating the number of expected arguments so that the
> unified condition handling will still work.

Yep. I'd meant to mention something like that in comment 0.


> * This means we can continue to report unrecognized directives parse errors
> at parse time.

Only for manifests parsed during the session, not at startup (see above).

Comment 5

[Benjamin Smedberg]

> But since these new handlers would be loaded based on "component" (or
> whatever) directives in manifests, handlers would be registered at some
> stage during/after parsing (not before). So it still needs to keep track of

The order we load manifests is deterministic. There is no reason that we couldn't require that the "component" line for some new manifest handler must be registered before we start using it.

I strongly object to a mechanism that allows for arbitrary ordering and exposes unhandled directives.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> The order we load manifests is deterministic. There is no reason that we
> couldn't require that the "component" line for some new manifest handler
> must be registered before we start using it.

Ah. I was assuming we'd use these new directives in manifests in the tree as well. But at most that's only a "nice to have" for the current use-cases, not something I see as needed.

If we change it so directives must have a handler at the time they're parsed, and handlers can't be unregistered, does everything else seem reasonable?

Comment 7

[Benjamin Smedberg]

We can use them in the tree; we just have to make sure that the handlers are registered before they are used. With that I think we're ok. I'd like to feedback? the API before you do the full impl, if that's ok.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: WiP v1

Wow does my C++ feel rusty :\
(And yes, I know I'm doing at least one stupid thing there.)

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Feedback ping?

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Blair, can you describe what you think you may be doing wrong here?

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP v1.1 (rebased, changed a couple minor things)

Benjamin, can you let me know if you will be able to provide feedback here?

Comment 12

[Benjamin Smedberg]

After thinking this over, I don't think this is a good idea after all. Manifest parsing is very sensitive in terms of startup time, and I don't think we should extend their usage to things which are not essential to core startup. Use a JSM and programmatic calls to add interesting things.

Comment 13

[Ben Bucksch (:BenB)]

This is a precondition of bug 598697. The lack of which has given us *major* headaches in the last 2 years.