JS -- Add a sandbox based on quickjs #12604

calixteman · 2020-11-09T13:49:47Z

quickjs-eval.js has been generated using /~https://github.com/calixteman/pdf.js.quickjs/
lazy load of sandbox code

calixteman · 2020-11-09T13:53:25Z

We need to decide where /~https://github.com/calixteman/pdf.js.quickjs/ should live.

Snuffleupagus

Besides a couple of comments based on a quick look, I've got a few more high-level questions:

How's the quickjs-project licensed? Is it compatible with "Apache License 2.0", as that's what the PDF.js library uses?
How will the quickjs-files, once added to the PDF.js library, be updated?
The reason for asking, is that we used to have loads of dependencies in the external/ folder, which made updating and maintaining them very difficult and time-consuming (why we switched to npm for most dependencies).
Would it be at all possible to pull in the quickjs-files as a dependency, rather than simply dumping them into the PDF.js repository as-is?

gulpfile.js

Snuffleupagus · 2020-11-09T14:49:16Z

gulpfile.js

+    return `\\${match}`;
+  });
+  return gulp
+    .src("./src/scripting_api/quickjs-sandbox.js")


Please follow the same pattern as for all of the other bundles, see above, by adding a new /src/pdf.sandbox.js file instead of this one.

gulpfile.js

Snuffleupagus · 2020-11-09T14:56:17Z

gulpfile.js

+  const defines = builder.merge(DEFINES, { GENERIC: true });
+  const scriptingDefines = builder.merge(defines, { NO_SOURCE_MAP: true });
+  return createScriptingBundle(scriptingDefines)
+    .pipe(stripComments())


Doesn't the fact that the files are run through Webpack already remove comments?

I thought the same but it seems that it isn't the case so I added that to do it.

src/scripting_api/quickjs-sandbox.js

web/app.js

Snuffleupagus · 2020-11-09T15:08:12Z

gulpfile.js

@@ -493,6 +521,20 @@ function makeRef(done, bot) {
  });
 }

+gulp.task("sandbox", function (done) {


I'd suggest separating the createScriptingBundle from this gulp-task, and possible even remove this completely.

For the mozcentral task you added the createScriptingBundle to list existing list of tasks in

pdf.js/gulpfile.js

Lines 1065 to 1067 in 55f55f5

createScriptingBundle(defines).pipe(

gulp.dest(MOZCENTRAL_CONTENT_DIR + "build")

),

hence I suggest adding createScriptingBundle and createSandboxBundle to buildGeneric instead:

pdf.js/gulpfile.js

Lines 723 to 729 in 55f55f5

function buildGeneric(defines, dir) {

rimraf.sync(dir);

return merge([

createMainBundle(defines).pipe(gulp.dest(dir + "build")),

createWorkerBundle(defines).pipe(gulp.dest(dir + "build")),

createWebBundle(defines).pipe(gulp.dest(dir + "web")),

It's on purpose.
The idea is to get a bundle from initialization.js and the convert the bundle content into a string and inject this string in the sandbox bundle. This string will be eval-ed in the sandbox in order to create all what we need for the JS api.
But of course, if you see a better way to do, please tell me.

.eslintignore

calixteman · 2020-11-09T15:57:48Z

Besides a couple of comments based on a quick look, I've got a few more high-level questions:
1. How's the quickjs-project licensed? Is it compatible with "Apache License 2.0", as that's what the PDF.js library uses?

According to https://raw.githubusercontent.com/bellard/quickjs/master/quickjs.c it's a MIT licence so it's ok with Apache License 2.0.

2. How will the quickjs-files, once added to the PDF.js library, be updated?
   The reason for asking, is that we used to have loads of dependencies in the `external/` folder, which made updating and maintaining them very difficult and time-consuming (why we switched to `npm` for most dependencies).

The easiest solution is the best here... so tell me what you prefer having.
I put the build stuff in a Docker image to avoid to have to set emsdk to build it.
I guess we can change the script build.py into js one using node.
For now it's quite easy to run: python3 build.py -c -o ../pdf.js.calixteman/src/scripting_api.
And to update quickjs, just need to change /~https://github.com/calixteman/pdf.js.quickjs/blob/main/Dockerfile#L5 and then python3 build.py -C -c -o ../pdf.js.calixteman/src/scripting_api (-C is to update the docker image).

3. Would it be at all possible to pull in the quickjs-files as a dependency, rather than simply dumping them into the PDF.js repository as-is?

I don't know enough to answer here.

The way it's built now is:

create a bundle for scripting stuff (initialization.js)
remove comments to decrease the size
stringify that stuff (it explains why I replace the \n,\t,*)
create a bundle for the sandbox (quickjs-sandbox.js)
insert the string we created above in the code (since this code will be loaded (eval-ed) in the sandbox)
and at the end we've a file pdf.sandbox.js which contains everything (sandbox code+initialization code)

If you think about a better way to do, feel free to tell it or amend the patch or whatever.

calixteman · 2020-11-10T17:38:28Z

I moved my quickjs stuff under mozilla's umbrella:
/~https://github.com/mozilla/pdf.js.quickjs/

calixteman · 2020-11-17T13:15:37Z

/botio test

pdfjsbot · 2020-11-17T13:15:38Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @calixteman received. Current queue size: 0

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

pdfjsbot · 2020-11-17T13:15:39Z

From: Bot.io (Windows)

Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://3.101.106.178:8877/09badca9d4b10e9/output.txt

pdfjsbot · 2020-11-17T13:41:43Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 26.06 mins

Font tests: Passed
Unit tests: FAILED
Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/f7e194b0841c93a/reftest-analyzer.html#web=eq.log

pdfjsbot · 2020-11-17T13:44:46Z

From: Bot.io (Windows)

Failed

Full output at http://3.101.106.178:8877/09badca9d4b10e9/output.txt

Total script time: 29.12 mins

Font tests: Passed
Unit tests: FAILED
Regression tests: FAILED

Image differences available at: http://3.101.106.178:8877/09badca9d4b10e9/reftest-analyzer.html#web=eq.log

calixteman · 2020-11-17T14:46:58Z

/botio test

pdfjsbot · 2020-11-17T14:47:00Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.67.70.0:8877/15e258dbf1c01a9/output.txt

pdfjsbot · 2020-11-17T14:47:00Z

From: Bot.io (Windows)

Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://3.101.106.178:8877/6be150a1b24be93/output.txt

pdfjsbot · 2020-11-17T15:12:57Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/15e258dbf1c01a9/output.txt

Total script time: 25.94 mins

Font tests: Passed
Unit tests: FAILED
Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/15e258dbf1c01a9/reftest-analyzer.html#web=eq.log

pdfjsbot · 2020-11-17T15:17:18Z

From: Bot.io (Windows)

Failed

Full output at http://3.101.106.178:8877/6be150a1b24be93/output.txt

Total script time: 30.30 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

Image differences available at: http://3.101.106.178:8877/6be150a1b24be93/reftest-analyzer.html#web=eq.log

brendandahl

r+ depending on comment

brendandahl · 2020-11-18T22:45:04Z

web/genericcom.js

+    const promise = loadScript(AppOptions.get("scriptingSrc")).then(() => {
+      return window.pdfjsSandbox.QuickJSSandbox();
+    });
+    const sandbox = {


Is there a benefit to having all of these methods using promise.then vs have scripting return a promise and then have all the methods be direct calls? e.g.

get scripting() { return loadScript(...).then((sbx) => { return { createSandbox(data) { sbx.create(data); }, ...

It's for consistency with:

pdf.js/web/firefoxcom.js

Lines 257 to 269 in d3936ac

class FirefoxScripting {

static createSandbox(data) {

FirefoxCom.requestSync("createSandbox", data);

}

static dispatchEventInSandbox(event, sandboxID) {

FirefoxCom.requestSync("dispatchEventInSandbox", event);

}

static destroySandbox() {

FirefoxCom.requestSync("destroySandbox", null);

}

}

* quickjs-eval.js has been generated using /~https://github.com/mozilla/pdf.js.quickjs/ * lazy load of sandbox code * Rewrite tests to use the sandbox * Add a task `watch-sandbox` which update bundle pdf.sandbox.js on change in the sandbox code

Snuffleupagus · 2020-11-20T19:14:00Z

At the very least, I would have expected e.g. a README file in /~https://github.com/mozilla/pdf.js/tree/master/external/quickjs mentioning things like:

What LICENSE applies to the code.
From where the code was obtained (original repository).
The exact build steps, and any necessary setup info, required to generate the code that's in the quickjs-eval.js file.
Without that, it can become very difficult for anyone to update that in e.g. six months or a years time (having to manually go through old PRs to find the information doesn't seem great).

Also,

pdf.js/web/app_options.js

Lines 226 to 233 in c88e805

    
           scriptingSrc: { 
        
             /** @type {string} */ 
        
             value: 
        
               typeof PDFJSDev === "undefined" || !PDFJSDev.test("PRODUCTION") 
        
                 ? "../build/generic/build/pdf.sandbox.js" 
        
                 : "../build/pdf.sandbox.js", 
        
             kind: OptionKind.VIEWER, 
        
           },

is really unfortunate since it breaks the fact that the development viewer should be usable without manually building anything.
Furthermore, that block was placed among the API-options despite being a viewer-option!

Snuffleupagus reviewed Nov 9, 2020

View reviewed changes

timvandermeij added the core label Nov 9, 2020

calixteman force-pushed the quickjs branch 2 times, most recently from 9bbd5fc to 37daefe Compare November 10, 2020 17:33

calixteman force-pushed the quickjs branch 6 times, most recently from dd18825 to eccdfce Compare November 17, 2020 13:06

calixteman force-pushed the quickjs branch from eccdfce to 7489747 Compare November 17, 2020 14:41

calixteman requested a review from brendandahl November 17, 2020 16:36

calixteman force-pushed the quickjs branch 2 times, most recently from 7ef72aa to 0d23b10 Compare November 18, 2020 18:18

brendandahl approved these changes Nov 18, 2020

View reviewed changes

JS -- Add a sandbox based on quickjs

c7974e9

* quickjs-eval.js has been generated using /~https://github.com/mozilla/pdf.js.quickjs/ * lazy load of sandbox code * Rewrite tests to use the sandbox * Add a task `watch-sandbox` which update bundle pdf.sandbox.js on change in the sandbox code

calixteman force-pushed the quickjs branch from 0d23b10 to c7974e9 Compare November 19, 2020 12:41

brendandahl merged commit c88e805 into mozilla:master Nov 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JS -- Add a sandbox based on quickjs #12604

JS -- Add a sandbox based on quickjs #12604

calixteman commented Nov 9, 2020

calixteman commented Nov 9, 2020

Snuffleupagus left a comment

Snuffleupagus Nov 9, 2020

Snuffleupagus Nov 9, 2020

calixteman Nov 10, 2020 •

edited

Loading

Snuffleupagus Nov 9, 2020

calixteman Nov 10, 2020

calixteman commented Nov 9, 2020

calixteman commented Nov 10, 2020

calixteman commented Nov 17, 2020

pdfjsbot commented Nov 17, 2020

pdfjsbot commented Nov 17, 2020

pdfjsbot commented Nov 17, 2020

pdfjsbot commented Nov 17, 2020

calixteman commented Nov 17, 2020

pdfjsbot commented Nov 17, 2020

pdfjsbot commented Nov 17, 2020

pdfjsbot commented Nov 17, 2020

pdfjsbot commented Nov 17, 2020

brendandahl left a comment

brendandahl Nov 18, 2020

calixteman Nov 19, 2020

Snuffleupagus commented Nov 20, 2020

	createScriptingBundle(defines).pipe(
	gulp.dest(MOZCENTRAL_CONTENT_DIR + "build")
	),

	function buildGeneric(defines, dir) {
	rimraf.sync(dir);

	return merge([
	createMainBundle(defines).pipe(gulp.dest(dir + "build")),
	createWorkerBundle(defines).pipe(gulp.dest(dir + "build")),
	createWebBundle(defines).pipe(gulp.dest(dir + "web")),

	class FirefoxScripting {
	static createSandbox(data) {
	FirefoxCom.requestSync("createSandbox", data);
	}

	static dispatchEventInSandbox(event, sandboxID) {
	FirefoxCom.requestSync("dispatchEventInSandbox", event);
	}

	static destroySandbox() {
	FirefoxCom.requestSync("destroySandbox", null);
	}
	}

JS -- Add a sandbox based on quickjs #12604

JS -- Add a sandbox based on quickjs #12604

Conversation

calixteman commented Nov 9, 2020

calixteman commented Nov 9, 2020

Snuffleupagus left a comment

Choose a reason for hiding this comment

Snuffleupagus Nov 9, 2020

Choose a reason for hiding this comment

Snuffleupagus Nov 9, 2020

Choose a reason for hiding this comment

calixteman Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Nov 9, 2020

Choose a reason for hiding this comment

calixteman Nov 10, 2020

Choose a reason for hiding this comment

calixteman commented Nov 9, 2020

calixteman commented Nov 10, 2020

calixteman commented Nov 17, 2020

pdfjsbot commented Nov 17, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Nov 17, 2020

From: Bot.io (Windows)

Received

pdfjsbot commented Nov 17, 2020

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Nov 17, 2020

From: Bot.io (Windows)

Failed

calixteman commented Nov 17, 2020

pdfjsbot commented Nov 17, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Nov 17, 2020

From: Bot.io (Windows)

Received

pdfjsbot commented Nov 17, 2020

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Nov 17, 2020

From: Bot.io (Windows)

Failed

brendandahl left a comment

Choose a reason for hiding this comment

brendandahl Nov 18, 2020

Choose a reason for hiding this comment

calixteman Nov 19, 2020

Choose a reason for hiding this comment

Snuffleupagus commented Nov 20, 2020

calixteman Nov 10, 2020 •

edited

Loading