1618553 - pdf viewer should not fire the load event until it's ready for printing

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Apparently this is something people commonly do: create an iframe pointing to a pdf, and in its load handler try to print. See bug 911444 comment 87 and some of the preceding comments.

The problem is that we currently fire the load event before pageViewsReady becomes true, because Gecko has no idea that's going on behind the scenes and thinks the pageload is done. Can we delay it until it's true? I can expose platform APIs here if needed, but someone who actually knows the PDF viewer should handle that side.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I put a testcase at http://web.mit.edu/bzbarsky/www/testcases/pdf-printing/print-from-onload.html

Comment 2

[dotnetCarpenter]

Hrm. As I said in https://bugzilla.mozilla.org/show_bug.cgi?id=911444#c70 printing from a hidden iframe has been the defacto way of printing a PDF since I became a webdeveloper around 2001. So yes people commonly do this. It was not until PDF.js got embedded in Firefox that this approach had any issue. It still works in all other browsers and it used to work in Firefox.

I will follow this issue.

Comment 3

[Brendan Dahl [:bdahl]]

One of our contributors has proposed a solution that would trigger a method notifyPagesLoaded in PdfStreamConverter.jsm. From that method we could then continue printing. Alternatively, we could dispatch an event before beforeprint that is cancellable (or make beforeprint cancellable for pdf.js), then pdf.js would handle retriggering window.print().

bz,
How would you like to handle this on the platform side?

Comment 4

[Boris Zbarsky [:bzbarsky]]

For this specific bug, I think the best thing would be to not fire the load event until we're ready to print, because it's possible that the parent page will want to show some sort of UI at that point too.

On the platform side that probably means adding a way to block the load event firing and then unblock it. As in, ChromeOnly API for Document::BlockOnload() and Document::UnblockOnload. Or the PDF code could just add a dummy request to the loadgroup itself, I guess, but exposing the Document APIs seems safer/saner.

Would that be workable on the pdf.js end?

We could try to make beforeprint cancelale or add another pdf-js-specific event, but I'd rather have less platform-side special-casing here....

Comment 5

[Brendan Dahl [:bdahl]]

Blocking the load event seems reasonable on the pdf.js side. I'm bit worried that could mess with how the viewer is loaded, but I don't think we really depend on the load event for anything (unless other things are indirectly tied to it).

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:bdahl, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1618553 - Allow pdf.js to block the load event until pages are loaded. r=bdahl

I need a test for this, and I think the viewer.html changes really
need to be upstreamed to web/viewer-snippet-firefox-extension.html,
right?

Not sure if there's a more elegant way to do this, so advice welcome,
but this is simple and should do the trick.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

I sent a patch which should do the trick. I need to add a test, and I have some questions about what the pdf.js story is.

I saw https://github.com/mozilla/pdf.js/pull/11647 while at it. Not sure what the best integration story is for this, maybe that route is a bit better?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ad92a745085
Allow pdf.js to block the load event until pages are loaded. r=bdahl,smaug

Comment 10

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on browser_download_open_with_internal_handler.js

backout: https://hg.mozilla.org/integration/autoland/rev/9929a911a9942318e7c0ce4147816d673d08ae65

push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=eMs8EJ8WTkaDegOSQi26QQ.0&revision=8ad92a74508503ef6105973f7ec8630ebbb1612b&searchStr=browser-chrome

failure log: https://treeherder.mozilla.org/logviewer?job_id=329150224&repo=autoland&lineNumber=20182

[task 2021-02-07T12:51:38.343Z] 12:51:38 INFO - TEST-PASS | uriloader/exthandler/tests/mochitest/browser_download_open_with_internal_handler.js | pdf.js should be opened in an adjacent tab -
[task 2021-02-07T12:51:38.343Z] 12:51:38 INFO - Buffered messages finished
[task 2021-02-07T12:51:38.344Z] 12:51:38 INFO - TEST-UNEXPECTED-FAIL | uriloader/exthandler/tests/mochitest/browser_download_open_with_internal_handler.js | Uncaught exception - undefined - timed out after 50 tries.
[task 2021-02-07T12:51:38.345Z] 12:51:38 INFO - Leaving test bound test_check_open_with_internal_handler
[task 2021-02-07T12:51:38.345Z] 12:51:38 INFO - Entering test bound test_check_open_with_external_application
[task 2021-02-07T12:51:38.347Z] 12:51:38 INFO - Testing with file_pdf_application_pdf.pdf

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Looks like a pdf.js issue where it doesn't unblock the load event. I can repro trivially. Jonas, can you take a look? If you don't have the time let me know and I can investigate.

Comment 12

[Jonas Jenwald [:Snuffleupagus]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #11)

Looks like a pdf.js issue where it doesn't unblock the load event. I can repro trivially.

That would suggest that all of the pages never load, which seems really weird to me.

If you don't have the time let me know and I can investigate.

I'm guessing that testing this locally would require doing a "full build", rather than just an "artifact build", right?
If so, then I'm sorry to say that I don't have time to test this unfortunately (being that I'm not employed by Mozilla).

---

Backed out for failures on browser_download_open_with_internal_handler.js

Looking briefly at that test it seems that it waits for internal PDF.js event "documentloaded", and my only immediate idea is that it's possibly an issue with "documentloaded" now firing too soon.

Can you please try changing line https://searchfox.org/mozilla-central/rev/18961a418593da05c39a90a7eff512a3fe25efea/toolkit/components/pdfjs/content/web/viewer.js#1464 to pagesPromise.then(() => { instead, and see if that helps?

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jonas Jenwald [:Snuffleupagus] from comment #12)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #11)

Looks like a pdf.js issue where it doesn't unblock the load event. I can repro trivially.

That would suggest that all of the pages never load, which seems really weird to me.

If you don't have the time let me know and I can investigate.

I'm guessing that testing this locally would require doing a "full build", rather than just an "artifact build", right?
If so, then I'm sorry to say that I don't have time to test this unfortunately (being that I'm not employed by Mozilla).

Yeah, that makes sense, no worries :)

It doesn't quite need a full build, after looking a bit at it. That waiting function seems unused.

I can repro this just loading an empty (zero-size) file. So literally touch foo.pdf, then load it, the pages loaded promise won't resolve, nor reject. I'll poke more but if you have any ideas that'd be awesome of course :)

Comment 14

[Emilio Cobos Álvarez (:emilio)]

I think https://github.com/mozilla/pdf.js/pull/12969 is probably the right fix.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7fe4363fd980
Allow pdf.js to block the load event until pages are loaded. r=bdahl,smaug

Comment 16

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/7fe4363fd980