Stop recording draw events from PrintTarget reference DrawTargets

VERIFIED FIXED in Firefox 59

Status

()

VERIFIED FIXED
a year ago
a year ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

unspecified
mozilla60
Points:
---

Firefox Tracking Flags

(firefox59 verified, firefox60 verified)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
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+

Comment 4

a year ago
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
(Assignee)

Comment 5

a year ago
(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.

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2ec583a00c07
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
(Assignee)

Comment 7

a year ago
This fixes bug 1434762.
Blocks: 1434762
(Assignee)

Comment 8

a year ago
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
Flags: qe-verify+
Attachment #8947879 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 10

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/9d8a25ef4fd7
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
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.