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

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect, P1)

Description

[Jonas Jenwald [:Snuffleupagus]]

Attachment: 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

Comment 1

[Jonas Jenwald [:Snuffleupagus]]

Attachment: Bad_Firefox51.png

Comment 2

[Ethan Lin[:ethlin]]

It looks like we still need EnsureTarget in GetImageData(). I'll see how to fix it.

Comment 3

[Nicolas Silva [:nical]]

Attachment: 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.

Comment 4

[Ethan Lin[:ethlin]]

(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.

Comment 5

[Ethan Lin[:ethlin]]

Attachment: Add EnsureTarget

'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.

Comment 6

[Nicolas Silva [:nical]]

Comment on attachment 8797050 [details] [diff] [review]
Add EnsureTarget

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

Good catch!

Comment 7

[Marcia Knous [:marcia]]

Tracking this PDF regression for 52.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Can we please get a regression test for this issue?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30cc764b67a4
Add EnsureTarget for GetMozCurrentTransform and GetMozCurrentTransformInverse. r=nical

Comment 10

[Ethan Lin[:ethlin]]

Attachment: add reftest

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.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/30cc764b67a4

Comment 12

[Gerry Chang [:gchang]]

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?

Comment 13

[Ethan Lin[: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.

Comment 14

[Ethan Lin[: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.

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8797050 [details] [diff] [review]
Add EnsureTarget

Fix a regression related to pdf viewer. Take it in 51 aurora.

Comment 16

[Carsten Book [:Tomcat]]

we still need to land the reftest still right ? seems the test hasn't landed so far

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6acd130cd76f

Comment 18

[Ethan Lin[: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].

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b548329e01cb
Add reftests for mozCurrentTransform and mozCurrentTransformInverse. r=nical

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c8b7e869c00a

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b548329e01cb