1457988 - XPCOMUtils.defineLazyProxy (was: lazy listeners for the content process)

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P1)

Description

[:Felipe Gomes (needinfo for replies!)]

nsBrowserGlue already has something similar, here:
https://searchfox.org/mozilla-central/rev/08df4e6e11284186d477d7e5b0ae48483ecc979c/browser/components/nsBrowserGlue.js#186

We should create some infrastructure to do this in the content process, and start moving things out of the main scripts into separate scripts that are only used when needed.

One slightly difficult aspect is that the existing implementation in nsBrowserGlue doesn't cover the case for event listeners, and when we do that, we'll need to pay attention into the cases where it's just a function (to call it directly) vs. when it's an object with handleEvent. This is done automagically by XPCOM now but if we might have to handle it manually on the first event.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Hey all, apologies for the delay here.. I went back and forth on this for a while, but I arrived at something that I think will be really useful in general, and not just for frame scripts in the content process.

Story time if you're curious (feel free to skip this comment otherwise):

I started following a similar pattern from what's in nsBrowserGlue (linked above) and to adapt it to the use cases of the stuff found in the content scripts.

It worked but it had shortcomings:
 - the changes needed to make on the current code were more verbose and more often required changes in behavior, specially around managing the adding/removal of listeners

- dealing with repeated msgs is harder here than what's covered in nsBrowserGlue, because events make it harder: two different callers might want to add a listener for the same event, but with different parameters (e.g. bubbling vs capture). Having a central object become the de-facto listener and correctly find and dispatch the real listeners for each case quickly became unwieldy.

- handling all different use cases (module singleton, singleton + .init() function, exported constructor) made things weird

- the lifetime of stuff is harder to reason about when the real listener is a central object and it keeps references to all other objects


So, after I had a lot working, I had an idea to go in another direction using Proxies, and once it worked I scrapped all that I had from the above

Comment 2

[:Felipe Gomes (needinfo for replies!)]

The better thing to have would be a way to define a lazy getter to be passed for an event listener/observer/msg listener, that is evaluated only when it'll be called.

The lazy getters provided by XPCOMUtils are not enough for this, because simply passing one as a reference to e.g. addEventListener will cause its getter to run.

So what we really need is some sort of lazy getter that is resilient to be passed around by reference, and is only evaluated when really used.

I went in this direction and implemented it using proxies, which can be passed around. As soon as any trap gets called, it will evaluate the lazy getter and forward everything to the underlying object.


(There are many more details in the implementation that cover the use cases needed for a lot of the stuff that I'm lazy-fying in content scripts, which I'll be doing in separate bugs)

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1457988 - Implement XPCOMUtils.defineLazyProxy.

This implements a new kind of lazy getter in XPCOMUtils that creates an object (implemented as a JS Proxy) that is resilient to be passed around as references to other functions, and will only evaluate the getter when it really needs to be used

Review commit: https://reviewboard.mozilla.org/r/243690/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243690/

Comment 4

[Kris Maglione [:kmag]]

(In reply to :Felipe Gomes (needinfo me!) from comment #0)
> nsBrowserGlue already has something similar, here:
> https://searchfox.org/mozilla-central/rev/
> 08df4e6e11284186d477d7e5b0ae48483ecc979c/browser/components/nsBrowserGlue.
> js#186
> 
> We should create some infrastructure to do this in the content process, and
> start moving things out of the main scripts into separate scripts that are
> only used when needed.
> 
> One slightly difficult aspect is that the existing implementation in
> nsBrowserGlue doesn't cover the case for event listeners, and when we do
> that, we'll need to pay attention into the cases where it's just a function
> (to call it directly) vs. when it's an object with handleEvent. This is done
> automagically by XPCOM now but if we might have to handle it manually on the
> first event.

It's worth noting that Nika and I are working on some things for Fission are aimed at making it easier to do this automatically. If there are specific cases you're especially concerned about, it would be interesting to hear about them so we can make sure they're covered by the new auto-loading infrastructure.

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8975389 [details]
Bug 1457988 - Implement XPCOMUtils.defineLazyProxy.

https://reviewboard.mozilla.org/r/243690/#review249708

Thanks. This looks useful. I just have some nits about performance concerns.

::: js/xpconnect/loader/XPCOMUtils.jsm:623
(Diff revision 1)
> +    let handler = new LazyProxyHandler(aName, aStubProperties, aUntrapCallback);
> +
> +    if (typeof(aInitFuncOrResource) == "string") {
> +      this.defineLazyModuleGetter(handler, "realObject", aInitFuncOrResource, aName);
> +    } else {
> +      this.defineLazyGetter(handler, "realObject", aInitFuncOrResource);

Hm. This also uses `delete` and will put the handler into dictionary mode... :( We should maybe fix that first.

::: js/xpconnect/loader/XPCOMUtils.jsm:661
(Diff revision 1)
> + * stored as `realObject`, which is defined as a lazy getter
> + * and which will be evaulated at the first time that any of these
> + * traps are called (with an exception in the get() trap for
> + * the properties provided in the `aStubProperties` parameter).
> + */
> +function LazyProxyHandler(aName, aStubProperties, aUntrapCallback) {

Please make this an ES6 class

::: js/xpconnect/loader/XPCOMUtils.jsm:673
(Diff revision 1)
> +LazyProxyHandler.prototype = {
> +  getObject() {
> +    if (this.pending) {
> +      this.pending = false;
> +      if (this.untrapCallback) {
> +        this.untrapCallback.call(undefined, this.realObject);

Should be `null` rather than `undefined`

::: js/xpconnect/loader/XPCOMUtils.jsm:674
(Diff revision 1)
> +  getObject() {
> +    if (this.pending) {
> +      this.pending = false;
> +      if (this.untrapCallback) {
> +        this.untrapCallback.call(undefined, this.realObject);
> +        delete this.untrapCallback;

Please don't delete properties. It changes the shape of the object, and generally puts it in dictionary mode, which is bad for the JIT. Just null it out instead.

::: js/xpconnect/loader/XPCOMUtils.jsm:681
(Diff revision 1)
> +      delete this.stubProperties;
> +    }
> +    return this.realObject;
> +  },
> +
> +  getPrototypeOf(...args) {

I hate to say this... but rest args and spread args are pretty expensive and don't have good JIT support at the moment. We should just use the explicit handler args in all of these traps.

::: js/xpconnect/tests/unit/test_lazyproxy.js:51
(Diff revision 1)
> +  //
> +  // Note: Even though Assert.equal uses a shallow
> +  // comparison (==), on success it stringifies the
> +  // two parameters given for the log,
> +  // which enumerates the proxy :S.
> +  Assert.ok(lazyProxy === tmp.myLazyProxy);

`Assert.strictEqual`, please.

Also, please add descriptions for all assertions. Debugging xpcshell failures without assertion descriptions is a nightmare.

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8975389 [details]
Bug 1457988 - Implement XPCOMUtils.defineLazyProxy.

https://reviewboard.mozilla.org/r/243690/#review249708

> Hm. This also uses `delete` and will put the handler into dictionary mode... :( We should maybe fix that first.

Honestly, we should probably just skip the existing lazy getter machinery and just have the proxy handler evaluate this the first time it's needed. This probably really isn't a place where we want to run into additional JIT overhead (having a proxy at all means we won't get ICs for most operations we otherwise would), and the existing lazy getter stuff tends to be pretty hairy, as far as the platform is concerned...

Comment 7

[:Felipe Gomes (needinfo for replies!)]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #5)

Thanks! Addressed all the comments, except:

> > +  Assert.ok(lazyProxy === tmp.myLazyProxy);
> 
> `Assert.strictEqual`, please.

Assert.strictEqual calls Assert.report, which wants to stringify the two objects compared (even when it succeeds), which ends up triggering the proxy evaluation.  I improved the comment a bit to make this more clear.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8975389 [details]
Bug 1457988 - Implement XPCOMUtils.defineLazyProxy.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243690/diff/1-2/

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8975389 [details]
Bug 1457988 - Implement XPCOMUtils.defineLazyProxy.

https://reviewboard.mozilla.org/r/243690/#review249842

Thanks

::: js/xpconnect/loader/XPCOMUtils.jsm:622
(Diff revision 2)
> +  defineLazyProxy: function XPCOMUtils__defineLazyProxy(aObject, aName, aInitFuncOrResource,
> +                                                        aStubProperties, aUntrapCallback) {
> +    let initFunc = aInitFuncOrResource;
> +
> +    if (typeof(aInitFuncOrResource) == "string") {
> +      initFunc = () => ChromeUtils.import(aInitFuncOrResource)[aName];

This is going to wind up defining a bunch of unwanted symbols on the XPCOMUtils global.

Passing `null` as the second arg would fix that problem. But there's another problem, which is that the module loader supports exporting lexical symbols (and also, for that matter, only exporting listed symbols), which are not accessible on the plain module global object.

We should really probably import into a temporary object and get `aName` from that.

::: js/xpconnect/loader/XPCOMUtils.jsm:719
(Diff revision 2)
> +  }
> +
> +  get(target, prop, receiver) {
> +    if (this.pending &&
> +        this.stubProperties &&
> +        this.stubProperties.hasOwnProperty(prop)) {

So, two things. 1) Using `hasOwnProperty` this way can be a bit of a footgun if there's any chance of that being defined on the object itself, and overriding the prototype method. 2) We may actually want to include object prototype properties here so they don't wind up forcing evaluation of the lazy getter.

I don't have that strong an opinion on #2, but for #1, please use something like Object.prototype.hasOwnProperty.call or Object.getOwnPropertyDescriptor.

::: js/xpconnect/loader/XPCOMUtils.jsm:737
(Diff revision 2)
> +  apply(target, thisArg, argsList) {
> +    return Reflect.apply(this.getObject(), thisArg, argsList);
> +  }
> +
> +  construct(target, argsList, newTarget) {
> +    return Reflect.construct(this.getObject(), argsList, newTarget);
> +  }

These aren't necessary. They only work if the target object is a function, which it never is here.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62451c9687b2
Implement XPCOMUtils.defineLazyProxy. r=kmag

Comment 11

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/62451c9687b2