Port XPCOMUtils.defineLazyGetter to ChromeUtils ?
Categories
(Core :: XPConnect, enhancement)
Tracking
()
Tracking | Status | |
---|---|---|
firefox112 | --- | fixed |
People
(Reporter: arai, Assigned: arai)
References
(Blocks 1 open bug)
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.shellService
is defined bydefineLazyServiceGetters
- when
ShellServiceInternal.shellService
is first accessed:
1.redefining
is set totrue
2. thegetService
inside the getter throws, and it's propagated to the caller - when
ShellServiceInternal.shellService
is accessed again:
1. becauseredefining
is alreadytrue
,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.
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
•