Closed Bug 573137 Opened 9 years ago Closed 9 years ago

Upgrade to zlib 1.2.5

Categories

(Core :: ImageLib, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- final+

People

(Reporter: djeter, Assigned: djeter)

References

()

Details

Attachments

(5 files, 8 obsolete files)

4.08 KB, patch
Details | Diff | Splinter Review
2.55 KB, patch
benjamin
: review+
benjamin
: approval2.0+
Details | Diff | Splinter Review
403.73 KB, patch
joe
: review+
Details | Diff | Splinter Review
4.99 KB, text/plain
Details
4.08 KB, patch
Details | Diff | Splinter Review
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
Attached patch zlib_patch.diff (obsolete) — Splinter Review
Patch to upgrade modules/zlib to version 1.2.5
Attachment #452348 - Flags: review?
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
It doesn't resolve bug #283214 but it would allow us to create a libpr0n patch for it.
Attached patch Updated zlib_patch.diff (obsolete) — Splinter Review
Fixed a nit: forgot the mozzconf.h include in zconf.h
Attachment #452348 - Attachment is obsolete: true
Attachment #452348 - Flags: review?
Attached patch zlib_patch.diff, v3 (obsolete) — Splinter Review
Fixed a nit: Use correct Makefile.in
Attachment #452422 - Attachment is obsolete: true
Attached patch zlib_patch.diff, v4 (obsolete) — Splinter Review
Last patch forgot to add some files, this patch includes those files.
Attachment #452465 - Attachment is obsolete: true
Comment on attachment 452490 [details] [diff] [review]
zlib_patch.diff, v4

Technically, I'm the module owner of this code.
Attachment #452490 - Flags: review?(joe)
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.
Attachment #452490 - Flags: review?(joe) → review+
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.
Attached patch zlib_patch.diff, v4 (nits fixed) (obsolete) — Splinter Review
This patch addresses the nits in comment 8 and removes the 5 symbols that caused the non-static builds to fail.
Blocks: 581181
Marking blocking 2.0? because this blocks bug 581181.
blocking2.0: --- → ?
Assignee: nobody → djeter
blocking2.0: ? → final+
Blocks: 407860
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
Attached patch zlib_patch.diff, v5 (obsolete) — Splinter Review
This patch addresses the nits in the comment above.
Attachment #456144 - Attachment is obsolete: true
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.
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.
I've only tested the patch on Windows.
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.
I recreated a patch that summarizes Mozilla's local changes to
zlib 1.2.3.
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.
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.
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.
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.
Assuming the OS2 code doesn't get enough attention,
this patch removes two pieces of code that cancel
each other.
Attachment #471006 - Flags: review?(benjamin)
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.
Attachment #471011 - Flags: review?(benjamin)
Attachment #471011 - Flags: approval2.0?
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 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
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.
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.
Attachment #452490 - Attachment is obsolete: true
Attachment #459985 - Attachment is obsolete: true
Attachment #471149 - Flags: review?(joe)
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.
Attachment #471011 - Flags: review?(benjamin)
Attachment #471011 - Flags: review+
Attachment #471011 - Flags: approval2.0?
Attachment #471011 - Flags: approval2.0+
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?
Attachment #471006 - Flags: review?(benjamin)
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.
Attachment #471149 - Flags: review?(joe) → review+
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
Attachment #471011 - Attachment description: Prerequisite patch: remove -DZLIB_INTERNAL → Prerequisite patch: remove -DZLIB_INTERNAL (pushed to mozilla-central)
(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.
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?
Attachment #471006 - Attachment is obsolete: true
Attachment #471895 - Flags: review?(benjamin)
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 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.
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.
Attachment #471149 - Flags: approval2.0?
Attachment #471149 - Flags: approval2.0? → approval2.0+
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]
Attachment #471149 - Attachment description: zlib_patch.diff, v6 by Daniel Jeter II → zlib_patch.diff, v6 by Daniel Jeter II (pushed to mozilla-central)
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
Attachment #471895 - Attachment is obsolete: true
Attachment #471895 - Flags: review?(benjamin)
Marked the bug fixed.  I will deal with -DZLIB_DLL in
a new bug report after Firefox 4.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0
Target Milestone: mozilla2.0 → mozilla2.0b7
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)?
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.
You need to log in before you can comment on or make changes to this bug.