Closed
Bug 1050788
Opened 8 years ago
Closed 8 years ago
black and white image in pdf file turns black in nightly 34
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: l_l, Assigned: mattwoodrow)
References
Details
(Keywords: regression, Whiteboard: [pdfjs-c-rendering])
Attachments
(3 files)
745 bytes,
text/html
|
Details | |
1.96 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
2.74 KB,
patch
|
jrmuizel
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
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•8 years ago
|
||
Can someone use mozregression to find a regression range on Linux, please. http://mozilla.github.io/mozregression/
Keywords: regressionwindow-wanted
Comment 4•8 years ago
|
||
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
Keywords: regressionwindow-wanted → regression
Whiteboard: [pdfjs-c-rendering]
Comment 5•8 years ago
|
||
[Tracking Requested - why for this release]:
Blocks: 1034593
tracking-firefox33:
--- → ?
tracking-firefox34:
--- → ?
Component: PDF Viewer → Graphics: Layers
Product: Firefox → Core
Comment 6•8 years ago
|
||
[Tracking Requested - why for this release]: There was uplift of the bug 1034593 to the FF32.
tracking-firefox32:
--- → ?
Version: 33 Branch → 34 Branch
Updated•8 years ago
|
status-firefox31:
--- → unaffected
status-firefox32:
--- → affected
status-firefox33:
--- → affected
status-firefox34:
--- → affected
Updated•8 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 8•8 years ago
|
||
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•8 years ago
|
||
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)
Updated•8 years ago
|
Flags: needinfo?(milan)
![]() |
||
Comment 14•8 years ago
|
||
(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.
Assignee | ||
Comment 15•8 years ago
|
||
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)
Comment 16•8 years ago
|
||
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•8 years ago
|
||
(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.
Comment 19•8 years ago
|
||
(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 | ||
Updated•8 years ago
|
Assignee: nobody → matt.woodrow
Assignee | ||
Comment 20•8 years ago
|
||
Attachment #8477127 -
Flags: review?(roc)
Assignee | ||
Comment 21•8 years ago
|
||
Attachment #8477130 -
Flags: review?(jmuizelaar)
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+
Updated•8 years ago
|
Flags: needinfo?(nical.bugzilla)
Comment 23•8 years ago
|
||
(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")
Updated•8 years ago
|
Attachment #8477130 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 24•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ee7a98d1e2f https://hg.mozilla.org/integration/mozilla-inbound/rev/c3474588bddd
Comment 25•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5ee7a98d1e2f https://hg.mozilla.org/mozilla-central/rev/c3474588bddd
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Updated•8 years ago
|
Flags: needinfo?(milan)
Comment 26•8 years ago
|
||
Matth, could you fill the uplift request for 33? Thanks
Flags: needinfo?(MattN+bmo)
Assignee | ||
Comment 27•8 years ago
|
||
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)
Updated•8 years ago
|
Flags: needinfo?(MattN+bmo)
Updated•8 years ago
|
Attachment #8477130 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 28•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/44c394208273 https://hg.mozilla.org/releases/mozilla-aurora/rev/3e0815b50bae
You need to log in
before you can comment on or make changes to this bug.
Description
•