Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the detection of Node.js environments on Electron strict. #12085

Merged
merged 1 commit into from
Jul 17, 2020

Conversation

tamuratak
Copy link
Contributor

Make the detection of Node.js environments on Electron strict. The main process and its child processes should be detected as Node.js environments.

Without this PR, pdfjsLib.getDocument(...).promise.then fails with an error message Setting up fake worker failed: "No "GlobalWorkerOptions.workerSrc" specified.". on the main process and its child processes of Electron.

src/shared/is_node.js Outdated Show resolved Hide resolved
const isNodeJS =
typeof process === "object" &&
process + "" === "[object process]" &&
!process.versions.nw &&
!process.versions.electron;
!(process.versions.electron && process.type && process.type !== "browser");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quoting from https://www.electronjs.org/docs/api/process#processtype-readonly:

A String representing the current process's type, can be "browser" (i.e. main process), "renderer", or "worker" (i.e. web worker).

Hence my question: Will process.type !== "browser" actually do the right thing when this code is run on the worker-thread in a browser?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which is what you are saying with the sentence,

when this code is run on the worker-thread in a browser?":

  • Running build/pdf.wroker.js on the worker-thread in a browser.
  • Running build/pdf.js with OffscreenCanvas on the worker-thread in a browser.

In either case, it should work properly. I will check.

Copy link
Collaborator

@Snuffleupagus Snuffleupagus Jul 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running build/pdf.worker.js on the worker-thread in a browser.

The changes you're making in this patch only applies to environments that pretend to be Node.js, when in reality they are not.
Hence my question pertains to using the Electron-framework in a browser, with workers enabled, and ensuring that e.g.

pdf.js/src/core/worker.js

Lines 676 to 684 in 7dabc5e

// Worker thread (and not Node.js)?
if (
typeof window === "undefined" &&
!isNodeJS &&
typeof self !== "undefined" &&
isMessagePort(self)
) {
WorkerMessageHandler.initializeFromPort(self);
}
works as expected.

Running build/pdf.js with OffscreenCanvas on the worker-thread in a browser.

We don't support OffscreenCanvas so that doesn't really matter right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have created a repository to check this PR.

git clone [email protected]:tamuratak/electron-pdfjs-sample.git
cd electron-pdfjs-sample 
npm i  (Install Electron locally)
npx electron app/

Then the PDF.js viewer launches. You can see through the dev tools that WebWoker works well. In both cases that nodeIntegration and nodeIntegrationInWorker are true or false, we can see that it woks well.

All the files at app/src/viewer are copied from pdfjs-2.4.456-dist.zip. I have edited app/src/viewer/pdf.js and app/src/viewer/pdf.worker.js as this PR. I have also edited app/src/viewer/viewer.js and app/src/viewer/viewer.html.

$  diff -u ../tmp/pdfjs-2.4/build/pdf.js app/src/viewer/pdf.js 
--- ../tmp/pdfjs-2.4/build/pdf.js	2020-03-19 22:43:52.000000000 +0900
+++ app/src/viewer/pdf.js	2020-07-12 10:58:53.000000000 +0900
@@ -4166,7 +4166,7 @@
   value: true
 });
 exports.isNodeJS = void 0;
-const isNodeJS = typeof process === "object" && process + "" === "[object process]" && !process.versions["nw"] && !process.versions["electron"];
+const isNodeJS = typeof process === "object" && process + "" === "[object process]" && !process.versions["nw"] && !(process.versions.electron && process.type && process.type !== "browser");
 exports.isNodeJS = isNodeJS;
 
 /***/ }),
$  diff -u ../tmp/pdfjs-2.4/build/pdf.worker.js app/src/viewer/pdf.worker.js 
--- ../tmp/pdfjs-2.4/build/pdf.worker.js	2020-03-19 22:44:08.000000000 +0900
+++ app/src/viewer/pdf.worker.js	2020-07-12 10:58:53.000000000 +0900
@@ -45266,7 +45266,7 @@
   value: true
 });
 exports.isNodeJS = void 0;
-const isNodeJS = typeof process === "object" && process + "" === "[object process]" && !process.versions["nw"] && !process.versions["electron"];
+const isNodeJS = typeof process === "object" && process + "" === "[object process]" && !process.versions["nw"] && !(process.versions.electron && process.type && process.type !== "browser");
 exports.isNodeJS = isNodeJS;
 
 /***/ }),
$  diff -u ../tmp/pdfjs-2.4/web/viewer.js app/src/viewer/viewer.js
--- ../tmp/pdfjs-2.4/web/viewer.js	2020-03-19 22:43:56.000000000 +0900
+++ app/src/viewer/viewer.js	2020-07-12 10:58:53.000000000 +0900
@@ -3629,11 +3629,11 @@
     kind: OptionKind.API
   },
   workerPort: {
-    value: null,
+    value: new Worker("pdf.worker.js"),
     kind: OptionKind.WORKER
   },
   workerSrc: {
-    value: "../build/pdf.worker.js",
+    value: "pdf.worker.js",
     kind: OptionKind.WORKER
   }
 };
$  diff -u ../tmp/pdfjs-2.4/web/viewer.html app/src/viewer/viewer.html 
--- ../tmp/pdfjs-2.4/web/viewer.html	2020-03-19 22:43:44.000000000 +0900
+++ app/src/viewer/viewer.html	2020-07-12 10:58:53.000000000 +0900
@@ -34,7 +34,7 @@
 
 <!-- This snippet is used in production (included from viewer.html) -->
 <link rel="resource" type="application/l10n" href="locale/locale.properties">
-<script src="../build/pdf.js"></script>
+<script src="pdf.js"></script>
 
 
     <script src="viewer.js"></script>

The main process and its child processes should be detected as Node.js environments.
@timvandermeij
Copy link
Contributor

/botio unittest

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_unittest from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/d850858654b4bee/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_unittest from @timvandermeij received. Current queue size: 0

Live output at: http://54.215.176.217:8877/204aa8cc78558df/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/d850858654b4bee/output.txt

Total script time: 3.75 mins

  • Unit Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://54.215.176.217:8877/204aa8cc78558df/output.txt

Total script time: 4.66 mins

  • Unit Tests: Passed

@timvandermeij timvandermeij merged commit a604973 into mozilla:master Jul 17, 2020
@timvandermeij
Copy link
Contributor

Thank you for your contribution!

@tamuratak tamuratak deleted the fix_isnodejs branch July 30, 2020 13:13
tamuratak added a commit to tamuratak/LaTeX-Workshop that referenced this pull request Feb 21, 2021
tamuratak added a commit to tamuratak/LaTeX-Workshop that referenced this pull request Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants