Closed Bug 1094201 Opened 10 years ago Closed 8 years ago

Implement an Integration.jsm module for low-overhead registration of overrides

Categories

(Toolkit :: Async Tooling, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file)

There is a use case for low-overhead startup registration in bug 899013. The lazy initialization flow looks like this:

1. Downloads.jsm is lazily loaded at some point during application lifetime.
2. Asynchronous methods are called, for example getList or createDownload.
3. The methods see the module is uninitialized and wait for initialization.
4. The initialization procedure calls async functions on DownloadIntegration.
5. When the functions return, the rest of the action can continue.

We want to add methods to Downloads.jsm to register custom DownloadIntegration overrides, so that the functions in step 4 can be provided by the host application rather than the Toolkit implementation.

These overrides must be registered before the DownloadIntegration functions are called, but we want to avoid loading the Downloads.jsm module early on startup to make the registrations, for performance reasons.

One solution is to use the Category Manager to register an XPCOM component that is called by Downloads.jsm to trigger the registrations, just before the asynchronous DownloadIntegration functions are called. This provides all we need, but is XPCOM-based and requires a component to be written.

This bug is about providing an alternative that is easier to set up from JavaScript, something symmetrical to AsyncShutdown. For example, an AsyncStartup module would be loaded once and could then be used at any point during application lifetime for any module that requires this.

AsyncStartup.registerBlocker("Downloads", function* () {
  Downloads.registerIntegration(...);
});

If the "Downloads" module has been initialized already, the callback is executed immediately. This could work for the parent process and the content process independently, based on where AsyncStartup is called. Restartless add-ons could use this to integrate easily, while still lazy-loading the modules they want to integrate to.

Other lazy-loading helpers could be added independently, for example a persistent message listener in the parent that loads and calls any JSM in the parent, triggered from the child. This would help avoiding that each module has to eagerly register a listener in the parent on startup to handle its messages.

Should we do this? Opinions? Alternatives?

The new Form Autofill code is another place where we would also use a similar mechanism. The integration functions have already been implemented there:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/formautofill/FormAutofill.jsm#30

We could also uplift the integration mechanism itself, something like AsyncStartup.registerIntegration("Downloads", integrationFn) on the caller side and AsyncStartup.getCombinedIntegration("Downloads", BaseObject, refreshFn) on the module side.
Flags: needinfo?(dteller)
More detailed answer later, but shouldn't this be part of bug 1039705?
Flags: needinfo?(dteller)
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #1)
> More detailed answer later, but shouldn't this be part of bug 1039705?

Wasn't aware of that bug, it looks like it is pretty similar, and probably slightly wider in scope. This is about registration and invocation of module startup functions, while the other one also involves adding the appropriate well-known timers and events during host application startup to invoke the right services.

Looks like we'll need both an "AsyncStartup.invokeBlockerSerial" (for things like DownloadIntegration registrations) and "AsyncStartup.invokeBlockerParallel" (for starting things requiring network access at the same time).
Blocks: 1039705
Here we are! The description in comment 0 is only partly accurate, to see how this is used in practice take a look at bug 899013, in addition to the Form Autofill module from which this code has been extracted.

In fact, the patch in this bug does not deal with registering the integration points on application startup, though this is now possible with a one-liner in any of the JavaScript code executed early during startup. More advanced registration may be part of the work in bug 1039705.

The first use in bug 899013 will be to allow regression tests to override DownloadIntegration functions.
Comment on attachment 8732997 [details]
MozReview Request: Bug 1094201 - Implement an Integration.jsm module for low-overhead registration of overrides. r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41499/diff/1-2/
I updated the patch so that we preserve the state we set on the "this" object across registrations.

In general, it might be better for integration override objects not to store private state using "this" at all, so that it's not left behind when they are unregistered and cannot conflict with other integrations. However, given that integrations are only expected to be unregistered by tests and restartless add-ons, this might not be a big issue in general.

There was also a problem with the properties not using __lookupGetter__ to propagate the correct "this" value to the caller. Good news is that we can now use the "super" keyword and the "__proto__" definition in the object initializer to avoid some of that boilerplate. I've still kept the "__proto__" optional, so that integration override objects won't break the prototype chain if "__proto__" is unspecified.
sorry for the dumb question, but is there a reason we call this "integration" rather than "override"? I'd expect registerOverride to be easier understandable than registerIntegration to vast majority of devs. I always found the "integration" term confusing in this context, though there may be very good reasons (like broader actions than just overrides) for it that escape me.
Apologies for the delay, this was a crazy (work)week.
Assignee: nobody → paolo.mozmail
No worries!
Comment on attachment 8732997 [details]
MozReview Request: Bug 1094201 - Implement an Integration.jsm module for low-overhead registration of overrides. r=mak

https://reviewboard.mozilla.org/r/41499/#review39445

I'm still a bit too fuzy about what this module does to perform a real review.

::: toolkit/modules/AsyncStartup.jsm:6
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> + * Implements the AsyncStartup module.

I'd prefer a full-blown documentation that explains what that module does and what it's for.

::: toolkit/modules/AsyncStartup.jsm:36
(Diff revision 2)
> +
> +this.AsyncStartupModule.prototype = {
> +  /**
> +   * Ordered set of registered functions defining integration overrides.
> +   */
> +  _integrationFns: null,

At this stage, it's not clear to me what an integration is/does.

::: toolkit/modules/AsyncStartup.jsm:92
(Diff revision 2)
> +  },
> +
> +  /**
> +   * Registers new overrides for the integration methods. Example:
> +   *
> +   *   AsyncStartup.moduleName.registerIntegration(base => ({

Who's supposed to call this?

Do I understand correctly that it's the client of the module?

::: toolkit/modules/AsyncStartup.jsm:146
(Diff revision 2)
> +   */
> +  defineLazyModuleGetter(targetObject, name, moduleUrl, symbol) {
> +    let moduleHolder = {};
> +    XPCOMUtils.defineLazyModuleGetter(moduleHolder, name, moduleUrl, symbol);
> +
> +    // The actual getter is not lazy, since it may refresh at every call.

I don't understand that remark.

::: toolkit/modules/AsyncStartup.jsm:164
(Diff revision 2)
> +  get(target, name) {
> +    let module = gModules.get(name);
> +    if (!module) {
> +      module = new AsyncStartupModule();
> +      gModules.set(name, module);
> +    }

I suspect that we want to impose a timeout for the loading of the module, to convert startup hangs into crashes.
After a conversation with Marco I'm going to rename the module to Integration.jsm and rename a few methods. I'll write some basic documentation in the header, then ask for review again. Thanks for the feedback!
Summary: Design an AsyncStartup module for low-overhead registration of integration points → Implement an Integration.jsm module for low-overhead registration of overrides
Comment on attachment 8732997 [details]
MozReview Request: Bug 1094201 - Implement an Integration.jsm module for low-overhead registration of overrides. r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41499/diff/2-3/
Attachment #8732997 - Attachment description: MozReview Request: Bug 1094201 - Design an AsyncStartup module for low-overhead registration of integration points. r=Yoric → MozReview Request: Bug 1094201 - Implement an Integration.jsm module for low-overhead registration of overrides. r=Yoric
Attachment #8732997 - Flags: review?(dteller)
Okay, this has more inline documentation than code now, so it should be more approachable :-)

I think it's better to treat this update as a new blank review, so I've dropped the previous comments.

The patch in the consumer bug is not updated yet, but it won't change much except for the renames.
And the patch in bug 899013 is also updated now.
Comment on attachment 8732997 [details]
MozReview Request: Bug 1094201 - Implement an Integration.jsm module for low-overhead registration of overrides. r=mak

Adding Marco as a possible reviewer, he's also reviewing the two bugs this depends on.
Attachment #8732997 - Flags: review?(mak77)
I have a long backlog, so I'm not sure how much time I may take to review this. Likely it's days.
I'm sorry, it looks like I have difficulties finding time to review this patch. I'll try this week.
Comment on attachment 8732997 [details]
MozReview Request: Bug 1094201 - Implement an Integration.jsm module for low-overhead registration of overrides. r=mak

https://reviewboard.mozilla.org/r/41499/#review42953

r=me with the issues addressed.

::: toolkit/components/formautofill/FormAutofill.jsm:83
(Diff revision 3)
> + * object using the registerIntegration method.
> + */
> +Integration.formAutofill.defineModuleGetter(this.FormAutofill,
> +            "integration",
> +            "resource://gre/modules/FormAutofillIntegration.jsm",
> +            "FormAutofillIntegration");

nit: just indent by 2 spaces, as usual.

::: toolkit/modules/Integration.jsm:97
(Diff revision 3)
> + * for example a "state" property, you can safely use it across registrations.
> + *
> + * Integration overrides provided by restartless add-ons should not use the
> + * "this" reference to store state, to avoid conflicts with other add-ons.
> + *
> + * Providing the combined object as an argument to any XPCOM method will

Is this still part of state handling? maybe it wants its own title, like notes

::: toolkit/modules/moz.build
(Diff revision 3)
>  ]
>  
>  SPHINX_TREES['toolkit_modules'] = 'docs'
>  
>  EXTRA_JS_MODULES += [
> -    'addons/MatchPattern.jsm',

I'm not sure I understand this change

::: toolkit/modules/tests/xpcshell/TestIntegration.jsm:2
(Diff revision 3)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */

nit: can be omitted in test files.

::: toolkit/modules/tests/xpcshell/test_Integration.js:2
(Diff revision 3)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */

nit: can be omitted in test files.

::: toolkit/modules/tests/xpcshell/test_Integration.js:6
(Diff revision 3)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/*
> + * Tests the Integration.jsm module.
> + */

could you please also briefly document what the test does and tries to ensure?

::: toolkit/modules/tests/xpcshell/test_Integration.js:13
(Diff revision 3)
> +"use strict";
> +
> +const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
> +
> +Cu.import("resource://gre/modules/Integration.jsm", this);
> +Cu.import("resource://gre/modules/Promise.jsm", this);

doesn't seem to be used?

::: toolkit/modules/tests/xpcshell/test_Integration.js:86
(Diff revision 3)
> +    // We cannot use the "super" keyword in methods defined using "Task.async".
> +    return "overridden-" + (yield base.asyncMethod.apply(this, arguments));
> +  }),
> +});
> +
> +function* assertCombinedResults(combined, times) {

please add some javadoc.

Off-hand I think times is a confusing name alone. I'd go for a very descriptive howManyOverrides or overridesCount.

::: toolkit/modules/tests/xpcshell/test_Integration.js:106
(Diff revision 3)
> +  Assert.equal(yield combined.asyncMethod("-argument"),
> +               prefix + "asyncMethod-argument");
> +  Assert.equal(combined.asyncMethodArgument, "-argument");
> +}
> +
> +function* assertCurrentCombinedResults(times) {

document
Attachment #8732997 - Flags: review?(mak77) → review+
Thanks for the review! Will land after a try build.

(In reply to Marco Bonardo [::mak] from comment #18)
> > -    'addons/MatchPattern.jsm',
> 
> I'm not sure I understand this change

Merge artifact, already fixed.

> > +/* Any copyright is dedicated to the Public Domain.
> > + * http://creativecommons.org/publicdomain/zero/1.0/ */
> 
> nit: can be omitted in test files.

I prefer keeping these two lines, also matches other files in the folder.

> > +/*
> > + * Tests the Integration.jsm module.
> > + */
> 
> could you please also briefly document what the test does and tries to
> ensure?

The descriptions are in the individual test cases, not sure what I could add here that is not obvious. Note that this comment is pretty obvious and doesn't give any additional information, but avoids the MXR logic just picking whatever arbitrary comment comes next to use as the description of the entire file.
Comment on attachment 8732997 [details]
MozReview Request: Bug 1094201 - Implement an Integration.jsm module for low-overhead registration of overrides. r=mak

https://reviewboard.mozilla.org/r/41499/#review41299

I'm sorry for the long delay, I was in deep deadline mode.

I haven't had time to look at everything yet, so let me publish a review of Integration.jsm and test_Integration.js, I'll try to complete this later (hopefully by tomorrow) with the rest of the patch.

I believe that this module can be very useful and the code looks good. My main preoccupation when seeing this module is that it is going to have far-reaching implications for any code that uses it – and will be pretty useless if people don't actually use it. In other words, I believe that it deserves a discussion on one of the mailing-lists (I'm not sure which is best) and/or a super-review. It would be great if we had a RFC process for toolkit/ or for mozilla-central/, but sadly, we don't.

Also, not marking this as "Ship It" yet just because I haven't finished looking at the code.

::: toolkit/modules/Integration.jsm:9
(Diff revision 3)
> +
> +/*
> + * Implements low-overhead integration between components of the application.
> + * This may have different uses depending on the component, including:
> + *
> + * - Providing product-specific implementations registered at startup.

Ahah, so that was what you had in mind :)

Now, this sounds useful. It's a shame that we do not have a RFC process for toolkit/, otherwise I believe that this would be a good place to discuss this module.

It would be great if you could someone from WebExtensions on board, too, to see if this would fulfill some of their needs. Otherwise, let's not dwell on allowing add-ons to change specific behaviors, as we're going towards WebExtensions.

::: toolkit/modules/Integration.jsm:39
(Diff revision 3)
> + *     getTemporaryDirectory() {
> + *       return base.getTemporaryDirectory.call(this) + "subdir/";
> + *     },
> + *   }));
> + *
> + * When the first component needs to call a method on the integration object,

First component? That's not very clear to me.

Also, nit: I'm not a big fan of the word `getCombined`. Do you know if any comparable mechanism exists in the Node/CommonJS community? If so, it might be a good idea to reuse the same vocable (and too bad for me if that vocable is already `getCombined` :) ). Otherwise, maybe `build`? `compile`?

::: toolkit/modules/Integration.jsm:116
(Diff revision 3)
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +/**
> + * Maps integration point names to IntegrationPoint objects.
> + */
> +const gIntegrationPoints = new Map();

Open question: is there a chance of memory leaks caused by misuse of this API?

::: toolkit/modules/Integration.jsm:143
(Diff revision 3)
> + */
> +this.IntegrationPoint = function () {
> +  this._overrideFns = new Set();
> +  this._combined = {
> +    QueryInterface: function() {
> +      let ex = new Components.Exception(

Good idea.

::: toolkit/modules/Integration.jsm:157
(Diff revision 3)
> +
> +this.IntegrationPoint.prototype = {
> +  /**
> +   * Ordered set of registered functions defining integration overrides.
> +   */
> +  _overrideFns: null,

This looks like an instance variable. What is this doing in the prototype? Is this purely for documentation purposes?

Same question for the fields below.

::: toolkit/modules/tests/xpcshell/test_Integration.js:86
(Diff revision 3)
> +    // We cannot use the "super" keyword in methods defined using "Task.async".
> +    return "overridden-" + (yield base.asyncMethod.apply(this, arguments));
> +  }),
> +});
> +
> +function* assertCombinedResults(combined, times) {

Nit: doc needed.
Attachment #8732997 - Flags: review?(dteller)
[Sorry, I forgot to use MozReview for replying!]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #21)
> I haven't had time to look at everything yet, so let me publish a review of
> Integration.jsm and test_Integration.js, I'll try to complete this later
> (hopefully by tomorrow) with the rest of the patch.

Thanks David! Don't worry about doing the review in a hurry, I don't think this is really blocked on landing for the reasons I outline below, and Marco has also looked at this code and interface.

> I believe that this module can be very useful and the code looks good. My
> main preoccupation when seeing this module is that it is going to have
> far-reaching implications for any code that uses it – and will be pretty
> useless if people don't actually use it. In other words, I believe that it
> deserves a discussion on one of the mailing-lists (I'm not sure which is
> best) and/or a super-review. It would be great if we had a RFC process for
> toolkit/ or for mozilla-central/, but sadly, we don't.

This system already existed for FormAutofillIntegration and was long planned for DownloadIntegration, so we already have two consumers for it. The alternative to having this file in the "modules" directory with its own tests would be to replicate the same functionality locally in the "formautofill" and "jsdownloads" folders, which is sub-optimal.

I agree with your comments on this as a general purpose module. I think we should allow testing and scrutiny before this module is generally advertised as the way to go. In other words, I'm not concerned about people not actually using it at this stage, because its presence solves an existing problem.

If as a result of a discussion on a mailing list we find a better or more standardized way to achieve the same goals, we can always migrate DownloadIntegration and FormAutofillIntegration to those new systems. Maybe in the future we will integrate a fully-fledged dependency injection container.

But I don't think we should block the immediate gain, including the powerful test refactoring in the Downloads.jsm code base that this solution has made possible, on a general discussion on the topic.

> It would be great if you could someone from WebExtensions on board, too, to
> see if this would fulfill some of their needs. Otherwise, let's not dwell on
> allowing add-ons to change specific behaviors, as we're going towards
> WebExtensions.

Thanks for the question :-) An integration override for Downloads.jsm is exactly the way WebExtension developers will go for listening and intercepting requests to assign a file name to new downloads.

To clarify, this "integration" system itself is not a part of the WebExtensions API, but the implementation of the "chrome.downloads" WebExtensions API will register an integration override so they don't have to code WebExtension-specific logic or use #ifdefs in Downloads.jsm.

> Also, nit: I'm not a big fan of the word `getCombined`. Do you know if any
> comparable mechanism exists in the Node/CommonJS community? If so, it might
> be a good idea to reuse the same vocable (and too bad for me if that vocable
> is already `getCombined` :) ). Otherwise, maybe `build`? `compile`?

Marco looked at a few related frameworks and getCombined was one of the existing terms used for the purpose. so we went for that, although I'm sure we could always nitpick about the exact semantics being different in the two cases :-)

> Open question: is there a chance of memory leaks caused by misuse of this
> API?

Not anymore than misusing any other observer-style registration.

> This looks like an instance variable. What is this doing in the prototype?
> Is this purely for documentation purposes?

Yes, just for JSDoc-style documentation.

> > +function* assertCombinedResults(combined, times) {
> 
> Nit: doc needed.

Added to the latest version on try. I'll now publish this to MozReview too.
Comment on attachment 8732997 [details]
MozReview Request: Bug 1094201 - Implement an Integration.jsm module for low-overhead registration of overrides. r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41499/diff/3-4/
Attachment #8732997 - Attachment description: MozReview Request: Bug 1094201 - Implement an Integration.jsm module for low-overhead registration of overrides. r=Yoric → MozReview Request: Bug 1094201 - Implement an Integration.jsm module for low-overhead registration of overrides. r=mak
Comment on attachment 8732997 [details]
MozReview Request: Bug 1094201 - Implement an Integration.jsm module for low-overhead registration of overrides. r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41499/diff/4-5/
https://hg.mozilla.org/mozilla-central/rev/9c7198e76b2d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
This sounds like a thing we should document, since I only found out about it today in passing, and it sounds pretty useful.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.