From 67b3780d381fe943b5cfe4d81708a6389b796a1b Mon Sep 17 00:00:00 2001 From: Rob Wu Date: Mon, 12 Mar 2018 19:23:28 +0100 Subject: [PATCH] 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. --- src/browser-polyfill.js | 31 ++++++++++++++++----------- test/test-proxied-properties.js | 38 +++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 13 deletions(-) diff --git a/src/browser-polyfill.js b/src/browser-polyfill.js index bb477fb..6178877 100644 --- a/src/browser-polyfill.js +++ b/src/browser-polyfill.js @@ -191,13 +191,12 @@ if (typeof browser === "undefined") { */ const wrapObject = (target, wrappers = {}, metadata = {}) => { let cache = Object.create(null); - let handlers = { - has(target, prop) { + has(proxyTarget, prop) { return prop in target || prop in cache; }, - get(target, prop, receiver) { + get(proxyTarget, prop, receiver) { if (prop in cache) { return cache[prop]; } @@ -253,7 +252,7 @@ if (typeof browser === "undefined") { return value; }, - set(target, prop, value, receiver) { + set(proxyTarget, prop, value, receiver) { if (prop in cache) { cache[prop] = value; } else { @@ -262,16 +261,27 @@ if (typeof browser === "undefined") { return true; }, - defineProperty(target, prop, desc) { + defineProperty(proxyTarget, prop, desc) { return Reflect.defineProperty(cache, prop, desc); }, - deleteProperty(target, prop) { + deleteProperty(proxyTarget, 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 - // 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); + return wrapObject(chrome, staticWrappers, apiMetadata); }; // The build process adds a UMD wrapper around this file, which makes the diff --git a/test/test-proxied-properties.js b/test/test-proxied-properties.js index fac13f5..e6935a8 100644 --- a/test/test-proxied-properties.js +++ b/test/test-proxied-properties.js @@ -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"); + }); + }); + }); });