+++ 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?

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

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?

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.

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.

(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.

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?

(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.

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.

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).

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).

(\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 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.

(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.

(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 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.

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

Comment on attachment 706707 [details] [diff] [review] disallow selection printing in pdf.js Fixes a crash & regression, low risk - approving.

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

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

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.

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?

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.

[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: --

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

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

https://hg.mozilla.org/mozilla-central/rev/af8e5720ccf2 We still need a follow-up for fixing the underlying issue.

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.

(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?

(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.

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.

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.

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

Comment on attachment 711556 [details] [diff] [review] Part 2 for real Review of attachment 711556 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense.

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 on attachment 711556 [details] [diff] [review] Part 2 for real We'll need this on beta as well.

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

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

Comment on attachment 711556 [details] [diff] [review] Part 2 for real Given the low risk profile here, approving for Aurora/Beta already.

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

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.

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

mass remove verifyme requests greater than 4 months old