573137 - Upgrade to zlib 1.2.5

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, enhancement)

Description

[Daniel Jeter II]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.3a6pre) Gecko/20100617 SeaMonkey/2.1a2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.3a6pre) Gecko/20100617 SeaMonkey/2.1a2pre

zlib-1.2.5 has been released. These changes should be picked up.

Reproducible: Always

Comment 1

[Daniel Jeter II]

Attachment: zlib_patch.diff

Patch to upgrade modules/zlib to version 1.2.5

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Glenn, does this resolve 283214?

Also, not sure that Imagelib is really the right component for this. Also not sure who should be reviewing this. Maybe biesi?

Comment 3

[Glenn Randers-Pehrson]

It doesn't resolve bug #283214 but it would allow us to create a libpr0n patch for it.

Comment 4

[Daniel Jeter II]

Attachment: Updated zlib_patch.diff

Fixed a nit: forgot the mozzconf.h include in zconf.h

Comment 5

[Daniel Jeter II]

Attachment: zlib_patch.diff, v3

Fixed a nit: Use correct Makefile.in

Comment 6

[Daniel Jeter II]

Attachment: zlib_patch.diff, v4

Last patch forgot to add some files, this patch includes those files.

Comment 7

[Joe Drew (not getting mail)]

Comment on attachment 452490 [details] [diff] [review]
zlib_patch.diff, v4

Technically, I'm the module owner of this code.

Comment 8

[Joe Drew (not getting mail)]

Comment on attachment 452490 [details] [diff] [review]
zlib_patch.diff, v4


>diff -u -N -r a/mozilla/modules/zlib/src/ChangeLog.moz b/mozilla/modules/zlib/src/ChangeLog.moz
>--- a/mozilla/modules/zlib/src/ChangeLog.moz	2010-04-09 22:49:19 -0500
>+++ b/mozilla/modules/zlib/src/ChangeLog.moz	2010-06-17 17:01:37 -0500
>@@ -42,3 +42,8 @@
> 
> - 13 September 2009
>   Don't enable zlib's debug output when the Mozilla build is in debug mode (bug 431950)
>+
>+- 17 June 2010
>+  Sync'ed with 1.2.5 release
>+  (keeping '#include "mozzconf.h"' in zconf.h)
>+  See bugs #299445 and #300349
>\ No newline at end of file

"mozconf.h", and add a newline.

This is otherwise a rubber-stamp r+.

This should go through a run through the tryserver to make sure it doesn't introduce new bugs.

Comment 9

[Daniel Jeter II]

Non-static builds appear to be busted with 5 unresolved externals in zlib.def... The 5 symbols were added in zlib-1.2.4, so I'll run through a couple builds with those symbols commented out and see if that was the problem. I should have a fixed patch soon.

Comment 10

[Daniel Jeter II]

Attachment: zlib_patch.diff, v4 (nits fixed)

This patch addresses the nits in comment 8 and removes the 5 symbols that caused the non-static builds to fail.

Comment 11

[Justin Lebar (not reading bugmail)]

Marking blocking 2.0? because this blocks bug 581181.

Comment 12

[Glenn Randers-Pehrson]

In mozzconf.h the comment

    /* New as of libpng-1.2.4 */

should be

    /* New as of zlib-1.2.4 */

Re: comment #8, I think "mozzconf.h" was correct
in ChangeLog.moz

Comment 13

[Daniel Jeter II]

Attachment: zlib_patch.diff, v5

This patch addresses the nits in the comment above.

Comment 14

[Wan-Teh Chang]

Some Mozilla makefiles define -DZLIB_INTERNAL:
http://mxr.mozilla.org/mozilla-central/search?string=-DZLIB_INTERNAL

-DZLIB_INTERNAL defines the ZLIB_INTERNAL macro as 1.

In zlib 1.2.5, the value of ZLIB_INTERNAL may be defined
as __attribute__((visibility ("hidden"))) for GCC 3.3 or
later.  See gzguts.h and zutil.h.  Please watch out for
this change.

Daniel (djeter), have you tested your patch on Linux or
Mac OS X?

I think Mozilla makefiles should not define -DZLIB_INTERNAL.

Comment 15

[Wan-Teh Chang]

Re: the ZLIB_DEBUG change to zutil.h:
I described an alternative solution in bug 431950 comment 7
that doesn't require patching zutil.h.

Comment 16

[Daniel Jeter II]

I've only tested the patch on Windows.

Comment 17

[Wan-Teh Chang]

Thanks for the reply.  That makes sense, as I expect
that we will need to deal with the -DZLIB_INTERNAL issue
for platforms that use GCC, such as Linux and Mac OS X.

Comment 18

[Wan-Teh Chang]

Attachment: Mozilla's patch for zlib 1.2.3 (for reference only)

I recreated a patch that summarizes Mozilla's local changes to
zlib 1.2.3.

Comment 19

[Daniel Jeter II]

Over the weekend, I set up a VM to test the patch on Linux. So far, I have made compiles pre-patch and post-patch with GCC versions 4.3.5, 4.4.4 and 4.5.1... All of the builds completed and so far have run successfully for me. I don't have a way to test the patch on Mac OS X, though I can set up more Linux VM's that use older versions of GCC and test the patch there to see if any problems come up.

Comment 20

[Wan-Teh Chang]

joedrew: I just updated the zlib in NSS to zlib 1.2.5, so I can review
and check in djeter's patch.

Two questions for you:
- algorithms.txt became a doc/ directory with four files.  Do you want
  * just doc/algorihtms.txt,
  * all four files in doc/, or
  * none?
- zlib 1.2.5 added zlib.3.pdf, which is the PDF version of zlib.3.  I assume
  you don't want it.  (I actually think we don't need zlib.3 either.)

djeter: thank you for your work on this bug.

Since this kind of patch can't be reviewed by inspection, I duplicated
your work and compared the results last weekend.  The differences
are minor.  I'll describe them later.

The main issue is the -DZLIB_INTERNAL problem I described in comment 14.
Although your test build on Linux worked, there are compiler warnings like
this:

adler32.cgcc -o adler32.o -c -I../../../dist/system_wrappers -include /usr/local/google/home/wtc/mozilla-central.djeter/src/config/gcc_hidden.h -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -DZLIB_INTERNAL -DOSTYPE=\"Linux2.6\" -DOSARCH=Linux  -I/usr/local/google/home/wtc/mozilla-central.djeter/src/modules/zlib/src -I. -I../../../dist/include -I../../../dist/include/nsprpub  -I/usr/local/google/home/wtc/mozilla-central.djeter/ff-dbg/dist/include/nspr -I/usr/local/google/home/wtc/mozilla-central.djeter/ff-dbg/dist/include/nss       -fPIC  -Wall -W -Wno-unused -Wpointer-arith -Wcast-align -W -pedantic -Wno-long-long -fno-strict-aliasing -pthread -pipe  -DDEBUG -D_DEBUG -DTRACING -g   -include ../../../mozilla-config.h -DMOZILLA_CLIENT -MD -MF .deps/adler32.pp /usr/local/google/home/wtc/mozilla-central.djeter/src/modules/zlib/src/adler32.c
In file included from /usr/local/google/home/wtc/mozilla-central.djeter/src/modules/zlib/src/adler32.c:8:/usr/local/google/home/wtc/mozilla-central.djeter/src/modules/zlib/src/zutil.h:17:1: warning: "ZLIB_INTERNAL" redefined

The fix is to remove -DZLIB_INTERNAL from Mozilla makefiles.

Comment 21

[Joe Drew (not getting mail)]

wtc: 

I actually am not choosy about what you include or don't include. If I was going to be choosy, though, I might drop all the docs, including the man page and pdf.

Comment 22

[Wan-Teh Chang]

joedrew: I agree.  I'll drop all the docs.

I am researching -DZLIB_INTERNAL now.  I've tracked it down to
bug 272783 and bug 273876.  I hope bsmedberg still remembers
it as he added it six years ago.

Comment 23

[Wan-Teh Chang]

Attachment: Optional patch: remove code that defines ZLIB_DLL and then undefines it

Assuming the OS2 code doesn't get enough attention,
this patch removes two pieces of code that cancel
each other.

Comment 24

[Wan-Teh Chang]

Attachment: Prerequisite patch: remove -DZLIB_INTERNAL (pushed to mozilla-central)

Ben: this patch reverts some of your zlib-related changes in
Dec 2004 for libxul (bug 272783 and bug 273876).

ZLIB_INTERNAL should only be defined when compiling the files
inside zlib.  Files that use zlib should not be compiled with
-DZLIB_INTERNAL.

With zlib 1.2.5, -DZLIB_INTERNAL causes compiler warnings on
Linux (shown at the end of comment 20) because zlib 1.2.5
(zutil.h) defines ZLIB_INTERNAL as

#if ((__GNUC__-0) * 10 + __GNUC_MINOR__-0 >= 33) && !defined(NO_VIZ)
#  define ZLIB_INTERNAL __attribute__((visibility ("hidden")))
#else
#  define ZLIB_INTERNAL
#endif

This patch is a prerequisite for the zlib 1.2.5 update, which
has blocking2.0=final+ status.  Therefore I'm requesting
approval2.0.

Comment 25

[Wan-Teh Chang]

In comment 20 I truncated the compiler warning.  The complete warning
has two lines:

/usr/local/google/home/wtc/mozilla-central.djeter/src/modules/zlib/src/zutil.h:17:1: warning: "ZLIB_INTERNAL" redefined
<command-line>: warning: this is the location of the previous definition

Comment 26

[Benjamin Smedberg]

Comment on attachment 471006 [details] [diff] [review]
Optional patch: remove code that defines ZLIB_DLL and then undefines it

http://mxr.mozilla.org/mozilla-central/search?string=zlib_dll lists several other places where we define ZLIB_DLL and presumably shouldn't any more, right?

Previously ZLIB_INTERNAL was needed to control dllimport/dllexport definitions, so if we're not building ZLIB_DLL in any configuration, we don't need the ZLIB_INTERNAL stuff either.

But it's not clear to me how this affects the non-libxul configuration, where I thought we *were* still building a separate mozzlib.dll

Comment 27

[Wan-Teh Chang]

Ben:

Thank you for your comments.

I did a lot of MXR searches to research -DZLIB_DLL and
-DZLIB_INTERNAL.  I'm afraid that I may have missed
something, but I think the following is what led to the
current mess.

1. Several Mozilla makefiles defines ZLIB_DLL on Windows
for all build configurations.  For example,
netwerk/streamconv/converters/Makefile.in has:
ifeq ($(OS_ARCH),WINNT)
DEFINES         += -DZLIB_DLL
endif

2. The ZLIB_DLL definitions in these makefiles weren't
updated when the libxul configuration was added.  I think
only xpinstall/src/Makefile.in got it right:
ifeq ($(OS_ARCH)$(MOZ_ENABLE_LIBXUL),WINNT)
DEFINES         += -DZLIB_DLL
endif

3. As a result, we added -DZLIB_INTERNAL for the libxul
configuration to suppress MSVC linker warnings caused
by dllimport.

If my reconstruction of past events is correct, I think
the correct fix is to define -DZLIB_DLL only for the
non-libxul configuration, and remove -DZLIB_INTERNAL.

Note: did xul.dll ever export the zlib functions as part
of the libxul API?  That would be a reason to compile all
libxul files as zlib-internal.

Comment 28

[Wan-Teh Chang]

Attachment: zlib_patch.diff, v6 by Daniel Jeter II (pushed to mozilla-central)

Here is my edited version of djeter's patch.  I made
the following changes.

1. Nit: remove the blank line at the end of ChangeLog.moz.
Joe, I expect that you will update ChangeLog.moz when you
commit this patch.

2. mozzconf.h: rename more new functions of zlib 1.2.4.
I discovered them by inspecting zconf.h.

3. zconf.h: resurrect Mozilla's local change for __OS2__.

4. zlib.def: I generated this with this sed command:
       sed 's/^    /    MOZ_Z_/g' zlib-1.2.5/win32/zlib.def

5. zutil.h: I applied Mozilla's local changes using the
'patch' command.  djeter must have applied the changes
manually so that there were a TAB character and Windows
line endings (CRLF).

Joe, this patch is equivalent to my review+ on djeter's
patch.  I will attach a "patch interdiff" patch to help
you review this patch.

Comment 29

[Wan-Teh Chang]

Attachment: Patch interdiff between zlib_patch.diff v5 and v6

Comment 30

[Benjamin Smedberg]

wtc, I believe your analysis is correct. We may have exported the zlib symbols from libxul at some point, but we don't now and shouldn't.

Comment 31

[Benjamin Smedberg]

Comment on attachment 471006 [details] [diff] [review]
Optional patch: remove code that defines ZLIB_DLL and then undefines it

I'm less-sure about this one, can you re-request review if a windows non-libxul build works?

Comment 32

[Joe Drew (not getting mail)]

Comment on attachment 471149 [details] [diff] [review]
zlib_patch.diff, v6 by Daniel Jeter II (pushed to mozilla-central)

Wan-Teh, it's not clear to me what you want me to do with Changelog.moz?

Everything else looks OK though.

Comment 33

[Wan-Teh Chang]

Comment on attachment 471011 [details] [diff] [review]
Prerequisite patch: remove -DZLIB_INTERNAL (pushed to mozilla-central)

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/983c38bb0f03

Comment 34

[Wan-Teh Chang]

(In reply to comment #32)
Joe: please replace the ChangeLog.moz entry when you commit
the patch.  At least the date needs to be updated.

Comment 35

[Wan-Teh Chang]

Attachment: Optional patch: define ZLIB_DLL only when we're building or using zlib as a DLL

Ben, please review this patch.

This patch assumes that we're building zlib as a DLL
only if neither BUILD_STATIC_LIBS (--enable-static)
nor MOZ_ENABLE_LIBXUL (--enable-libxul) is set.  Is
this correct?

This patch moves the common code into config/config.mk.
Makefiles that need to define ZLIB_DLL just need to set
the make variable INCLUDE_ZLIB_H to 1.  (Is the variable
name OK?)

This approach is difficult to maintain because failure
to define ZLIB_DLL is just a tiny performance penalty.
I used MXR to find some other makefiles that should also
define ZLIB_DLL, but it was a painful process.

So I'm wondering if we should instead define ZLIB_DLL in
mozzconf.h as follows:

#if defined(WIN32) || defined(OS2)
#if !defined(MOZ_STATIC_BUILD) && !defined(MOZ_ENABLE_LIBXUL)
#define ZLIB_DLL 1
#endif
#endif

Would you prefer this alternative approach?

Comment 36

[Benjamin Smedberg]

Comment on attachment 471895 [details] [diff] [review]
Optional patch: define ZLIB_DLL only when we're building or using zlib as a DLL

I'd just skip the INCLUDE_ZLIB_H bit... can't we can just do this globally?

Comment 37

[Wan-Teh Chang]

Comment on attachment 471895 [details] [diff] [review]
Optional patch: define ZLIB_DLL only when we're building or using zlib as a DLL

Ben: we can define ZLIB_DLL globally.  We can also just
give up the tiny performance gain and not worry about
ZLIB_DLL.  One reason I'm proposing the latter is that I
don't fully understand all the possible build configurations.

Do you know what modules/zlib/standalone/Makefile.in is
used for?  In the two build configurations I tested
(libxul and non-libxul), modules/zlib/standalone/Makefile.in
builds a static library mozz_s.lib, but the static library
is not being used.

Comment 38

[Benjamin Smedberg]

Hrm, I thought standalone got removed a long time ago: IIRC it was only used by the ancient xpinstall code which we no longer build.

Comment 39

[Wan-Teh Chang]

Comment on attachment 471149 [details] [diff] [review]
zlib_patch.diff, v6 by Daniel Jeter II (pushed to mozilla-central)

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/fb3a132de4ec
http://hg.mozilla.org/mozilla-central/rev/2e571d635a60

The second changeset reverted one of our changes to zutil.h
that defines HAVE_VSNPRINTF for recent versions of MSVC,
because that issue has been addressed (in a different way),
described by this entry in ChangeLog:

Changes in 1.2.3.5 (8 Jan 2010)
...
- Don't use _vsnprintf on later versions of MSVC [Lowman]

Comment 40

[Wan-Teh Chang]

Attachment: Mozilla's patch for zlib 1.2.5 (for reference only)

This patch is for future reference.  I generated our zlib.def
using this sed command:
    sed 's/^    /    MOZ_Z_/g' zlib-1.2.5/win32/zlib.def

Comment 41

[Wan-Teh Chang]

Marked the bug fixed.  I will deal with -DZLIB_DLL in
a new bug report after Firefox 4.

Comment 42

[Glenn Randers-Pehrson]

About dropping the docs, should we also drop docs from the next libpng update (especially CHANGES which is always a major part of the patch)?

Comment 43

[Wan-Teh Chang]

I should have noted in comment 39 that I removed algorithm.txt
and zlib.3 before pushing the zlib_patch.diff, v6 patch:
http://hg.mozilla.org/mozilla-central/rev/6066d938a0dd

I think ChangeLog, FAQ, and INDEX can also be removed.