fix: Lazily initialize API via the original target (#71)
Originally, the polyfill created a Proxy with the original API object as the target. This was changed to `Object.create(chrome)` because not doing so would prevent the `browser.devtools` API from working because the devtools API object is assigned as a read-only & non-configurable property (#57). However, that action itself caused a new bug: Whenever an API object is dereferenced via the `browser` namespace, the original API is no longer available in the `chrome` namespace, and trying to access the API through `chrome` returns `undefined` plus the "Previous API instantiation failed" warning (#58). This is because Chrome lazily initializes fields in the `chrome` API, but on the object from which the property is accessed, while the polyfill accessed the property through an object with the prototype set to `chrome` instead of directly via chrome. To fix that, `Object.create(chrome)` was replaced with `Object.assign({}, chrome)`. This fixes both of the previous issues because 1) It is still a new object. 2) All lazily initialized fields are explicitly initialized. This fix created a new issue: In Chrome some APIs cannot be used even though they are visible in the API (e.g. `chrome.clipboard`), so calling `Object.assign({}, chrome)` causes an error to be printed to the console (#70). To solve this, I use `Object.create(chrome)` again as a proxy target, but dereference the API via the original target (`chrome`) to not regress on #58. Besides fixing the bug, this also reduces the performance impact of the API because all API fields are lazily initialized again, instead of upon start-up. This fixes #70.
This commit is contained in:
parent
94efb908b8
commit
67b3780d38
|
@ -191,13 +191,12 @@ if (typeof browser === "undefined") {
|
||||||
*/
|
*/
|
||||||
const wrapObject = (target, wrappers = {}, metadata = {}) => {
|
const wrapObject = (target, wrappers = {}, metadata = {}) => {
|
||||||
let cache = Object.create(null);
|
let cache = Object.create(null);
|
||||||
|
|
||||||
let handlers = {
|
let handlers = {
|
||||||
has(target, prop) {
|
has(proxyTarget, prop) {
|
||||||
return prop in target || prop in cache;
|
return prop in target || prop in cache;
|
||||||
},
|
},
|
||||||
|
|
||||||
get(target, prop, receiver) {
|
get(proxyTarget, prop, receiver) {
|
||||||
if (prop in cache) {
|
if (prop in cache) {
|
||||||
return cache[prop];
|
return cache[prop];
|
||||||
}
|
}
|
||||||
|
@ -253,7 +252,7 @@ if (typeof browser === "undefined") {
|
||||||
return value;
|
return value;
|
||||||
},
|
},
|
||||||
|
|
||||||
set(target, prop, value, receiver) {
|
set(proxyTarget, prop, value, receiver) {
|
||||||
if (prop in cache) {
|
if (prop in cache) {
|
||||||
cache[prop] = value;
|
cache[prop] = value;
|
||||||
} else {
|
} else {
|
||||||
|
@ -262,16 +261,27 @@ if (typeof browser === "undefined") {
|
||||||
return true;
|
return true;
|
||||||
},
|
},
|
||||||
|
|
||||||
defineProperty(target, prop, desc) {
|
defineProperty(proxyTarget, prop, desc) {
|
||||||
return Reflect.defineProperty(cache, prop, desc);
|
return Reflect.defineProperty(cache, prop, desc);
|
||||||
},
|
},
|
||||||
|
|
||||||
deleteProperty(target, prop) {
|
deleteProperty(proxyTarget, prop) {
|
||||||
return Reflect.deleteProperty(cache, prop);
|
return Reflect.deleteProperty(cache, prop);
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|
||||||
return new Proxy(target, handlers);
|
// Per contract of the Proxy API, the "get" proxy handler must return the
|
||||||
|
// original value of the target if that value is declared read-only and
|
||||||
|
// non-configurable. For this reason, we create an object with the
|
||||||
|
// prototype set to `target` instead of using `target` directly.
|
||||||
|
// Otherwise we cannot return a custom object for APIs that
|
||||||
|
// are declared read-only and non-configurable, such as `chrome.devtools`.
|
||||||
|
//
|
||||||
|
// The proxy handlers themselves will still use the original `target`
|
||||||
|
// instead of the `proxyTarget`, so that the methods and properties are
|
||||||
|
// dereferenced via the original targets.
|
||||||
|
let proxyTarget = Object.create(target);
|
||||||
|
return new Proxy(proxyTarget, handlers);
|
||||||
};
|
};
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
@ -348,12 +358,7 @@ if (typeof browser === "undefined") {
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|
||||||
// Create a new empty object and copy the properties of the original chrome object
|
return wrapObject(chrome, staticWrappers, apiMetadata);
|
||||||
// to prevent a Proxy violation exception for the devtools API getter
|
|
||||||
// (which is a read-only non-configurable property on the original target).
|
|
||||||
const targetObject = Object.assign({}, chrome);
|
|
||||||
|
|
||||||
return wrapObject(targetObject, staticWrappers, apiMetadata);
|
|
||||||
};
|
};
|
||||||
|
|
||||||
// The build process adds a UMD wrapper around this file, which makes the
|
// The build process adds a UMD wrapper around this file, which makes the
|
||||||
|
|
|
@ -136,4 +136,42 @@ describe("browser-polyfill", () => {
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe("without side effects", () => {
|
||||||
|
it("should proxy non-wrapped methods", () => {
|
||||||
|
let lazyInitCount = 0;
|
||||||
|
const fakeChrome = {
|
||||||
|
get runtime() {
|
||||||
|
// Chrome lazily initializes API objects by replacing the getter with
|
||||||
|
// the value. The initialization is only allowed to occur once,
|
||||||
|
// after that `undefined` is returned and a warning is printed.
|
||||||
|
// https://chromium.googlesource.com/chromium/src/+/4d6b3a067994ce6dcf0ed9a9efd566c083736952/extensions/renderer/module_system.cc#414
|
||||||
|
//
|
||||||
|
// The polyfill should invoke the getter only once (on the global chrome object).
|
||||||
|
++lazyInitCount;
|
||||||
|
|
||||||
|
const onMessage = {
|
||||||
|
addListener(listener) {
|
||||||
|
equal(this, onMessage, "onMessage.addListener should be called on the original chrome.onMessage object");
|
||||||
|
},
|
||||||
|
};
|
||||||
|
const value = {onMessage};
|
||||||
|
Object.defineProperty(this, "runtime", {value});
|
||||||
|
return value;
|
||||||
|
},
|
||||||
|
};
|
||||||
|
return setupTestDOMWindow(fakeChrome).then(window => {
|
||||||
|
equal(lazyInitCount, 0, "chrome.runtime should not be initialized without explicit API call");
|
||||||
|
|
||||||
|
window.browser.runtime.onMessage.addListener(() => {});
|
||||||
|
equal(lazyInitCount, 1, "chrome.runtime should be initialized upon accessing browser.runtime");
|
||||||
|
|
||||||
|
window.browser.runtime.onMessage.addListener(() => {});
|
||||||
|
equal(lazyInitCount, 1, "chrome.runtime should be re-used upon accessing browser.runtime");
|
||||||
|
|
||||||
|
window.chrome.runtime.onMessage.addListener(() => {});
|
||||||
|
equal(lazyInitCount, 1, "chrome.runtime should be re-used upon accessing chrome.runtime");
|
||||||
|
});
|
||||||
|
});
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
Loading…
Reference in New Issue