Currently PrintTarget::GetReferenceDrawTarget and its overrides are passed a DrawEventRecorder which we used to record draw events from the returned reference DrawTarget. This makes no sense. These DTs should simply be used for measuring while doing reflows. There should be no draw commands omitted to them, and no need to waste time or memory passing these DTs over to the parent process during printing.
Comment on attachment 8947879 [details] [diff] [review] patch Review of attachment 8947879 [details] [diff] [review]: ----------------------------------------------------------------- As discussed on IRC, perhaps a follow-up to come up with some way to ensure the reference DrawTarget isn't used for any actual draw commands.
Attachment #8947879 - Flags: review?(bobowencode) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/2ec583a00c07 Stop recording draw events for reference DrawTargets returned from PrintTarget. r=bobowen
(In reply to Bob Owen (:bobowen) from comment #3) > As discussed on IRC, perhaps a follow-up to come up with some way to ensure > the reference DrawTarget isn't used for any actual draw commands. I filed bug 1435357.
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment on attachment 8947879 [details] [diff] [review] patch Requesting beta approval since this fixes topcrash-win bug 1434762. Approval Request Comment [Feature/Bug causing the regression]: bug 1354624 [User impact if declined]: topcrash-win bug 1434762 [Is this code covered by automated tests?]: some, but we should have more [Has the fix been verified in Nightly?]: yes - bug 1434762 comment 5 and 6 [Needs manual test from QE? If yes, steps to reproduce]: myself and philipp have both manually tested the steps in bug 1434762 comment 0, but I think more manual testing of printing in general from QE would be wise [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: it *shouldn't* be [Why is the change risky/not risky?]: in principle this change is clearly the correct thing to do. However, the printing code is quite unloved and does some bizarre things in places. [String changes made/needed]: none
Attachment #8947879 - Flags: approval-mozilla-beta?
Agreed that this is a bit scary given the current state of our printing code. I'm going to take this for 59b8 because it fixes the topcrash, but I also agree that we should have QA do some printing smoketesting after it's uplifted.
status-firefox59: --- → affected
Attachment #8947879 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-firefox59: affected → fixed
I have reproduced this issue using Firefox 60.0a1(2018.02.02) on Win 8.1 x64. I can confirm this issue is fixed, I verified using Firefox 59.0b8 and on 60.0a1 (2018.02.09) on Win 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.13.3.
Status: RESOLVED → VERIFIED
status-firefox59: fixed → verified
status-firefox60: fixed → verified
You need to log in before you can comment on or make changes to this bug.