Closed Bug 1305963 Opened 4 years ago Closed 4 years ago

[PDF Viewer] Incorrect rendering of PDF file in Firefox 51

Categories

(Core :: Canvas: 2D, defect, P1)

51 Branch
x86_64
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 --- unaffected
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 + fixed
firefox52 + fixed

People

(Reporter: Snuffleupagus, Assigned: ethlin)

Details

(Keywords: regression)

Attachments

(5 files)

Attached image Good_Firefox50.png
STR: Open this PDF file in Firefox: https://www.sec.gov/investor/pubs/sec-guide-to-proxy-brochures.pdf

ER: The PDF file is rendered correctly, please see the "Good_Firefox50.png" screenshot from Firefox 50.

AR: The PDF file renders incorrectly, please see the "Bad_Firefox51.png" screenshot.
Note that the actual rendering errors seem be be somewhat random, since changing the zoom level in the PDF Viewer or even reloading the file can result in slightly different rendering errors.

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=12c339c6063083e62ead7eeb20a4505982fd39aa&tochange=dab0693cae68fd5da764f311b20383d905435b1c

Suspected bug:
Ethan Lin — Bug 1296166 - Avoid calling EnsureTarget in CanvasRenderingContext2D::GetImageData. r=nical
Flags: needinfo?(ethlin)
Attached image Bad_Firefox51.png
Summary: [PDF Viewer] → [PDF Viewer] Incorrect rendering of PDF file in Firefox 51
It looks like we still need EnsureTarget in GetImageData(). I'll see how to fix it.
Assignee: nobody → ethlin
Flags: needinfo?(ethlin)
Priority: -- → P1
This is the log of all canvas calls issued on this pdf mentioned in the STR. The log contains the address of the CanvasRenderingContext2D object (note that there are several of them).

Calling EnsureTarget at the end of GetImageData fixed the issue, which indicates that this might be a bug happening not in GetImageData, but in one of the commands coming immediately after on the same canvas.

Also interesting to note, I have not been able to reproduce this with the PersistentBufferProviderShared, while I reproduce 100% of the ime with PersistentBufferProviderBasic.
(In reply to Nicolas Silva [:nical] from comment #3)
> Created attachment 8796476 [details]
> Log of most of the canvas method calls
> 
> This is the log of all canvas calls issued on this pdf mentioned in the STR.
> The log contains the address of the CanvasRenderingContext2D object (note
> that there are several of them).
> 
> Calling EnsureTarget at the end of GetImageData fixed the issue, which
> indicates that this might be a bug happening not in GetImageData, but in one
> of the commands coming immediately after on the same canvas.
> 
> Also interesting to note, I have not been able to reproduce this with the
> PersistentBufferProviderShared, while I reproduce 100% of the ime with
> PersistentBufferProviderBasic.

Thanks for the analysis. It helps a lots. I think I found the problem according to the canvas calls. I'll have a patch for it.
Attached patch Add EnsureTargetSplinter Review
'GetMozCurrentTransform()' and 'GetMozCurrentTransformInverse()' should call 'EnsureTarget()' too or the transform will be wrong. So it's no thing to do with 'GetImageData()' and the bug may happen with other combination.
Attachment #8797050 - Flags: review?(nical.bugzilla)
Comment on attachment 8797050 [details] [diff] [review]
Add EnsureTarget

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

Good catch!
Attachment #8797050 - Flags: review?(nical.bugzilla) → review+
Keywords: checkin-needed
Tracking this PDF regression for 52.
Can we please get a regression test for this issue?
Flags: needinfo?(ethlin)
Flags: in-testsuite?
Component: PDF Viewer → Canvas: 2D
Product: Firefox → Core
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30cc764b67a4
Add EnsureTarget for GetMozCurrentTransform and GetMozCurrentTransformInverse. r=nical
Keywords: checkin-needed
Attached patch add reftestSplinter Review
Add reftests for mozCurrentTransform and mozCurrentTransformInverse. Without the fix, we'll get identical matrix from mozCurrentTransform or mozCurrentTransformInverse, and then draw a wrong rectangle without the transform.
Flags: needinfo?(ethlin)
Attachment #8797335 - Flags: review?(nical.bugzilla)
Attachment #8797335 - Flags: review?(nical.bugzilla) → review+
https://hg.mozilla.org/mozilla-central/rev/30cc764b67a4
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Track 51+ as regression in pdf viewer.

Hi :ethlin,
Since this bug is a regression and also affects 51, do you consider to uplift this for 51 if the patch is not too risky?
Flags: needinfo?(ethlin)
(In reply to Gerry Chang [:gchang] from comment #12)
> Track 51+ as regression in pdf viewer.
> 
> Hi :ethlin,
> Since this bug is a regression and also affects 51, do you consider to
> uplift this for 51 if the patch is not too risky?

Sure.
Flags: needinfo?(ethlin)
Comment on attachment 8797050 [details] [diff] [review]
Add EnsureTarget

Approval Request Comment
[Feature/regressing bug #]: Bug 1296166
[User impact if declined]: PDF files may display wrong.
[Describe test coverage new/current, TreeHerder]: Tested on locally and try server.
[Risks and why]: Low. The patch just ensures the draw target at the beginning of the functions.
[String/UUID change made/needed]: None.
Attachment #8797050 - Flags: approval-mozilla-aurora?
Comment on attachment 8797050 [details] [diff] [review]
Add EnsureTarget

Fix a regression related to pdf viewer. Take it in 51 aurora.
Attachment #8797050 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
we still need to land the reftest still right ? seems the test hasn't landed so far
Flags: needinfo?(ethlin)
(In reply to Carsten Book [:Tomcat] from comment #16)
> we still need to land the reftest still right ? seems the test hasn't landed
> so far

Right. I sent the reftest to try server.
The result looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c665506c56e

Please push the attachment 8797335 [details] [diff] [review].
Flags: needinfo?(ethlin)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b548329e01cb
Add reftests for mozCurrentTransform and mozCurrentTransformInverse. r=nical
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/c8b7e869c00a
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.