830278 - printing selections in pdf.js don't work and can cause OOM errors in large PDFs

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Cameron McCormack (:heycam)]

+++ This bug was initially created as a clone of Bug #830214 +++

[Bug 830214 explains how we creates surfaces and dispatch tasks to paint all mozPaintCallback canvases in a document at once.]

Can we avoid creating the surfaces and dispatching these tasks all at once, and instead do a page at a time, maybe when we come back in to PrePrintNextPage?

Comment 1

[Lukas Blakk [:lsblakk] use ?needinfo]

This bug isn't a regression or something we would block a release on, seems speculative, so not marking for tracking.

Comment 2

[Cameron McCormack (:heycam)]

The PDF from bug 830214 has 638 pages.  The canvases are 1190x1684, so with 4 bytes per pixel that would take 4.4 GB of memory.  I think my suggestion in comment 0 wouldn't avoid this, either, since the canvases all need to remain in the document with their image data when printing happens.

Brendan, I think we need to handle printing in pdf.js a little differently for large documents like this.  Is it possible for the print engine to have each page's canvas callback invoked just when it needs to print that page?  Or is that not allowed since script shouldn't be able to run in the middle of printing?  Could we allow script to run between each page, and check that the document is still in a good state for printing each time it runs?

Comment 3

[Alex Keybl [:akeybl]]

We'll revisit in triage due to https://bugzilla.mozilla.org/show_bug.cgi?id=830214#c14

(In reply to Cameron McCormack (:heycam))
> I believe the crash due to Windows
> running out of handles is fixed by the patch here, but the more general
> problem of not being able to print PDFs with many pages continues to exist.

Comment 4

[Cameron McCormack (:heycam)]

By the way this still only seems to happen when you choose to print a selection, not the whole document.  I'm not sure why that is yet.

Comment 5

[Brendan Dahl [:bdahl]]

(In reply to Cameron McCormack (:heycam) from comment #2)
> The PDF from bug 830214 has 638 pages.  The canvases are 1190x1684, so with
> 4 bytes per pixel that would take 4.4 GB of memory.  I think my suggestion
> in comment 0 wouldn't avoid this, either, since the canvases all need to
> remain in the document with their image data when printing happens.
> 
> Brendan, I think we need to handle printing in pdf.js a little differently
> for large documents like this.  Is it possible for the print engine to have
> each page's canvas callback invoked just when it needs to print that page? 
> Or is that not allowed since script shouldn't be able to run in the middle
> of printing?  Could we allow script to run between each page, and check that
> the document is still in a good state for printing each time it runs?

Dispatching the callbacks one after another sounds good, and shouldn't be to hard to do. I'm not actually sure why we didn't do that to start with(I didn't write the original patch).

That's very strange that only print selection causes this issue.

Comment 6

[Alex Keybl [:akeybl]]

Cameron - how risky would the fix you propose be? Could it be completed before Tuesday to make it into Beta 4, or should we start making other preparations?

Comment 7

[Cameron McCormack (:heycam)]

(In reply to Brendan Dahl from comment #5)
> Dispatching the callbacks one after another sounds good, and shouldn't be to
> hard to do. I'm not actually sure why we didn't do that to start with(I
> didn't write the original patch).

Per comment 2, I don't think simply changing nsSimplePageSequence::PrePrintNextPage to wait until one canvas callback is done before allocating the image surface and invoking the callback for the next canvas will help actually

Now that I think about it: should nsSimplePageSequenceFrame::PrePrintNextPage be invoked once per page that is going to be printed?  If so, why does mCurrentCanvasList contain all ~600 canvases?  Is there something about selections that causes GetPrintCanvasElementsInFrame to think they are all within the a single page?

(In reply to Alex Keybl [:akeybl] from comment #6)
> Cameron - how risky would the fix you propose be? Could it be completed
> before Tuesday to make it into Beta 4, or should we start making other
> preparations?

I am leaving for a conference tomorrow and won't be back until after Tuesday, so I'm not going to have time to look at it, sorry.

Comment 8

[Cameron McCormack (:heycam)]

So it looks like when printing a selection, all of the canvases get put in the same page frame, but when printing the whole document, each canvas is in a page frame of its own.

Comment 9

[Cameron McCormack (:heycam)]

Actually, printing of selections in pdf.js doesn't seem to work at all.  Should it?  Even if I open a very simple PDF and select some text to print, I get a blank document (apart from the page headers/footers).

Comment 10

[Cameron McCormack (:heycam)]

Attachment: disallow selection printing in pdf.js

If printing of selections in pdf.js doesn't work at all, and the plan is still to ship it in Firefox 19, then I suggest disabling selection printing.  If that's the case, here's a patch that does it (though I suspect the change in pdf.js should go upstream).

Comment 11

[Julian Viereck]

(\cc :dholbert as I think he knows about printing of selection and the "magic" that goes the way there)

In comment on:

(In reply to Cameron McCormack (:heycam) from comment #7)
> Now that I think about it: should
> nsSimplePageSequenceFrame::PrePrintNextPage be invoked once per page that is
> going to be printed?  If so, why does mCurrentCanvasList contain all ~600
> canvases?  Is there something about selections that causes
> GetPrintCanvasElementsInFrame to think they are all within the a single page?

(In reply to Brendan Dahl from comment #5)
> Dispatching the callbacks one after another sounds good, and shouldn't be to
> hard to do. I'm not actually sure why we didn't do that to start with(I
> didn't write the original patch).

Looking at the patch at https://bug745025.bugzilla.mozilla.org/attachment.cgi?id=657086, we the nsSimplePageSequenceFrame::PrePrintNextPage is called one page after another. |mCurrentCanvasList| is computed by calling

  GetPrintCanvasElementsInFrame(mCurrentPageFrame, &mCurrentCanvasList);

from within nsSimplePageSequenceFrame::PrePrintNextPage.

When I was working on the printing code, I think :dholbert told me, that printing the selection is actually doing crasy stuff. I'm not sure, but I think it was something in the lines of "we create one huge page and divide it again". That might be the case why all the canvas end up in the one |mCurrentCanvasList|.

@dholbert: can you maybe help out here?

Comment 12

[Brendan Dahl [:bdahl]]

Comment on attachment 706707 [details] [diff] [review]
disallow selection printing in pdf.js

Review of attachment 706707 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Cameron McCormack (:heycam) from comment #10)
> Created attachment 706707 [details] [diff] [review]
> disallow selection printing in pdf.js
> 
> If printing of selections in pdf.js doesn't work at all, and the plan is
> still to ship it in Firefox 19, then I suggest disabling selection printing.
> If that's the case, here's a patch that does it (though I suspect the change
> in pdf.js should go upstream).

I misread the previous comments. I thought you were referring to printing a range of pages(which should work).  Printing a selection is probably just picking up our invisible text layer that we use for text selection and none of the canvases. I'm fine with disabling print selection and can upstream the changes.

I can only review the changes to the viewer.html file, someone else should review the rest.

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Julian Viereck from comment #11)
> When I was working on the printing code, I think :dholbert told me, that
> printing the selection is actually doing [...] "we create one huge page and divide
> it again".
[...]
> That might be the case why all the canvas end up in the one
> |mCurrentCanvasList|.
> 
> @dholbert: can you maybe help out here?

That's basically right, yeah -- for print-selection, we create one giant page and print it in chunks.  It's kind of crazy, but we don't have anything better. :)

The code for that is spread out a bit, but at least one place where we do that chunking is here, in nsPageFrame::BuildDisplayList, FWIW:
  https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsPageFrame.cpp#559

I haven't touched that code for a while, but I'd definitely believe that this could cause memory usage issues if that giant page had a giant canvas, or a zillion merged page-sized canvases, or something like that.

Disabling print-selection within PDFs seems reasonable to me, anyway.

Comment 14

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d016fb6d847

Comment 15

[Julian Viereck]

(In reply to Daniel Holbert [:dholbert] from comment #13)
> The code for that is spread out a bit, but at least one place where we do
> that chunking is here, in nsPageFrame::BuildDisplayList, FWIW:
>  
> https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsPageFrame.
> cpp#559


Looking at that code, I guess the right aproach is to make GetPrintCanvasElementsInFrame take a look at the current clipping of the pageFrame and only get the canvas element that are in the clipped area. Does this sounds to be the reasonable way to fix this? (Note: I don't have the time ATM to do this myself :/)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8d016fb6d847

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 706707 [details] [diff] [review]
disallow selection printing in pdf.js

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Trying to print a selection in pdf.js will either result in a blank printed document or an OOM crash
Testing completed (on m-c, etc.): patch is on m-c; tested locally with corresponding pdf.js changes
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: N/A

The corresponding pdf.js change will need to be made upstream and then ported to beta/aurora too.

Comment 18

[Scoobidiver (away)]

Here is a crash ID when printing a selection in a large PDF, bp-7740721d-9fce-48a8-8a39-931cf2130124, before the patch landed.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 706707 [details] [diff] [review]
disallow selection printing in pdf.js

Fixes a crash & regression, low risk - approving.

Comment 20

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/releases/mozilla-beta/rev/51342071344d
https://hg.mozilla.org/releases/mozilla-aurora/rev/481e98d4984e

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Why is this [leave open]? We normally don't keep bugs open if they're only waiting on a branch uplift.

Comment 22

[Cameron McCormack (:heycam)]

Oh OK.  I normally do leave bugs open that await approval or landing on branches, but if that's not the done thing I'll avoid it in future.  Since dependent bug 835954 will land the pdf.js changes, let's close this bug.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Cameron, when I look at the Aurora/Beta changesets, it appears that the viewer.html change wasn't actually included? That's fine for m-c since we'll get it from upstream, but do we need a follow-up patch for the branches?

Comment 24

[Ryan VanderMeulen [:RyanVM]]

BTW, we either need either a follow-up bug to fix the underlying issue here or to leave this one open until a proper fix lands.

Comment 25

[Yury Delendik (:yury)]

Attachment: Disable printing a selection for PDF Viewer (beta/aurora)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): --
User impact if declined: printing selections in pdf.js will not work and can cause OOM errors in large PDFs
Testing completed (on m-c, etc.): https://tbpl.mozilla.org/?tree=Try&rev=e2e77cc0dbfa
Risk to taking this patch (and alternatives if risky): low, affects PDF Viewer functionality only
String or UUID changes made by this patch: --

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e5b2b8b59eb5
https://hg.mozilla.org/releases/mozilla-aurora/rev/41c33989407b

Comment 27

[Cameron McCormack (:heycam)]

Thanks for handling that Ryan.  I had assumed that the pdf.js update to m-c would be uplifted to Aurora and Beta too.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/af8e5720ccf2

We still need a follow-up for fixing the underlying issue.

Comment 29

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.1; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130206083616

Printing selections in pdf.js still crashes Firefox 19 beta 5:
https://crash-stats.mozilla.com/report/index/bb4f17b9-0685-43dc-acac-a6eac2130207

Everything happens as described in Comment 12 in Bug 830214, with the difference that the signature is not empty anymore.

Comment 30

[Julian Viereck]

(In reply to Simona B [QA] from comment #29)
> Mozilla/5.0 (Windows NT 6.1; rv:19.0) Gecko/20100101 Firefox/19.0
> Build ID: 20130206083616
> 
> Printing selections in pdf.js still crashes Firefox 19 beta 5:
> https://crash-stats.mozilla.com/report/index/bb4f17b9-0685-43dc-acac-
> a6eac2130207
> 
> Everything happens as described in Comment 12 in Bug 830214, with the
> difference that the signature is not empty anymore.

Are the required changes to PDF.JS from Comment #25 included in that version of Firefox already?

Comment 31

[Simona Badau, Desktop QA]

(In reply to Julian Viereck from comment #30)
> (In reply to Simona B [QA] from comment #29)
> > Mozilla/5.0 (Windows NT 6.1; rv:19.0) Gecko/20100101 Firefox/19.0
> > Build ID: 20130206083616
> > 
> > Printing selections in pdf.js still crashes Firefox 19 beta 5:
> > https://crash-stats.mozilla.com/report/index/bb4f17b9-0685-43dc-acac-
> > a6eac2130207
> > 
> > Everything happens as described in Comment 12 in Bug 830214, with the
> > difference that the signature is not empty anymore.
> 
> Are the required changes to PDF.JS from Comment #25 included in that version
> of Firefox already?

It should be, the tracking flag for Firefox 19 is set to fixed, and according with Comment 26 the changes landed in 2013-02-05.

Comment 32

[u279076]

Steps to Reproduce:
1. Load http://www.samba.org/samba/docs/Samba3-ByExample.pdf
2. Select "Samba-3 by Example" (top line of the first page)
3. Click the PDF.js print icon
4. Select "Selection" under "Print range" in the Print dialog
5. Click "OK"

Results:
Firefox 19.0b4 crashes after several seconds with an empty signature.
Firefox 19.0b5 crashes after several seconds with an empty signature.
Firefox 20.0a2 2013-02-07 crashes after several seconds with an empty signature.
Firefox 21.0a1 2013-02-07 crashes after several seconds with an empty signature.

Resetting status flags to affected at Alex's request.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Move mDisallowSelectionPrint check into IsThereARangeSelection to make sure code doesn't re-enable selection printing

http://dxr.mozilla.org/mozilla-central/embedding/components/printingui/src/win/nsPrintDialogUtil.cpp.html#l1047 was calling IsThereARangeSelection via GetIsRangeSelection and reenabling selection printing based on the result. This is arguably bogus, but anyway the correct thing to do is make IsThereARangeSelection always return false when mDisallowSelectionPrint is set.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2 for real

That patch had some extra goop in it. This is the correct patch.

Comment 35

[Cameron McCormack (:heycam)]

Comment on attachment 711556 [details] [diff] [review]
Part 2 for real

Review of attachment 711556 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85751b3b03b6

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 711556 [details] [diff] [review]
Part 2 for real

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: crashes when trying to print selection in PDF.js
Testing completed (on m-c, etc.): Just landed
Risk to taking this patch (and alternatives if risky): very low risk
String or UUID changes made by this patch: none

Comment 38

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/85751b3b03b6

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 711556 [details] [diff] [review]
Part 2 for real

We'll need this on beta as well.

Comment 40

[u279076]

I have confirmed with Firefox Nightly 21.0a1 2013-02-08 that the Print Selection option is no longer enabled.

Comment 41

[u279076]

Adding the verifyme keyword so we can verify this once it lands on other branches.

Comment 42

[Alex Keybl [:akeybl]]

Comment on attachment 711556 [details] [diff] [review]
Part 2 for real

Given the low risk profile here, approving for Aurora/Beta already.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8755cbf3acc5
https://hg.mozilla.org/releases/mozilla-beta/rev/ddedca74a489

Comment 44

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.1; rv:19.0) Gecko/20100101 Firefox/19.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:19.0) Gecko/20100101 Firefox/19.0
Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130212082553

Verified that Print Selection option is not enabled on Firefox 19 beta 6.

Comment 45

[Simona Badau, Desktop QA]

Verified as fixed on Firefox 20 beta 1 - print selection option is disabled.

Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20100101 Firefox/20.0
Build ID: 20130220104816

Comment 46

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old