1811457 - Transfer, rather than copy, PDF-data to the viewer

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, enhancement)

Description

[Jonas Jenwald [:Snuffleupagus]]

In order to reduce overall memory usage, we should transfer PDF-data from the platform code to the PDF Viewer.

Comment 1

[Jonas Jenwald [:Snuffleupagus]]

Attachment: Bug 1811457 - Transfer, rather than copy, PDF-data to the viewer. r=#pdfjs-reviewers

In order to reduce overall memory usage, we should transfer PDF-data from the platform code to the PDF Viewer.

Comment 2

[Calixte Denizet (:calixte)]

:Snuffleupagus, are you able to run mach try ?

Comment 3

[Calixte Denizet (:calixte)]

If not you can ask for Level 1 access: https://firefox-source-docs.mozilla.org/tools/try/configuration.html#getting-level-1-commit-access

Comment 4

[Jonas Jenwald [:Snuffleupagus]]

(In reply to Calixte Denizet (:calixte) from comment #2)

:Snuffleupagus, are you able to run mach try ?

Unfortunately not, since I don't have access.

(In reply to Calixte Denizet (:calixte) from comment #3)

If not you can ask for Level 1 access: https://firefox-source-docs.mozilla.org/tools/try/configuration.html#getting-level-1-commit-access

Given that I'm submitting so few mozilla-central patches, I'm not sure if it's really worth it. (Every time that I do, I usually need to refer back to the documentation for how to do things...)

Comment 5

[Calixte Denizet (:calixte)]

A try:
https://treeherder.mozilla.org/jobs?repo=try&revision=eb4d887c957772d8aa27d00c305261962ba5256b

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20bb31347ce4
Transfer, rather than copy, PDF-data to the viewer. r=pdfjs-reviewers,robwu

Comment 7

[Cristian Tuns]

Backed out for causing mochitest failures

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | dom/base/test/test_pdf_print.html | Test timed out. -

another fail: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=1155668&selectedTaskRun=ejpaFBkxQTW6iSwb5ZOpHg.0&revision=20bb31347ce4541f0484f3915ef3093e091c4756&searchStr=linux%2C18.04%2Cx64%2Cwebrender%2Casan%2Copt%2Cmochitests%2Cwith%2Csoftware%2Cwebrender%2Cenabled%2Ctest-linux1804-64-asan-qr%2Fopt-mochitest-browser-chrome-swr%2Cbc16

Comment 8

[Jonas Jenwald [:Snuffleupagus]]

TL;DR It seems that using null as the default value for the transfers parameter, in the postMessage-calls, doesn't work in some(?) of the test-suites; however using undefined instead seems to work.

---

The error comes from https://treeherder.mozilla.org/logviewer?job_id=403057239&repo=autoland&lineNumber=14240 and happens in the https://searchfox.org/mozilla-central/source/dom/base/test/test_pdf_print.html test-case.
As can be seen there that test-case opens nonsensical "PDF documents", and in particular this line causes the error.

Strangely enough directly loading data:application/pdf, in the browser, with the original patch applied, works fine so it seems to be something "special" about the test-suite in use.
Locally this test-case does work with the updated patch, although I don't at all understand why it fixes things; however it's probably the best I can do here.

Comment 9

[Calixte Denizet (:calixte)]

The error comes from WindowBinding.cpp which means that null is not a valid argument hence your fix is the correct one.

Comment 10

[Calixte Denizet (:calixte)]

A new try:
https://treeherder.mozilla.org/jobs?repo=try&revision=9bd4d9ddb9459119291305899c0c6c5ca7b43baf

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/52f9e6382dce
Transfer, rather than copy, PDF-data to the viewer. r=pdfjs-reviewers,robwu,calixte

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/52f9e6382dce