From 7ce6634c51a38cf7d43c4a259cb59cfaa69e45a7 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Mon, 7 Dec 2020 17:11:32 +0100 Subject: [PATCH 1/5] Ensure that the `pdf.sandbox.js` scriptElement is also removed from the DOM (PR 12695 follow-up) I completely missed this previously, but we obviously should remove the scriptElement as well to *really* clean-up everything properly. Given that there's multiple existing usages of `loadScript` in the code-base, the safest/quickest solution seemed to be to have call-sites opt-in to remove the scriptElement using a new parameter. --- src/display/display_utils.js | 10 ++++++++-- web/genericcom.js | 5 ++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/display/display_utils.js b/src/display/display_utils.js index 302dd02440465..0c3fd615850f7 100644 --- a/src/display/display_utils.js +++ b/src/display/display_utils.js @@ -530,14 +530,20 @@ function isValidFetchUrl(url, baseUrl) { /** * @param {string} src + * @param {boolean} [removeScriptElement] * @returns {Promise} */ -function loadScript(src) { +function loadScript(src, removeScriptElement = false) { return new Promise((resolve, reject) => { const script = document.createElement("script"); script.src = src; - script.onload = resolve; + script.onload = function (evt) { + if (removeScriptElement) { + script.remove(); + } + resolve(evt); + }; script.onerror = function () { reject(new Error(`Cannot load script at: ${script.src}`)); }; diff --git a/web/genericcom.js b/web/genericcom.js index 2b83f0dfec107..0f914218d0ac9 100644 --- a/web/genericcom.js +++ b/web/genericcom.js @@ -41,7 +41,10 @@ class GenericPreferences extends BasePreferences { class GenericScripting { constructor() { - this._ready = loadScript(AppOptions.get("sandboxBundleSrc")).then(() => { + this._ready = loadScript( + AppOptions.get("sandboxBundleSrc"), + /* removeScriptElement = */ true + ).then(() => { return window.pdfjsSandbox.QuickJSSandbox(); }); } From 8d72981c18d350f5fd9f86d62f3e11fbd8b94685 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Mon, 7 Dec 2020 17:21:49 +0100 Subject: [PATCH 2/5] Move cancelling of idleCallbacks from `PDFViewerApplication.close` and into its own helper method Since the `close` method has become quite large, this small re-factoring shouldn't hurt. --- web/app.js | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/web/app.js b/web/app.js index ff7aa9fb8d70a..9fa21cd710dde 100644 --- a/web/app.js +++ b/web/app.js @@ -763,6 +763,19 @@ const PDFViewerApplication = { document.title = title; }, + /** + * @private + */ + _cancelIdleCallbacks() { + if (!this._idleCallbacks.size) { + return; + } + for (const callback of this._idleCallbacks) { + window.cancelIdleCallback(callback); + } + this._idleCallbacks.clear(); + }, + /** * Closes opened PDF document. * @returns {Promise} - Returns the promise, which is resolved when all @@ -799,10 +812,8 @@ const PDFViewerApplication = { this._contentLength = null; this.triggerDelayedFallback = null; this._saveInProgress = false; - for (const callback of this._idleCallbacks) { - window.cancelIdleCallback(callback); - } - this._idleCallbacks.clear(); + + this._cancelIdleCallbacks(); if (this._scriptingInstance) { const { scripting, events } = this._scriptingInstance; From 6c807f3f86efdf33d41746d3e578815c7d2401e6 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Mon, 7 Dec 2020 17:54:44 +0100 Subject: [PATCH 3/5] Move destroying of the scripting-instance from `PDFViewerApplication.close` and into its own helper method Since the `close` method has become quite large, this small re-factoring shouldn't hurt (and may also be useful with future changes to the `_initializeJavaScript` method). --- web/app.js | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/web/app.js b/web/app.js index 9fa21cd710dde..4b48e3a5b1f9d 100644 --- a/web/app.js +++ b/web/app.js @@ -776,6 +776,26 @@ const PDFViewerApplication = { this._idleCallbacks.clear(); }, + /** + * @private + */ + async _destroyScriptingInstance() { + if (!this._scriptingInstance) { + return; + } + const { scripting, events } = this._scriptingInstance; + try { + await scripting.destroySandbox(); + } catch (ex) {} + + for (const [name, listener] of events) { + window.removeEventListener(name, listener); + } + events.clear(); + + this._scriptingInstance = null; + }, + /** * Closes opened PDF document. * @returns {Promise} - Returns the promise, which is resolved when all @@ -788,8 +808,9 @@ const PDFViewerApplication = { if (!this.pdfLoadingTask) { return undefined; } + const promises = []; - const promise = this.pdfLoadingTask.destroy(); + promises.push(this.pdfLoadingTask.destroy()); this.pdfLoadingTask = null; if (this.pdfDocument) { @@ -814,18 +835,7 @@ const PDFViewerApplication = { this._saveInProgress = false; this._cancelIdleCallbacks(); - - if (this._scriptingInstance) { - const { scripting, events } = this._scriptingInstance; - try { - scripting.destroySandbox(); - } catch (ex) {} - - for (const [name, listener] of events) { - window.removeEventListener(name, listener); - } - this._scriptingInstance = null; - } + promises.push(this._destroyScriptingInstance()); this.pdfSidebar.reset(); this.pdfOutlineViewer.reset(); @@ -844,7 +854,7 @@ const PDFViewerApplication = { if (typeof PDFBug !== "undefined") { PDFBug.cleanup(); } - return promise; + return Promise.all(promises); }, /** From a7230eb03371e77f4f0fd4da5966b5bba9d43ecb Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Mon, 7 Dec 2020 18:15:24 +0100 Subject: [PATCH 4/5] Move the `GenericScripting` class to its own file, such that it can be used in the Chromium-extension While it's not entirely clear to me that it's ultimately desirable to use the `pdf.sandbox.js` in the Chromium-extension, given that the MOZCENTRAL-build uses `pdf.scripting.js` directly in a *custom* sandbox, the current state isn't that great since setting `enableScripting = true` with the Chromium-extension will currently fail completely. Hence this patch, which should at least unbreak things for now. --- web/app_options.js | 6 ++++++ web/chromecom.js | 5 +++++ web/generic_scripting.js | 45 ++++++++++++++++++++++++++++++++++++++++ web/genericcom.js | 29 +------------------------- 4 files changed, 57 insertions(+), 28 deletions(-) create mode 100644 web/generic_scripting.js diff --git a/web/app_options.js b/web/app_options.js index caf2455caed68..f65b637b605a7 100644 --- a/web/app_options.js +++ b/web/app_options.js @@ -265,6 +265,12 @@ if ( : "../build/pdf.sandbox.js", kind: OptionKind.VIEWER, }; +} else if (PDFJSDev.test("CHROME")) { + defaultOptions.sandboxBundleSrc = { + /** @type {string} */ + value: "../build/pdf.sandbox.js", + kind: OptionKind.VIEWER, + }; } const userOptions = Object.create(null); diff --git a/web/chromecom.js b/web/chromecom.js index df055fb17ed6f..f3dec140bf99e 100644 --- a/web/chromecom.js +++ b/web/chromecom.js @@ -19,6 +19,7 @@ import { AppOptions } from "./app_options.js"; import { BasePreferences } from "./preferences.js"; import { DownloadManager } from "./download_manager.js"; import { GenericL10n } from "./genericl10n.js"; +import { GenericScripting } from "./generic_scripting.js"; if (typeof PDFJSDev === "undefined" || !PDFJSDev.test("CHROME")) { throw new Error( @@ -428,6 +429,10 @@ class ChromeExternalServices extends DefaultExternalServices { static createL10n(options) { return new GenericL10n(navigator.language); } + + static get scripting() { + return new GenericScripting(); + } } PDFViewerApplication.externalServices = ChromeExternalServices; diff --git a/web/generic_scripting.js b/web/generic_scripting.js new file mode 100644 index 0000000000000..4c2f9164ccb24 --- /dev/null +++ b/web/generic_scripting.js @@ -0,0 +1,45 @@ +/* Copyright 2020 Mozilla Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { AppOptions } from "./app_options.js"; +import { loadScript } from "pdfjs-lib"; + +class GenericScripting { + constructor() { + this._ready = loadScript( + AppOptions.get("sandboxBundleSrc"), + /* removeScriptElement = */ true + ).then(() => { + return window.pdfjsSandbox.QuickJSSandbox(); + }); + } + + async createSandbox(data) { + const sandbox = await this._ready; + sandbox.create(data); + } + + async dispatchEventInSandbox(event) { + const sandbox = await this._ready; + sandbox.dispatchEvent(event); + } + + async destroySandbox() { + const sandbox = await this._ready; + sandbox.nukeSandbox(); + } +} + +export { GenericScripting }; diff --git a/web/genericcom.js b/web/genericcom.js index 0f914218d0ac9..abca71efca5ff 100644 --- a/web/genericcom.js +++ b/web/genericcom.js @@ -14,11 +14,10 @@ */ import { DefaultExternalServices, PDFViewerApplication } from "./app.js"; -import { AppOptions } from "./app_options.js"; import { BasePreferences } from "./preferences.js"; import { DownloadManager } from "./download_manager.js"; import { GenericL10n } from "./genericl10n.js"; -import { loadScript } from "pdfjs-lib"; +import { GenericScripting } from "./generic_scripting.js"; if (typeof PDFJSDev !== "undefined" && !PDFJSDev.test("GENERIC")) { throw new Error( @@ -39,32 +38,6 @@ class GenericPreferences extends BasePreferences { } } -class GenericScripting { - constructor() { - this._ready = loadScript( - AppOptions.get("sandboxBundleSrc"), - /* removeScriptElement = */ true - ).then(() => { - return window.pdfjsSandbox.QuickJSSandbox(); - }); - } - - async createSandbox(data) { - const sandbox = await this._ready; - sandbox.create(data); - } - - async dispatchEventInSandbox(event) { - const sandbox = await this._ready; - sandbox.dispatchEvent(event); - } - - async destroySandbox() { - const sandbox = await this._ready; - sandbox.nukeSandbox(); - } -} - class GenericExternalServices extends DefaultExternalServices { static createDownloadManager(options) { return new DownloadManager(); From 6218b9a51255dd2960f579db32cd29ea3b687e35 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Mon, 7 Dec 2020 18:23:34 +0100 Subject: [PATCH 5/5] Re-factor/re-name the `scripting` getter, on the `externalServices`-implementations, to a `createScripting` method Given that the GENERIC default viewer supports opening more than one document, and that a unique scripting-instance is now used for each document, the changes made in this patch seem appropriate. --- web/app.js | 10 +++++----- web/chromecom.js | 2 +- web/firefoxcom.js | 2 +- web/genericcom.js | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/web/app.js b/web/app.js index 4b48e3a5b1f9d..e67d7bc079956 100644 --- a/web/app.js +++ b/web/app.js @@ -168,6 +168,10 @@ class DefaultExternalServices { throw new Error("Not implemented: createL10n"); } + static createScripting() { + throw new Error("Not implemented: createScripting"); + } + static get supportsIntegratedFind() { return shadow(this, "supportsIntegratedFind", false); } @@ -186,10 +190,6 @@ class DefaultExternalServices { static get isInAutomation() { return shadow(this, "isInAutomation", false); } - - static get scripting() { - throw new Error("Not implemented: scripting"); - } } const PDFViewerApplication = { @@ -1456,7 +1456,7 @@ const PDFViewerApplication = { // or the document was closed while the data resolved. return; } - const { scripting } = this.externalServices; + const scripting = this.externalServices.createScripting(); // Store a reference to the current scripting-instance, to allow destruction // of the sandbox and removal of the event listeners at document closing. this._scriptingInstance = { scripting, events: new Map() }; diff --git a/web/chromecom.js b/web/chromecom.js index f3dec140bf99e..6694755e75205 100644 --- a/web/chromecom.js +++ b/web/chromecom.js @@ -430,7 +430,7 @@ class ChromeExternalServices extends DefaultExternalServices { return new GenericL10n(navigator.language); } - static get scripting() { + static createScripting() { return new GenericScripting(); } } diff --git a/web/firefoxcom.js b/web/firefoxcom.js index 26a347bed2ace..3d764e87f8049 100644 --- a/web/firefoxcom.js +++ b/web/firefoxcom.js @@ -360,7 +360,7 @@ class FirefoxExternalServices extends DefaultExternalServices { return new MozL10n(mozL10n); } - static get scripting() { + static createScripting() { return FirefoxScripting; } diff --git a/web/genericcom.js b/web/genericcom.js index abca71efca5ff..2bf923a7bed20 100644 --- a/web/genericcom.js +++ b/web/genericcom.js @@ -51,7 +51,7 @@ class GenericExternalServices extends DefaultExternalServices { return new GenericL10n(locale); } - static get scripting() { + static createScripting() { return new GenericScripting(); } }