Closed Bug 1292870 Opened 4 years ago Closed 4 years ago

PDF rendering regression, areas with content being rendered blank

Categories

(Firefox :: PDF Viewer, defect)

50 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 51
Tracking Status
firefox48 --- unaffected
firefox49 --- unaffected
firefox50 --- verified
firefox51 --- verified

People

(Reporter: ke5trel, Assigned: nical)

References

Details

(Keywords: regression, Whiteboard: [regression])

Attachments

(3 files)

Whiteboard: [regression]
confirmed on linux and OSX.

here's regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=57cf7cae92f1b234b763240a33f030fd68fc2166&tochange=56715a33b016d05afc2541e19fd7e0c907258971

looks like yet another regression from bug 1167235.
nical, do you have any idea?
Blocks: 1167235
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(nical.bugzilla)
Keywords: regression
I am looking into this. Interestingly it reproduces both with and without the SharedBufferProvider, so not a regression from the big architectural change in bug 1167235 like all of the other regressions so far, although it could have been introduced by a related patch.
Flags: needinfo?(nical.bugzilla)
This bug is caused by the order in which we restore clips and transforms. Currently for each state in the style stack we restore the transform and then all clips associated with that state. Clips are affected by the transform and can be applied before and/or after the transform so we have to record restore clips and transforms in a way that preserve the ordering.
It would be so much easier if we could just swap the destination surface and preserve all of the drawing state encapsulated in the DrawTarget, instead of duplicating this information externally.
This fixes the issue.
Assignee: nobody → nical.bugzilla
Attachment #8778856 - Flags: review?(bas)
Comment on attachment 8778856 [details] [diff] [review]
Record transforms in the style stack.

Review of attachment 8778856 [details] [diff] [review]:
-----------------------------------------------------------------

This is gross :(
Attachment #8778856 - Flags: review?(bas) → review+
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5584f816e28
Record/replay clips and transforms properly in CanvasRenderingContext2D. r=Bas
https://hg.mozilla.org/mozilla-central/rev/a5584f816e28
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
I have reproduced this bug with Firefox Nightly 51.0a1 (Build ID:20160806030806) on 
Windows 8.1, 64-bit.

Verified as fixed with Latest Firefox Nightly 51.0a1 (Build ID: 20160811030201)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
QA Whiteboard: [bugday-20160810]
Will this be uplifted?
Flags: needinfo?(nical.bugzilla)
(In reply to Brendan Dahl [:bdahl] from comment #9)
> Will this be uplifted?

Yes, but first I want to hold off for a few days while I investigate a talos regression that may have been caused by this patch.
Flags: needinfo?(nical.bugzilla)
I have reproduced this bug with Nightly 51.0a1 (2016-08-06) on Ubuntu 14.04, 64 bit!

The bug's fix is now verified on latest Nightly 51.0a1.

Nightly 51.0a1:
Build ID 	20160812030200
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0

[testday-20160812]
(In reply to Nicolas Silva [:nical] from comment #10)
> (In reply to Brendan Dahl [:bdahl] from comment #9)
> > Will this be uplifted?
> 
> Yes, but first I want to hold off for a few days while I investigate a talos
> regression that may have been caused by this patch.

Is it possible to uplift this now?
Flags: needinfo?(nical.bugzilla)
Comment on attachment 8778856 [details] [diff] [review]
Record transforms in the style stack.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: canvas correctness issue that affects pdfjs badly
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: low, may cause with a perf regression, which is fixed by another patch that is upliftable with low risks as well.
[String/UUID change made/needed]:
Flags: needinfo?(nical.bugzilla)
Attachment #8778856 - Flags: approval-mozilla-aurora?
Hello Kestrel, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(kestrel)
Comment on attachment 8778856 [details] [diff] [review]
Record transforms in the style stack.

new regression in 50, Aurora50+
Attachment #8778856 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi Joel, in the uplift request (comment 13), Nical mentioned a potential perf impact. So this is just an FYI for when you guys do perf testing on 50 in a week or so.
Flags: needinfo?(jmaher)
Needs rebasing for Aurora.
Flags: needinfo?(nical.bugzilla)
Verified as fixed for Nightly 51.0a1 (2016-09-01) on Ubuntu 16.04.
Status: RESOLVED → VERIFIED
Flags: needinfo?(kestrel)
oh this is great to know ahead of time!
Flags: needinfo?(jmaher)
Attached patch Aurora versionSplinter Review
Flags: needinfo?(nical.bugzilla)
Hi Ryan, there's a rebased patch in comment 20.
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
Flags: qe-verify+
I managed to reproduce the issue using an old build of Firefox Nightly 50.0a1 (2016-07-09) on Ubuntu 16.04 LTS x64.

The fix is now verified on Firefox Beta 50.0b4 on the same platform.
You need to log in before you can comment on or make changes to this bug.