1050788 - black and white image in pdf file turns black in nightly 34

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Felix Kälberer]

Steps to reproduce: Open the following file with pdf.js: http://cita.disability.uiuc.edu/presentations/pdf/examples/scanned_example_1.pdf

Results: The university logo turns to a black square after showing the correct logo _very_ shortly.

FF34 nightly (2014-08-04) shows the above issue,
FF33 nightly (2014-07-05) worked fine.

The issue is independent of the pdf.js-version (tested with 1.0.277 and 1.0.618).
The issue appears on Linux (Linux Mint in a VirtualBox), but not on Windows 7.

Comment 1

[WaltS48 [:walts48]]

Works in Firefox 31.0.

Logo turns black using Firefox 32.0b4 and Nightly 34.0a1.

Mozilla/5.0 (X11; Linux x86_64; rv:32.0) Gecko/20100101 Firefox/32.0 BuildID: 20140804164216

Mozilla/5.0 (X11; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0 BuildID: 20140808030201

Using openSUSE 13.1 KDE desktop.

Comment 2

[Yury Delendik (:yury)]

Attachment: Minimal test case to replicate the issue

Comment 3

[Loic]

Can someone use mozregression to find a regression range on Linux, please.
http://mozilla.github.io/mozregression/

Comment 4

[Yury Delendik (:yury)]

Last good revision: cb75d6cfb004 (2014-07-10)
First bad revision: e1a037c085d1 (2014-07-11)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cb75d6cfb004&tochange=e1a037c085d1

inbound
Last good revision: c3100f4255d1
First bad revision: 8d582915f484
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c3100f4255d1&tochange=8d582915f484

Comment 5

[Yury Delendik (:yury)]

[Tracking Requested - why for this release]:

Comment 6

[Yury Delendik (:yury)]

[Tracking Requested - why for this release]:

There was uplift of the bug 1034593 to the FF32.

Comment 7

[Sylvestre Ledru [:sylvestre]]

Nical, could you help here? Merci

Comment 8

[Nicolas Silva [:nical]]

I am on PTO next week and won't have time for this today, I'll assign myself in a week if the bug hasn't been assigned/fixed by then.

Comment 9

[Lawrence Mandel [:lmandel] (use needinfo)]

Milan - Nical is out. If we want a fix for this we need it before Thu. What is your take on this bug and do you have anyone who can take a look at it?

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Yury Delendik (:yury) from comment #2)
> Created attachment 8470100 [details]
> Minimal test case to replicate the issue

I'm assuming the correct result on the minimal test case is just white page, because everything is clipped out, and the bug shows as the red rectangle.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Yury Delendik (:yury) from comment #4)
> Last good revision: cb75d6cfb004 (2014-07-10)
> First bad revision: e1a037c085d1 (2014-07-11)
> Pushlog:
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=cb75d6cfb004&tochange=e1a037c085d1
> 
> inbound
> Last good revision: c3100f4255d1
> First bad revision: 8d582915f484
> Pushlog:
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=c3100f4255d1&tochange=8d582915f484

While this analysis is no doubt correct, I find that just backing out this change on the latest trunk does not fix the problem. I wonder if the backout of the first patch on bug 1034593 messed up the inbound search.  I'll dig in a bit more.

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

I confirmed both, at a first glance contradictory, comment 4 and comment 11 are correct, based on local builds.  This started happening (at least in the range I looked at), as mentioned in comment 4, and it was due to CanvasRenderingContext2D.cpp change from bug 1034593.  Since then, something else happened on trunk that made it so that backing out the offending change does not fix the problem anymore.  I would have said Skia update, except that we're not using Skia canvas in this case.  I will try aurora.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

On Aurora, I verified that backing out the fix to bug 1034593 does in fact fix the problem.  I assume the same to be the case for the Beta.

On the trunk, I verified that backing out the fix to bug 1034593 *does not* fix the problem.

Comment 14

[Robert Kaiser]

(In reply to Milan Sreckovic [:milan] from comment #13)
> On Aurora, I verified that backing out the fix to bug 1034593 does in fact
> fix the problem.

Well, getting the crashes back might be worse. IIRC, that bug was supposed to fix a root cause of crashes we had papered over with a different change before. Sad to hear it causes other issues.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Probably bug 1051592 for why the backout doesn't work on trunk.

This looks like a cairo clipping bug (again). Bas was looking at one of these recently, any idea if it's related?

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Is this restricted to Linux or has it been fixed elsewhere as I can't reproduce with the PDF link in the description on Nightly 2014-08-20 on OSX?

Also, as today is the last beta 32 build, it looks like we're going to ship this regression in 32.

Comment 17

[Yury Delendik (:yury)]

(In reply to Lawrence Mandel [:lmandel] from comment #16)
> Is this restricted to Linux or has it been fixed elsewhere as I can't
> reproduce with the PDF link in the description on Nightly 2014-08-20 on OSX?
> 

Only on Linux at the moment

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

Yes, this is more complicated and dangerous to jump into because of the difference between the trunk and beta, as well as the fact that the obvious backout is of a bug that fixed a crash.  So, we live with this until we get a proper fix.

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Milan Sreckovic [:milan] from comment #18)
> Yes, this is more complicated and dangerous to jump into because of the
> difference between the trunk and beta, as well as the fact that the obvious
> backout is of a bug that fixed a crash.  So, we live with this until we get
> a proper fix.

Agreed. With apologies to our Linux users, we're going to have to live with this in 32.

Comment 20

[Matt Woodrow (:mattwoodrow)]

Attachment: Reftest

Comment 21

[Matt Woodrow (:mattwoodrow)]

Attachment: Fix region clipping with complex-but-empty regions

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8477127 [details] [diff] [review]
Reftest

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

::: layout/reftests/bugs/1050788-1.html
@@ +10,5 @@
> +      ctx.moveTo(0,0);
> +      ctx.lineTo(100,0);
> +      ctx.lineTo(100, 100);
> +      ctx.lineTo(0,100);
> +      ctx.closePath();

Use ctx.rect()?

@@ +16,5 @@
> +      ctx.moveTo(0,0);
> +      ctx.lineTo(100,0);
> +      ctx.lineTo(100, 100);
> +      ctx.lineTo(0,100);
> +      ctx.closePath();

Use ctx.rect()?

Comment 23

[Yury Delendik (:yury)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> Comment on attachment 8477127 [details] [diff] [review]
> Reftest
> 
> Review of attachment 8477127 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/reftests/bugs/1050788-1.html
> @@ +10,5 @@
> > +      ctx.moveTo(0,0);
> > +      ctx.lineTo(100,0);
> > +      ctx.lineTo(100, 100);
> > +      ctx.lineTo(0,100);
> > +      ctx.closePath();
> 
> Use ctx.rect()?

Per spec the rect has a diffect meaning from the above: "The rect(x, y, w, h) method must create a new subpath containing just the four points (x, y), (x+w, y), (x+w, y+h), (x, y+h), with those four points connected by straight lines, and must then mark the subpath as closed. It must then create a new subpath with the point (x, y) as the only point in the subpath." (During PDF display we need it to be as "append a rectangle to the current path as a complete subpath... equivalent to: x y m (x+width) y l (x+width) (y+height) l x (y+height) l h")

Comment 24

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ee7a98d1e2f
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3474588bddd

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5ee7a98d1e2f
https://hg.mozilla.org/mozilla-central/rev/c3474588bddd

Comment 26

[Sylvestre Ledru [:sylvestre]]

Matth, could you fill the uplift request for 33? Thanks

Comment 27

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8477130 [details] [diff] [review]
Fix region clipping with complex-but-empty regions

Approval Request Comment
[Feature/regressing bug #]:Bug 1034593
[User impact if declined]: Incorrect clipping in rare <canvas> cases
[Describe test coverage new/current, TBPL]: Tested manually, added reftest
[Risks and why]: Low risk, just makes us bail out from an optimized clip path when it was goingto fail.
[String/UUID change made/needed]: None

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/44c394208273
https://hg.mozilla.org/releases/mozilla-aurora/rev/3e0815b50bae