Port XPCOMUtils.defineLazyGetter to ChromeUtils ?
Categories
(Core :: XPConnect, enhancement)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox112 | --- | fixed |
People
(Reporter: arai, Assigned: arai)
References
Details
Attachments
(2 files)
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.
|
Assignee | |
Comment 1•2 years ago
|
||
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.
|
|
Assignee | |
Comment 2•2 years ago
|
||
Looks like there are at least 5 implementations of defineLazyGetter in non-test code.
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,
});
},
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,
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,
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,
});
};
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,
|
|
Assignee | |
Comment 3•2 years ago
|
||
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
|
|
Assignee | |
Comment 4•2 years ago
|
||
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
|
|
Assignee | |
Comment 5•2 years ago
|
||
|
|
Assignee | |
Comment 6•2 years ago
|
||
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 8•2 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/e717e0354d72
https://hg.mozilla.org/mozilla-central/rev/f7a22635f81a
|
||
Comment 10•2 years ago
|
||
Backed out for causing Bug 1820250
Backout link: https://hg.mozilla.org/integration/autoland/rev/056c6b740f51f26df3f439385200f3d78bd02a1f
|
|
Assignee | |
Comment 11•2 years ago
|
||
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:
XPCOMUtils.defineLazyServiceGetters(ShellServiceInternal, {
shellService: ["@mozilla.org/browser/shell-service;1", "nsIShellService"],
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•2 years ago
|
||
Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/056c6b740f51
|
|
Assignee | |
Comment 13•2 years ago
|
||
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.
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:
ShellServiceInternal.shellServiceis defined bydefineLazyServiceGetters- when
ShellServiceInternal.shellServiceis first accessed:
1.redefiningis set totrue
2. thegetServiceinside the getter throws, and it's propagated to the caller - when
ShellServiceInternal.shellServiceis accessed again:
1. becauseredefiningis alreadytrue,undefinedis 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.
|
||
Updated•2 years ago
|
|
|
||
Updated•2 years ago
|
|
|
Assignee | |
Comment 14•2 years ago
|
||
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:
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•2 years ago
|
||
|
||
Comment 16•2 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/a09d7f956078
https://hg.mozilla.org/mozilla-central/rev/64b0a4a734ea
Description
•