[api-minor] Add the possibility to collect Javascript actions #12429

calixteman · 2020-09-30T19:00:35Z

No description provided.

Snuffleupagus

Based on a very cursory look, I've added a few quick comments.

src/shared/util.js

src/core/annotation.js

src/core/document.js

src/core/annotation.js

src/display/api.js

src/core/annotation.js

timvandermeij · 2020-09-30T19:57:34Z

I'm missing some context for this patch. Which issue does this solve? Why are we interested in collecting this information, and what is it used for?

calixteman · 2020-09-30T20:03:28Z

I'm missing some context for this patch. Which issue does this solve? Why are we interested in collecting this information, and what is it used for?

I'll fix the nits tomorrow: thanks for that.
We plan to add support to exectute js stuff embedded in the PDF.
A first step has been made here: https://phabricator.services.mozilla.com/D91746 for the m-c version.
And we plan to add something else (probably quickjs in using emscripten) for the extension version.
Anyway if there is an open issue for js stuff, then please assign me.

src/core/worker.js

test/unit/annotation_spec.js

src/core/annotation.js

Snuffleupagus

Sorry, but the API implemented here is really not correct!

Similar to the regular getAnnotations method, which lives on the page rather than the document, this new API-method must also be on the page-level.

Please keep in mind that the PDF.js library is written such that data can be loaded when needed, using e.g. range requests and streaming, to explicitly not require that the entire document is loaded in order for rendering to start (see e.g. the disableAutoFetch option).

Hence we really can't/shouldn't add an API-method that essentially forces the entire document to load and all pages to be parsed, which would be particularly bad when invoked from the viewer. (It would force a lot of data to be requested very early on, possibly affecting general viewer performance negatively when loading longer documents.)

calixteman · 2020-10-05T08:34:26Z

I thought about that of course and the problem is when a script is getting a value from a Field which hasn't been rendered: we can't await for its value.
Setting a value shouldn't be a problem: we can put it in annotationStorage if the Field hasn't been rendered and use this value when rendered for this first time.

What we can do:

only get all the annotations and then run the sandbox when at least one contains some JS
make the hypothesis that no script is trying to get a value from a non-rendered field (I guess it should be pretty rare but tbh I don't know) and then use a by-page approach.
or another good idea I didn't think about: any suggestion will be appreciated.

calixteman · 2020-10-05T17:13:16Z

I moved the data collection stuff in PDFDocument in using Fields entry from Acroform dictionary.
So this way, we don't parse all the pages.
@Snuffleupagus does it sound better ?

src/core/annotation.js

Snuffleupagus · 2020-10-07T10:48:58Z

I moved the data collection stuff in PDFDocument in using Fields entry from Acroform dictionary.
So this way, we don't parse all the pages.
@Snuffleupagus does it sound better ?

I suppose I still don't really understand why this, similar to annotations in general, cannot be parsed on a page-by-page basis!?
Are you perhaps expecting that JavaScript actions on one page will affect fields on other pages?

(Generally speaking, given the lack of context/information in the commit message it's quite difficult to get a good overview of the entire situation and e.g. understand why you went with a particular approach.)

calixteman · 2020-10-07T10:59:54Z

I moved the data collection stuff in PDFDocument in using Fields entry from Acroform dictionary.
So this way, we don't parse all the pages.
@Snuffleupagus does it sound better ?

I suppose I still don't really understand why this, similar to annotations in general, cannot be parsed on a page-by-page basis!?
Are you perhaps expecting that JavaScript actions on one page will affect fields on other pages?

Yes exactly

(Generally speaking, given the lack of context/information in the commit message it's quite difficult to get a good overview of the entire situation and e.g. understand why you went with a particular approach.)

My bad, you're right, sorry about that.
Anyway I gave more explanations in the different comments I made (e.g. #12429 (comment)).

calixteman · 2020-10-12T07:41:14Z

Can we move forward on this patch please ?

timvandermeij

Now that it's finally clear to me why the extra method is needed, I think this approach can be used now with these comments addressed, in particular to add some documentation on why they are there (see the comment below).

src/core/annotation.js

src/core/document.js

src/shared/compatibility.js

src/shared/util.js

test/unit/annotation_spec.js

src/core/worker.js

src/core/document.js

timvandermeij · 2020-10-13T19:13:57Z

/botio unittest

pdfjsbot · 2020-10-13T19:13:59Z

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/73449564b1e981f/output.txt

pdfjsbot · 2020-10-13T19:13:59Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/04c5dc96f5a5eb5/output.txt

pdfjsbot · 2020-10-13T19:18:16Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/73449564b1e981f/output.txt

Total script time: 4.26 mins

Unit Tests: FAILED

pdfjsbot · 2020-10-13T19:22:11Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/04c5dc96f5a5eb5/output.txt

Total script time: 5.22 mins

Unit Tests: Passed

timvandermeij

Approved pending these final comments now that I did a full review, and passing tests afterwards (ignore the intermittent one above). Thanks!

src/core/annotation.js

src/core/document.js

src/shared/util.js

test/unit/annotation_spec.js

Snuffleupagus

Seems fine to me too, with just one more question; please see #12429 (comment)

timvandermeij · 2020-10-14T20:29:39Z

/botio test

pdfjsbot · 2020-10-14T20:29:41Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/c25b840b1a5ae14/output.txt

pdfjsbot · 2020-10-14T20:29:41Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2020-10-14T20:55:30Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 25.80 mins

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

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

pdfjsbot · 2020-10-14T21:01:04Z

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/c25b840b1a5ae14/output.txt

Total script time: 31.36 mins

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

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

timvandermeij · 2020-10-14T21:27:57Z

Nice work!

calixteman force-pushed the collect_js branch from fcbe180 to b247073 Compare September 30, 2020 19:07

Snuffleupagus requested changes Sep 30, 2020

View reviewed changes

timvandermeij added the annotations label Sep 30, 2020

calixteman force-pushed the collect_js branch 2 times, most recently from f2286d8 to 6045c63 Compare October 1, 2020 08:22

Snuffleupagus reviewed Oct 1, 2020

View reviewed changes

src/core/worker.js Outdated Show resolved Hide resolved

calixteman force-pushed the collect_js branch from 6045c63 to 82866fc Compare October 1, 2020 09:06

Snuffleupagus reviewed Oct 1, 2020

View reviewed changes

test/unit/annotation_spec.js Outdated Show resolved Hide resolved

calixteman force-pushed the collect_js branch from 82866fc to bdd3e01 Compare October 1, 2020 12:58

Snuffleupagus reviewed Oct 1, 2020

View reviewed changes

src/core/annotation.js Outdated Show resolved Hide resolved

calixteman force-pushed the collect_js branch from bdd3e01 to 53cbe33 Compare October 1, 2020 14:00

Snuffleupagus requested changes Oct 3, 2020

View reviewed changes

calixteman force-pushed the collect_js branch from 53cbe33 to 77a0da4 Compare October 5, 2020 17:08

calixteman force-pushed the collect_js branch from 77a0da4 to 94690de Compare October 5, 2020 17:19

timvandermeij reviewed Oct 5, 2020

View reviewed changes

src/core/annotation.js Outdated Show resolved Hide resolved

calixteman force-pushed the collect_js branch from 94690de to dde92a5 Compare October 9, 2020 13:47

timvandermeij reviewed Oct 12, 2020

View reviewed changes

calixteman force-pushed the collect_js branch 2 times, most recently from af0f193 to cf451a1 Compare October 13, 2020 09:54

calixteman mentioned this pull request Oct 13, 2020

Some urls behind pushbutton are not extracted from the AA dictionary #12475

Closed

Snuffleupagus reviewed Oct 13, 2020

View reviewed changes

src/core/worker.js Outdated Show resolved Hide resolved

src/core/document.js Outdated Show resolved Hide resolved

src/core/document.js Outdated Show resolved Hide resolved

src/core/document.js Outdated Show resolved Hide resolved

calixteman force-pushed the collect_js branch 2 times, most recently from dea74f0 to 932f441 Compare October 13, 2020 13:59

timvandermeij approved these changes Oct 13, 2020

View reviewed changes

Add the possibility to collect Javascript actions

71ecc31

calixteman force-pushed the collect_js branch from 932f441 to 71ecc31 Compare October 14, 2020 08:44

Snuffleupagus approved these changes Oct 14, 2020

View reviewed changes

timvandermeij changed the title ~~Add the possibility to collect Javascript actions~~ [api-minor] Add the possibility to collect Javascript actions Oct 14, 2020

timvandermeij approved these changes Oct 14, 2020

View reviewed changes

timvandermeij merged commit a373137 into mozilla:master Oct 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api-minor] Add the possibility to collect Javascript actions #12429

[api-minor] Add the possibility to collect Javascript actions #12429

calixteman commented Sep 30, 2020

Snuffleupagus left a comment

timvandermeij commented Sep 30, 2020

calixteman commented Sep 30, 2020

Snuffleupagus left a comment

calixteman commented Oct 5, 2020

calixteman commented Oct 5, 2020

Snuffleupagus commented Oct 7, 2020

calixteman commented Oct 7, 2020

calixteman commented Oct 12, 2020

timvandermeij left a comment

timvandermeij commented Oct 13, 2020

pdfjsbot commented Oct 13, 2020

pdfjsbot commented Oct 13, 2020

pdfjsbot commented Oct 13, 2020

pdfjsbot commented Oct 13, 2020

timvandermeij left a comment •

edited

Loading

Snuffleupagus left a comment

timvandermeij commented Oct 14, 2020

pdfjsbot commented Oct 14, 2020

pdfjsbot commented Oct 14, 2020

pdfjsbot commented Oct 14, 2020

pdfjsbot commented Oct 14, 2020

timvandermeij commented Oct 14, 2020

[api-minor] Add the possibility to collect Javascript actions #12429

[api-minor] Add the possibility to collect Javascript actions #12429

Conversation

calixteman commented Sep 30, 2020

Snuffleupagus left a comment

Choose a reason for hiding this comment

timvandermeij commented Sep 30, 2020

calixteman commented Sep 30, 2020

Snuffleupagus left a comment

Choose a reason for hiding this comment

calixteman commented Oct 5, 2020

calixteman commented Oct 5, 2020

Snuffleupagus commented Oct 7, 2020

calixteman commented Oct 7, 2020

calixteman commented Oct 12, 2020

timvandermeij left a comment

Choose a reason for hiding this comment

timvandermeij commented Oct 13, 2020

pdfjsbot commented Oct 13, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Oct 13, 2020

From: Bot.io (Windows)

Received

pdfjsbot commented Oct 13, 2020

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Oct 13, 2020

From: Bot.io (Windows)

Success

timvandermeij left a comment • edited Loading

Choose a reason for hiding this comment

Snuffleupagus left a comment

Choose a reason for hiding this comment

timvandermeij commented Oct 14, 2020

pdfjsbot commented Oct 14, 2020

From: Bot.io (Windows)

Received

pdfjsbot commented Oct 14, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Oct 14, 2020

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Oct 14, 2020

From: Bot.io (Windows)

Failed

timvandermeij commented Oct 14, 2020

timvandermeij left a comment •

edited

Loading