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

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: smaug, Unassigned)

Tracking

50 Branch
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

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)
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)
Created attachment 8822877 [details] [diff] [review]
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

2 years ago
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f8218f821e3
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]
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.)
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.

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3f8218f821e3
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.