1324000 - Crash in nsPrintEngine::DonePrintingPages

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Bob Owen (:bobowen)]

Spotted this while working on another printing bug.

Looks like there is already a bug for this (bug 413942), although it was originally opened for the same as bug 1321566, I think.
Example crash:
https://crash-stats.mozilla.com/report/index/acc8befd-d765-4e55-91bd-f4c6f2161215

Anyway as that's a really old bug, thought it was best to open a new sec bug for this, with no links back from the original.

mPageSeqFrame originally comes from [1], but never gets nulled out.
Looks like we don't reference count frames for performance reasons.

When I saw it, it occurred when the nsPrintEngine is reused and we fail before we've got the new mPageSeqFrame, but DonePrintingPages still gets called.

But it generally (according to crash-stats) seems to happen during printing, when called from [2].
Not sure how it's been deleted at this point. My first thought was to do away with mPageSeqFrame and get it from the nsPrintObject each time, but as far as I can tell the object pointed to by mPageSeqFrame only gets deleted when the nsPrintObject dies, so maybe that's not valid at this point either.

[1] https://hg.mozilla.org/releases/mozilla-release/file/cc272f7d48d3/layout/printing/nsPrintEngine.cpp#l2415
[2] https://hg.mozilla.org/releases/mozilla-release/file/cc272f7d48d3/layout/printing/nsPagePrintTimer.cpp#l92

Comment 1

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

nsWeakFrame could we used here.

Comment 2

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

nsWeakFrame could be used here.

Comment 3

[Bob Owen (:bobowen)]

Attachment: Use nsWeakFrame to hold nsIPageSequenceFrame member in nsPrintEngine

Not sure if I'm using nsWeakFrame correctly here.

Comment 4

[Bob Owen (:bobowen)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a610591e1fcbdf489d02ceaf19e0c2f91ce37fe

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 6

[Bob Owen (:bobowen)]

Comment on attachment 8824715 [details] [diff] [review]
Use nsWeakFrame to hold nsIPageSequenceFrame member in nsPrintEngine

Approval Request Comment
[Feature/Bug causing the regression]:
Long-term crash.

[User impact if declined]:
Small number of users will still experience this crash.

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
We do not have STR, but tested printing manually.

[Needs manual test from QE? If yes, steps to reproduce]: 
No STR.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
Slightly.

[Why is the change risky/not risky?]:
The changes are straight forward, but the printing code is quite fragile and does not have automated testing. So only requesting uplift to Aurora.

[String changes made/needed]:
No

Comment 7

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8824715 [details] [diff] [review]
Use nsWeakFrame to hold nsIPageSequenceFrame member in nsPrintEngine

printing-related uaf fix for aurora52

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c0ed1d46370d