Stop recording draw events from PrintTarget reference DrawTargets

VERIFIED FIXED in Firefox 59

Status

()

defect
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 verified, firefox60 verified)

Details

Attachments

(1 attachment)

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.
Posted patch patchSplinter Review
Attachment #8947879 - Flags: review?(bobowencode)
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 jwatt@jwatt.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.
https://hg.mozilla.org/mozilla-central/rev/2ec583a00c07
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
This fixes bug 1434762.
Blocks: 1434762
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.
Flags: qe-verify+
Attachment #8947879 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.