Last Comment Bug 624133 - Update libpng to version 1.4.7
: Update libpng to version 1.4.7
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla6
Assigned To: Glenn Randers-Pehrson
:
Mentors:
Depends on: 645519
Blocks: 648690 669863
  Show dependency treegraph
 
Reported: 2011-01-08 06:33 PST by Glenn Randers-Pehrson
Modified: 2011-08-01 07:50 PDT (History)
9 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
png_handle_fcTL() fix (3.35 KB, patch)
2011-02-04 02:43 PST, Max Stepin
no flags Details | Diff | Splinter Review
v00 Ugrade to libpng-1.4.6 (387.15 KB, patch)
2011-04-08 05:44 PDT, Glenn Randers-Pehrson
no flags Details | Diff | Splinter Review
v01 Ugrade to libpng-1.4.6 (389.45 KB, patch)
2011-04-08 16:01 PDT, Glenn Randers-Pehrson
no flags Details | Diff | Splinter Review
libpng 1.4.6 v1 patch (227.63 KB, patch)
2011-04-08 17:33 PDT, Ryan VanderMeulen [:RyanVM]
no flags Details | Diff | Splinter Review
v02 Upgrade to libpng-1.4.6 (304.15 KB, patch)
2011-04-08 17:46 PDT, Glenn Randers-Pehrson
no flags Details | Diff | Splinter Review
libpng 1.4.7 v1 patch (239.80 KB, patch)
2011-04-09 15:11 PDT, Ryan VanderMeulen [:RyanVM]
no flags Details | Diff | Splinter Review
v05 Upgrade to libpng-1.4.7 (303.66 KB, patch)
2011-04-10 10:31 PDT, Glenn Randers-Pehrson
no flags Details | Diff | Splinter Review
v5 patch update to libpng 1.4.7 (238.39 KB, patch)
2011-04-10 10:33 PDT, Ryan VanderMeulen [:RyanVM]
joe: review+
Details | Diff | Splinter Review
v06 Upgrade to libpng-1.4.7 (240.81 KB, patch)
2011-04-13 08:24 PDT, Glenn Randers-Pehrson
no flags Details | Diff | Splinter Review
v07 Fix coding errors in png_handle_fcTL (3.24 KB, patch)
2011-04-13 13:56 PDT, Glenn Randers-Pehrson
joe: review+
Details | Diff | Splinter Review

Description Glenn Randers-Pehrson 2011-01-08 06:33:20 PST
Libpng-1.5.0 has been released.  We should update Mozilla's embedded libpng to version 1.5.0, but there are no security or performance issues for Mozilla that I know of, so there's no hurry.
Comment 1 Glenn Randers-Pehrson 2011-01-08 10:46:10 PST
It seems that libpng-1.5.0 may have a bug with regard to decoding interlaced PNG images, so let's wait for 1.5.1.
Comment 2 Glenn Randers-Pehrson 2011-01-08 11:53:43 PST
Changed title because we will be waiting for 1.5.1.
Comment 3 Glenn Randers-Pehrson 2011-01-08 14:07:41 PST
The bug was not with interlaced images but with the png_rgb_to_gray
transformation, which we don't use.  But 1.5.1 will be out soon so
we might as well wait anyhow.
Comment 4 Max Stepin 2011-02-04 02:43:12 PST
Created attachment 509715 [details] [diff] [review]
png_handle_fcTL() fix

Glenn, please add this fix for png_handle_fcTL() too.

It fixed a problem which does *not* affect Firefox, but can cause trouble for people who would install Mozilla's version of libpng as a system library.
Comment 5 NVD 2011-03-31 11:40:14 PDT
Libpng 1.5.2 has been released, can we please get it in to the trunk before Firefox 5.0 ships?

http://libpng.sourceforge.net/

"UPDATE 31 March 2011: The latest released versions are libpng-1.5.2"
Comment 6 Glenn Randers-Pehrson 2011-03-31 14:06:54 PDT
The reduced libpng that we use doesn't require anything new from libpng-1.5.2.
We do need to apply the patch in bug #644519 though so users can run with a system libpng-1.5.x.
Comment 7 NVD 2011-04-08 04:10:03 PDT
Bug 645519 is fixed now, would it be possible to update to libpng 1.5.2 before the code freeze for Firefox 5.0 on the 12th?
Comment 8 Glenn Randers-Pehrson 2011-04-08 05:41:55 PDT
Probably not.  Applying the diff of system libpng-1.4.3 to the embedded apng-supporting libpng results in .rej files totaling 3793 lines.  So there's
a lot of manual slogging through that to be done and zero payoff, since as I
have said before, libpng-1.5.x doesn't offer anything new for mozilla.

Rushing through a big job like that would just open us up to mistakes and
possible security problems.

On the other hand, upgrading to libpng-1.4.6 would be fairly simple and
that is what I recommend.  I'd need help from someone who is able and
willing to to do triserver tests.  libpng-1.4.6 is scheduled for release
next week but we can test with libpng-1.4.6rc02 now.
Comment 9 Glenn Randers-Pehrson 2011-04-08 05:44:13 PDT
Created attachment 524609 [details] [diff] [review]
v00 Ugrade to libpng-1.4.6
Comment 10 Glenn Randers-Pehrson 2011-04-08 05:48:37 PDT
Note the v00 patch internally calls itself libpng-1.4.6rc02.  This is just for testing; a final patch will call itself libpng-1.4.6.
Comment 11 Ryan VanderMeulen [:RyanVM] 2011-04-08 06:05:15 PDT
I'll run this through Try for you tonight (EDT) when I get home from work.
Comment 12 Glenn Randers-Pehrson 2011-04-08 06:21:16 PDT
I've asked the PNG group for permission to push 1.4.6 out a little early (it's scheduled for April 14 but is ready to go now).
Comment 13 Glenn Randers-Pehrson 2011-04-08 06:23:32 PDT
(In reply to comment #4)
> Created attachment 509715 [details] [diff] [review]
> png_handle_fcTL() fix
> 
> Glenn, please add this fix for png_handle_fcTL() too.

@max, It's in the v00 patch.
Comment 14 Glenn Randers-Pehrson 2011-04-08 16:01:16 PDT
Created attachment 524780 [details] [diff] [review]
v01 Ugrade to libpng-1.4.6

Libpng-1.4.6 has been released.
Comment 15 Glenn Randers-Pehrson 2011-04-08 16:04:05 PDT
@Ryan, there was one bug in v00's png.h (a stray "};" )
that's fixed in the v01 patch.
Comment 16 Ryan VanderMeulen [:RyanVM] 2011-04-08 17:32:17 PDT
Bug 645519 landed on trunk, so the changes to nsPNGDecoder.cpp are already on mozilla-central. Also, apng.diff is not in the Mozilla tree, so I got rid of that as well. Pushed to try:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=8d146b477ab0

Builds will be available at the link below once finished:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/ryanvm@gmail.com-8d146b477ab0
Comment 17 Ryan VanderMeulen [:RyanVM] 2011-04-08 17:33:25 PDT
Created attachment 524786 [details] [diff] [review]
libpng 1.4.6 v1 patch

As pushed to Try
Comment 18 Glenn Randers-Pehrson 2011-04-08 17:46:40 PDT
Created attachment 524788 [details] [diff] [review]
v02 Upgrade to libpng-1.4.6

Removed stray apng.diff file and the nsPNGDecoder.cpp changes.
Comment 19 Ryan VanderMeulen [:RyanVM] 2011-04-08 17:48:27 PDT
Glenn, the diff I posted was in ready to apply hg mq format with proper attribution.
Comment 20 Glenn Randers-Pehrson 2011-04-08 17:55:08 PDT
Oops, I thought I was making my own v01 patch obsolete!

I don't see a way to un-obsolete your v1 patch and
make my v02 obsolete.  If you know how, go ahead and do that.
Comment 21 Ryan VanderMeulen [:RyanVM] 2011-04-08 17:58:25 PDT
Comment on attachment 524788 [details] [diff] [review]
v02 Upgrade to libpng-1.4.6

You have to click the Edit link on the patch details to mark them obsolete. It changed with one of the recent Bugzilla updates.
Comment 22 Glenn Randers-Pehrson 2011-04-08 18:10:07 PDT
Ok thanks.  I thought that button was just for editing the patch title.
Comment 23 Ryan VanderMeulen [:RyanVM] 2011-04-08 18:13:44 PDT
Doesn't build on Windows:
e:/builds/moz2_slave/try-w32/build/modules/libimg/png/pngwutil.c(711) : error C2143: syntax error : missing ';' before 'type'
e:/builds/moz2_slave/try-w32/build/modules/libimg/png/pngwutil.c(712) : error C2143: syntax error : missing ';' before 'type'
e:/builds/moz2_slave/try-w32/build/modules/libimg/png/pngwutil.c(713) : error C2065: 'half_z_window_size' : undeclared identifier
e:/builds/moz2_slave/try-w32/build/modules/libimg/png/pngwutil.c(713) : warning C4018: '<=' : signed/unsigned mismatch
e:/builds/moz2_slave/try-w32/build/modules/libimg/png/pngwutil.c(716) : error C2065: 'z_cinfo' : undeclared identifier
Comment 24 Ryan VanderMeulen [:RyanVM] 2011-04-08 18:27:52 PDT
I see the failure locally as well. Apparently MSVC doesn't like declaring variables inline?
http://andre.stechert.org/urwhatu/2006/01/error_c2143_syn.html
Comment 25 Glenn Randers-Pehrson 2011-04-08 18:37:45 PDT
Ack.  Fixed that in libpng-1.5.2 last week.
Comment 26 Glenn Randers-Pehrson 2011-04-08 19:37:20 PDT
The fix is being tested in libpng-1.4.7rc01.  Libpng-1.4.7 will be released once I see test results for the various platforms.
Comment 27 Ryan VanderMeulen [:RyanVM] 2011-04-09 11:35:47 PDT
Seeing this error now repeatedly:
e:\builds\moz2_slave\try-w32\build\obj-firefox\dist\include\png.h(651) : error C2144: syntax error : 'int' should be preceded by ';'
e:\builds\moz2_slave\try-w32\build\obj-firefox\dist\include\png.h(651) : error C2208: 'int' : no members defined using this type
e:\builds\moz2_slave\try-w32\build\obj-firefox\dist\include\png.h(652) : error C2144: syntax error : 'int' should be preceded by ';'
e:\builds\moz2_slave\try-w32\build\obj-firefox\dist\include\png.h(652) : error C2208: 'int' : no members defined using this type

etc...

I'm seeing the same errors locally on a clobber build. This error likely predates the 1.4.7 patch but wasn't hit because of the other build failure. It appears that pngwutil.cpp builds without error now at least.
Comment 28 Glenn Randers-Pehrson 2011-04-09 13:49:25 PDT
What compiler issued these errors?

Not sure what is happening here.  Can you try a build with
PNG_NO_PEDANTIC_WARNINGS defined in mozpngconf.h?  png.h line 651
is the first place PNG_DEPSTRUCT is used, and this will be blank with
PNG_NO_PEDANTIC_WARNINGS defined.

You can put the definition in mozpngconf.h
Comment 29 Glenn Randers-Pehrson 2011-04-09 13:55:13 PDT
This is probably relevant.  The PNG_DEPSTRUCT was blank for
all versions of MSC prior to 1.4.6.  See the libpng CHANGES
file:

version 1.4.6beta07 [March 22, 2011]
  Added attribute definition (warnings) for MSC_VER >= 1300 in pngconf.h
Comment 30 Ryan VanderMeulen [:RyanVM] 2011-04-09 14:01:25 PDT
I'm building with MSVC 2010 SP1. I'm doing a local build now with PNG_NO_PEDANTIC_WARNINGS defined in mozpngconf.h.
Comment 31 Ryan VanderMeulen [:RyanVM] 2011-04-09 15:11:01 PDT
Created attachment 524880 [details] [diff] [review]
libpng 1.4.7 v1 patch

Now we're in business. My local build succeeded with the mozpngconf.h change. Pushed to try:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=69ea57309066

Builds when ready:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/ryanvm@gmail.com-69ea57309066

Glenn, I just arbitrarily added the define near the top. Not sure if that's exactly how you'd want it for a final check-in.
Comment 32 Glenn Randers-Pehrson 2011-04-09 15:55:58 PDT
It doesn't make much difference where you put it.  I'll put it near the
top with a comment in the final version.

Do you know what MSC_VER is defined to on the compiler that emitted the
error messages?  In pngconf.h we turn on the PNG_DEPSTRUCT and other
attributes when MSC_VER >= 1300.
Comment 33 Ryan VanderMeulen [:RyanVM] 2011-04-09 15:59:38 PDT
Mozilla uses MSVC 2005 for their official builds (_MSC_VER = 1400). Plan is to upgrade to MSVC 2010 in the future (once they can sort out building jemalloc with it), which is _MSC_VER = 1600.
Comment 34 Glenn Randers-Pehrson 2011-04-09 16:18:59 PDT
Thanks.  I've passed that info along to the PNG developers.
I think we should play it safe and not use the feature for any
MSC version:

     in mozlibpng.h

     #ifdef _MSC_VER
     #  define PNG_NO_PEDANTIC_WARNINGS
     #endif

We don't need the warnings under MSC because we'll still see them while
building our same code with gcc.
Comment 35 Glenn Randers-Pehrson 2011-04-10 10:31:16 PDT
Created attachment 524971 [details] [diff] [review]
v05 Upgrade to libpng-1.4.7
Comment 36 Ryan VanderMeulen [:RyanVM] 2011-04-10 10:33:58 PDT
Created attachment 524972 [details] [diff] [review]
v5 patch update to libpng 1.4.7

Same as the other v05 patch minus the unwanted makefile changes. Functionally the same as the 1.4.7rc1 patch, which passed on Try. Built locally on a clobber build without issue.
Comment 37 Glenn Randers-Pehrson 2011-04-10 16:20:41 PDT
libpng-1.4.7 has been released.
Comment 38 Glenn Randers-Pehrson 2011-04-12 06:49:00 PDT
(In reply to comment #5 and comment #7)
> Libpng 1.5.2 has been released, can we please get it in to the trunk before
> Firefox 5.0 ships?

Libpng-1.4.7 is out and the patch is ready to go, passed Try, and is
just waiting for review. @Ryan, thanks much for helping with this.

However, if it doesn't make it into 5.0 that's not a problem.  The current
embedded libpng-1.4.3 is adequate, and neither libpng-1.4.6 nor 1.5.2
offers performance or security improvements in the parts that are
not #ifdef'ed out in mozpngconf.h
Comment 40 Max Stepin 2011-04-13 05:09:41 PDT
Glenn, the patch I suggested in comment #4 landed incorrectly.

The patch should have 3 blocks like that, but it only has 2:

-        png_crc_finish(png_ptr, length);
+        png_crc_finish(png_ptr, length-4);
         return;

I also suggested that some existing piece of code that uses info_ptr should be put inside
if (info_ptr != NULL)
{
}

but it also landed a little bit incorrectly. Part of that code ended up both inside and outside. Could you check this, please?
Comment 41 Glenn Randers-Pehrson 2011-04-13 07:58:26 PDT
(In reply to comment #40)
> Glenn, the patch I suggested in comment #4 landed incorrectly.
> ... Could you check this, please?

Yes, it's incorrect but fairly harmless.  The wrong length on
png_crc_finish() will cause a png_error when libpng attempts
to read the next chunk.  The extra code inside the if{} is
harmless except for the increase in the size of the executable.

I'll try to generate a new patch today (against the current trunk
which seems to be still at libpng-1.4.3) but I think it has missed
the Firefox-5.0 cutoff.
Comment 42 Glenn Randers-Pehrson 2011-04-13 08:24:32 PDT
Created attachment 525679 [details] [diff] [review]
v06 Upgrade to libpng-1.4.7

Revised per Max's comment.  Patch is against trunk (assumes v5 has not
been applied).
Comment 44 :Ehsan Akhgari 2011-04-13 11:18:29 PDT
(In reply to comment #42)
> Revised per Max's comment.  Patch is against trunk (assumes v5 has not
> been applied).

Can you please attach a patch on top of the current trunk which addresses comment 41?
Comment 45 Glenn Randers-Pehrson 2011-04-13 11:42:37 PDT
@ehsan, hopefully this evening (EST), don't have access to my files and tools right now.
Comment 46 Glenn Randers-Pehrson 2011-04-13 12:07:57 PDT
While making a new patch I will also fix the fact that, when info_ptr is
NULL, max's (and the v06) patch still returns success (sets a mode bit).
It shouldn't set the mode bit in this case.  This error does not affect
Firefox, though.  The fix is easy, just move the mode-setting line inside
the if{}.
Comment 47 :Ehsan Akhgari 2011-04-13 13:53:12 PDT
Is this patch as landed safe to get on a nightly?  If not, I can just back it out and reland the fixed version...
Comment 48 Glenn Randers-Pehrson 2011-04-13 13:56:31 PDT
Created attachment 525794 [details] [diff] [review]
v07 Fix coding errors in png_handle_fcTL

See comment #40 and comment #46
This patch assumes the v5 patch has already been applied.
Comment 49 Glenn Randers-Pehrson 2011-04-13 13:58:45 PDT
(In reply to comment #47)
> Is this patch as landed safe to get on a nightly?  If not, I can just back it
> out and reland the fixed version...

That's OK.  I made a fresh clone and built the small v07 patch against that.
Comment 50 Ryan VanderMeulen [:RyanVM] 2011-04-13 15:51:39 PDT
v07 builds fine locally. Running through Try:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=bf80756497cb
Comment 51 Glenn Randers-Pehrson 2011-07-25 04:18:17 PDT
Both the v5 and v07 patches have been checked in to mozilla-central a while ago.
Comment 52 Glenn Randers-Pehrson 2011-07-27 04:49:34 PDT
(In reply to comment #51)
> Both the v5 and v07 patches have been checked in to mozilla-central a while
> ago.

Correction: only the v5 patch was checked in.  Reopening.
Comment 53 Glenn Randers-Pehrson 2011-07-27 05:07:35 PDT
checkin-needed (of the v07 patch only)
Comment 54 Boris Zbarsky [:bz] 2011-07-29 11:31:51 PDT
Pushed v07: http://hg.mozilla.org/integration/mozilla-inbound/rev/88329c72d50c
Comment 55 Marco Bonardo [::mak] 2011-08-01 07:50:05 PDT
http://hg.mozilla.org/mozilla-central/rev/88329c72d50c

Please check status of the bug, esp target milestone and such, since it's a bit unclear to me.

Note You need to log in before you can comment on or make changes to this bug.