1805288 - Port XPCOMUtils.defineLazyGetter to ChromeUtils ?

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Description

[Tooru Fujisawa [:arai]]

sometimes XPCOMUtils is imported only to define lazy getter by XPCOMUtils.defineLazyGetter.

given ChromeUtils.defineModuleGetter provides similar functionality, we might be able to provide defineLazyGetter from ChromeUtils, while sharing the underlying implementation,
so that no extra module needs to be imported.

Comment 1

[Tooru Fujisawa [:arai]]

in the current tree, ignoring XPCOMUtils.{defineLazyModuleGetter,defineLazyModuleGetters} consumers, there are 789 files that has reference to XPCOMUtils.define* functions, and 392 of them use XPCOMUtils.defineLazyGetter only, and 229 files within them seems to be non-test.
So, introducing ChromeUtils.defineLazyGetter would be beneficial.

Comment 2

[Tooru Fujisawa [:arai]]

Looks like there are at least 5 implementations of defineLazyGetter in non-test code.

https://searchfox.org/mozilla-central/rev/6444ed17e9f5e0d4e7dabc13c930d46b529fca15/js/xpconnect/loader/XPCOMUtils.sys.mjs#24-32,34,46-61

function redefine(object, prop, value) {
  Object.defineProperty(object, prop, {
    configurable: true,
    enumerable: true,
    value,
    writable: true,
  });
  return value;
}
...
export var XPCOMUtils = {
...
  defineLazyGetter(aObject, aName, aLambda) {
    let redefining = false;
    Object.defineProperty(aObject, aName, {
      get() {
        if (!redefining) {
          // Make sure we don't get into an infinite recursion loop if
          // the getter lambda does something shady.
          redefining = true;
          return redefine(aObject, aName, aLambda.apply(aObject));
        }
        return undefined;
      },
      configurable: true,
      enumerable: true,
    });
  },

https://searchfox.org/mozilla-central/rev/6444ed17e9f5e0d4e7dabc13c930d46b529fca15/toolkit/components/extensions/ExtensionCommon.jsm#148-171,2849,2863

function defineLazyGetter(object, prop, getter) {
  let redefine = (obj, value) => {
    Object.defineProperty(obj, prop, {
      enumerable: true,
      configurable: true,
      writable: true,
      value,
    });
    return value;
  };

  Object.defineProperty(object, prop, {
    enumerable: true,
    configurable: true,

    get() {
      return redefine(this, getter.call(this));
    },

    set(value) {
      redefine(this, value);
    },
  });
}
...
ExtensionCommon = {
...
  defineLazyGetter,

https://searchfox.org/mozilla-central/rev/6444ed17e9f5e0d4e7dabc13c930d46b529fca15/devtools/shared/loader/builtin-modules.js#31-51,156,158-159

function defineLazyGetter(object, name, lambda) {
  Object.defineProperty(object, name, {
    get() {
      // Redefine this accessor property as a data property.
      // Delete it first, to rule out "too much recursion" in case object is
      // a proxy whose defineProperty handler might unwittingly trigger this
      // getter again.
      delete object[name];
      const value = lambda.apply(object);
      Object.defineProperty(object, name, {
        value,
        writable: true,
        configurable: true,
        enumerable: true,
      });
      return value;
    },
    configurable: true,
    enumerable: true,
  });
}
...
exports.globals = {
...
  loader: {
    lazyGetter: defineLazyGetter,

https://searchfox.org/mozilla-central/rev/6444ed17e9f5e0d4e7dabc13c930d46b529fca15/devtools/shared/DevToolsUtils.js#406-416

exports.defineLazyGetter = function(object, name, lambda) {
  Object.defineProperty(object, name, {
    get() {
      delete object[name];
      object[name] = lambda.apply(object);
      return object[name];
    },
    configurable: true,
    enumerable: true,
  });
};

https://searchfox.org/mozilla-central/rev/6444ed17e9f5e0d4e7dabc13c930d46b529fca15/toolkit/components/osfile/modules/osfile_shared_allthreads.jsm#47,63,103-116,1329,1331,1340

var EXPORTED_SYMBOLS = [
...
  "defineLazyGetter",
...
var defineLazyGetter = function defineLazyGetter(object, name, getter) {
  Object.defineProperty(object, name, {
    configurable: true,
    get: function lazy() {
      delete this[name];
      let value = getter.call(this);
      Object.defineProperty(object, name, {
        value,
      });
      return value;
    },
  });
};
exports.defineLazyGetter = defineLazyGetter;
...
exports.OS = {
...
  Shared: {
...
    defineLazyGetter,

Comment 3

[Tooru Fujisawa [:arai]]

there are slight differences between those implementation,
especially the one in ExtensionCommon.jsm supports setter.

maybe better checking each consumer to see which behavior is actually necessary,
and replace all of them with the new ChromeUtils.defineLazyGetter

Comment 4

[Tooru Fujisawa [:arai]]

testing a patch to replace all implementation
https://treeherder.mozilla.org/jobs?repo=try&revision=62a25ee1ab2a22a619b1de1275ea9d6933af5fa4

and also to check if the setter in ExtensionCommon.jsm is called
https://treeherder.mozilla.org/jobs?repo=try&revision=5d0733528f91e987038eae806b1f2c35ad88253b

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Bug 1805288 - Part 1: Add ChromeUtils.defineLazyGetter. r=smaug!

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Bug 1805288 - Part 2: Use ChromeUtils.defineLazyGetter from XPCOMUtils.defineLazyGetter. r=smaug!

Also fix test_safe-getter.js not to use defineLazyGetter, given the
test expects the getter function be scripted function, while the getter
function created by ChromeUtils.defineLazyGetter is native.

Rewriting XPCOMUtils.defineLazyGetter consumers should be done in separate bugs.

Depends on D171319

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e717e0354d72
Part 1: Add ChromeUtils.defineLazyGetter. r=smaug
https://hg.mozilla.org/integration/autoland/rev/f7a22635f81a
Part 2: Use ChromeUtils.defineLazyGetter from XPCOMUtils.defineLazyGetter. r=smaug

Comment 8

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/e717e0354d72
https://hg.mozilla.org/mozilla-central/rev/f7a22635f81a

Comment 9

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/056c6b740f51
Backed out 2 changesets for causing Bug 1820250. CLOSED TREE

Comment 10

[Atila Butkovits]

Backed out for causing Bug 1820250

Backout link: https://hg.mozilla.org/integration/autoland/rev/056c6b740f51f26df3f439385200f3d78bd02a1f

Comment 11

[Tooru Fujisawa [:arai]]

Now looking into bug 1820250, and here's what I found so far:

When ChromeUtils.defineLazyGetter is used by XPCOMUtils.defineLazyGetter, defineLazyServiceGetter for nsIShellService fails while calling GetService:

https://searchfox.org/mozilla-central/rev/00ea1649b59d5f427979e2d6ba42be96f62d6e82/browser/components/shell/ShellService.jsm#472-473

XPCOMUtils.defineLazyServiceGetters(ShellServiceInternal, {
  shellService: ["@mozilla.org/browser/shell-service;1", "nsIShellService"],

https://searchfox.org/mozilla-central/rev/00ea1649b59d5f427979e2d6ba42be96f62d6e82/js/xpconnect/loader/XPCOMUtils.sys.mjs#134,137

defineLazyServiceGetter(aObject, aName, aContract, aInterfaceName) {
...
      return Cc[aContract].getService(Ci[aInterfaceName]);

and that seems to cause the test to fail.

but when I change that specific call for XPCOMUtils.defineLazyGetter to use the old implementation, the GetService still fails, but the test passes.
and it seems to be that the timing of the call is slightly different.

I wonder if there's some code that lets the test harness to ignore the specific error, and the change to the stack or something causes the filter stops working...?

Comment 12

[Cristian Tuns]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/056c6b740f51

Comment 13

[Tooru Fujisawa [:arai]]

looks like the test was passing by hitting edge case in the old implementation.

here's the code before the patch, that has guard against redefinition.

https://searchfox.org/mozilla-central/rev/e1da0e81bb105ca9d328aaf0cc66f5be5b480458/js/xpconnect/loader/XPCOMUtils.sys.mjs#34,46-54

export var XPCOMUtils = {
...
  defineLazyGetter(aObject, aName, aLambda) {
    let redefining = false;
    Object.defineProperty(aObject, aName, {
      get() {
        if (!redefining) {
          // Make sure we don't get into an infinite recursion loop if
          // the getter lambda does something shady.
          redefining = true;
          return redefine(aObject, aName, aLambda.apply(aObject));

In the testcase's scenario, the following happens:

1. ShellServiceInternal.shellService is defined by defineLazyServiceGetters
2. when ShellServiceInternal.shellService is first accessed:
   1. redefining is set to true
   2. the getService inside the getter throws, and it's propagated to the caller
3. when ShellServiceInternal.shellService is accessed again:
   1. because redefining is already true, undefined is returned

the new implementation in the patch doesn't have the guard against redefinition,
and the error is thrown each time, and that's caught by the test harness during the test.

I'll see if I can add a lightweight redefinition guard in the native function.

Comment 14

[Tooru Fujisawa [:arai]]

there was one more issue with the new implementation.

The the patch was defining the property on the this object of the getter, that can be different if the lazy getter is defined on prototype object.
the following case hits the issue:

https://searchfox.org/mozilla-central/rev/00ea1649b59d5f427979e2d6ba42be96f62d6e82/toolkit/components/extensions/Extension.jsm#2365-2368

XPCOMUtils.defineLazyGetter(
  BootstrapScope.prototype,
  "BOOTSTRAP_REASON_TO_STRING_MAP",
  () => {

The lazy getter's result should be defined on the target object BootstrapScope.prototype, but it was defined on the this object, that is BootstrapScope instance.
I've fixed the patch to store the target object also to the generated function and let the lazy getter define the property on the target object.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a09d7f956078
Part 1: Add ChromeUtils.defineLazyGetter. r=smaug
https://hg.mozilla.org/integration/autoland/rev/64b0a4a734ea
Part 2: Use ChromeUtils.defineLazyGetter from XPCOMUtils.defineLazyGetter. r=smaug,devtools-reviewers,ochameau

Comment 16

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/a09d7f956078
https://hg.mozilla.org/mozilla-central/rev/64b0a4a734ea