1326418 - Assertion failure: !mHasActivePage (Missing EndPage() call) when trying to print-to-file http://www.yle.fi/

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

I was trying to test a printing related patch, but even without my patch printing crashes on debug builds / linux, so couldn't really test my patch.
Testcase is to load http://www.yle.fi/ and print it to a pdf.

(I guess I could test my patch on top of code before bug 1280324 landed)

Comment 1

[Jonathan Watt [:jwatt]]

What platform? This assertion doesn't happen for me on macOS.

Comment 2

[Bob Owen (:bobowen)]

(In reply to Jonathan Watt [:jwatt] from comment #1)
> What platform? This assertion doesn't happen for me on macOS.

smaug is testing on Linux.
I've possibly seen something similar on Windows, but don't have time to test now.

Comment 3

[Mats Palmgren (inactive)]

Attachment: wip

I was investigating a different printing issue that hit this assertion so
I did a quick fix to get around it...

The error is that PrintTargetPDF overrides PrintTarget::EndPage
so the corresponding DEBUG code there is never invoked, so at
the next BeginPage() call (not overridden), the mHasActivePage
is still true.

The logic around the Begin/EndPage calls in nsSimplePageSequenceFrame
also looked wrong to me so I fixed those too.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f8218f821e3
part 1 - Make PrintTargetPDF::EndPage call PrintTarget::EndPage to fix MOZ_ASSERT failures. r=jwatt

Comment 5

[Jonathan Watt [:jwatt]]

r+ on the PrintTargetPDF changes, which I've landed on your behalf to get that fix into the hands of people like smaug. I'd like to look more closely at the nsSimplePageSequenceFrame changes though.

Comment 6

[Jonathan Watt [:jwatt]]

Comment on attachment 8822877 [details] [diff] [review]
wip

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

As far as I can see the current nsSimplePageSequenceFrame.cpp code is correct and your changes would break things. That said this code is pretty poorly structured.

::: layout/generic/nsSimplePageSequenceFrame.cpp
@@ +748,2 @@
>          } else {
>            mCalledBeginPage = false;

The point of this code is that after this point mCalledBeginPage should be false so that if we loop around to print another page's worth of selection we will call BeginPage again (mandatory). This change would break that.

@@ +780,2 @@
>          PR_PL(("***************** End Page (PrintNextPage) *****************\n"));
>          rv = dc->EndPage();

We do not want to call EndPage unless we know we're looping around and will be calling BeginPage again. (The normal place for EndPage to be called is in DoEndPage.)

Comment 7

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #6)
> That said this code is pretty poorly structured.

I'll put up a patch in bug 1328275 to try and tidy this code up somewhat.

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3f8218f821e3