Closed Bug 1326418 Opened 5 years ago Closed 5 years ago

Assertion failure: !mHasActivePage (Missing EndPage() call) when trying to print-to-file


(Core :: Printing: Output, defect)

50 Branch
Not set



Tracking Status
firefox53 --- fixed


(Reporter: smaug, Unassigned)




(1 file)

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 and print it to a pdf.

(I guess I could test my patch on top of code before bug 1280324 landed)
Flags: needinfo?(jwatt)
What platform? This assertion doesn't happen for me on macOS.
Flags: needinfo?(jwatt)
(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.
Flags: needinfo?(jwatt)
Attached patch wipSplinter Review
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.
Pushed by
part 1 - Make PrintTargetPDF::EndPage call PrintTarget::EndPage to fix MOZ_ASSERT failures. r=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.
Flags: needinfo?(jwatt)
Comment on attachment 8822877 [details] [diff] [review]

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.)
Attachment #8822877 - Flags: review-
(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.
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.