Closed Bug 1050788 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Graphics: Layers, defect)

34 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox31 --- unaffected
firefox32 + wontfix
firefox33 + fixed
firefox34 + fixed

People

(Reporter: l_l, Assigned: mattwoodrow)

References

Details

(Keywords: regression, Whiteboard: [pdfjs-c-rendering])

Attachments

(3 files)

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.
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.
Can someone use mozregression to find a regression range on Linux, please.
http://mozilla.github.io/mozregression/
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
Whiteboard: [pdfjs-c-rendering]
[Tracking Requested - why for this release]:
Blocks: 1034593
Component: PDF Viewer → Graphics: Layers
Product: Firefox → Core
Version: 34 Branch → 33 Branch
[Tracking Requested - why for this release]:

There was uplift of the bug 1034593 to the FF32.
Version: 33 Branch → 34 Branch
Nical, could you help here? Merci
Flags: needinfo?(nical.bugzilla)
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
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?
Flags: needinfo?(milan)
(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.
(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.
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.
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.
Flags: needinfo?(milan)
Flags: needinfo?(milan)
(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.
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?
Flags: needinfo?(bas)
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.
(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
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.
(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.
Assignee: nobody → matt.woodrow
Attached patch ReftestSplinter Review
Attachment #8477127 - Flags: review?(roc)
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()?
Attachment #8477127 - Flags: review?(roc) → review+
Flags: needinfo?(nical.bugzilla)
(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")
Attachment #8477130 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/5ee7a98d1e2f
https://hg.mozilla.org/mozilla-central/rev/c3474588bddd
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: needinfo?(milan)
Matth, could you fill the uplift request for 33? Thanks
Flags: needinfo?(MattN+bmo)
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
Attachment #8477130 - Flags: approval-mozilla-aurora?
Flags: needinfo?(bas)
Flags: needinfo?(MattN+bmo)
Attachment #8477130 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.