86323 - mozilla using non-standard zlib.dll conflicts with third party product in Windows platform

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect, P3)

Description

[[e-mail]]

On Windows platform mozilla/modules/zlib is compiled with __cdecl for exported 
functions instead of WINAPI or __stdcall as expected in standard zlib.dll. This 
cause conflicts with third party product such as plug-ins or gecko embedding 
application that use standard zlib.dll because the non-standard one is loaded 
in address space and hence the crash.

Compiling with ZLIB_DLL and _WINDOWS will not solve problem because mozilla 
hard-code PR_API types in the zlib source files. As a matter of fact mozilla 
MUST uses standard zlib so that third party products will not complain.

Comment 1

[[e-mail]]

Attachment: Patch from Todd Brannum

Comment 2

[[e-mail]]

Patch attached, save it as .zip file. It has new files and diff.

Comment 3

[Adam Lock]

-> Leaf

Comment 4

[chris hofmann]

leaf/seawood,  can you do the r= and sr= on this?

until we get this in its causing a big embedding customer a lot of headaches...
if this patch looks good it would be good to get it in today.

Comment 5

[Daniel (Leaf) Nunes]

reviewing/building now.

Comment 6

[Daniel (Leaf) Nunes]

can you repost the diff as a text file, rather than application/octet-stream, 
please?

Comment 7

[Daniel (Leaf) Nunes]

alright, it's a zip file; patch is barfing for me on a simple diff, so it could 
be until tomorrow night (when i predict to have another windows machine up and 
building) before i get a windows build with this patch.

What i'm concerned with more is the effect on the unix platforms, which i bet 
hasn't been tested yet. Someone feel free to correct me before i waste time 
testing it.

Comment 8

[cls]

Ugh. So rather than taking these patches, why don't we just import a copy of the
the latest release of zlib or the original release that was checked in?  I don't
know about leaf but outside of the PR_API changes, I have no clue what this
relatively huge (184k) patch is doing.

Comment 9

[[e-mail]]

Turn out the gecko version of zlib is two years older than the current version 
1.1.3 (which Todd patched from).  Version 1.1.3 is the current version and has 
been out for several years now.

If we simply grab the source code of 1.1.3 and drop it into mozilla (and define 
ZLIB_DLL for Windows), will it work for Unix without using prtypes? If it will 
then I will go with Christopher on this and not use the patch, which *might* 
not work for UNIX due to the same problem.

As a temparory solution I am using the pre-built version of zlib.dll/zlib.lib 
for Gecko, of course this will only work for Windows.

Comment 10

[cls]

On unix, by default, we use the system installed version of zlib and it appears
to work fine. 

Comment 11

[cls]

Does anyone know if the mac requires us to use the prtypes or can we just plop
in the latest zlib release without mods?

Comment 12

[Simon Fraser [no longer active]]

Mac requires some PR_ macro stuff in zlib to manage symbol exports. If we had an 
alternative way to control exports (e.g. .exp file), then this could go away.

Comment 13

[cls]

Ok, I got the zlib 1.1.3 tarball (extracted from a srpm since the main zlib site
is down).  Dropping the source files into the tree works for the linux build. We
already compile with -D_WINDOWS (set in config/WIN32) but I do see the mention
of needing -DZLIB_DLL for each project that uses zlib (ugh).  

http://www.winimage.com/zLibDll/build.html

Simon, how much work would it be to provide the alternative method for the mac?

Comment 14

[Simon Fraser [no longer active]]

Maybe 30 minutes of someone's time.

Comment 15

[cls]

Attachment: zip of modules/zlib updated to zlib 1.1.3

Comment 16

[cls]

You _should_ be able to move modules/zlib out of the way and unzip the attached
zip file in modules.  It uses pristine zlib 1.1.3 src with the mozilla build
files with one exception.  I modified zconf.h to automatically define ZLIB_DLL
if MOZILLA_CLIENT & XP_WIN32 are set.  

The zip compiles on windows but doesn't create a .lib library even though
nothing in the win32 build files have changed.  Help!

Simon, I created a zlib.exp file under macbuild using the jpeg one as a
template.  Any chance you could modify the project file to use it?

Comment 17

[Daniel (Leaf) Nunes]

i'll track down where the .lib went to, chris; are you, by any chance, building 
with the static build branch?

Comment 18

[cls]

I've checked about 20 times to make sure but the static build stuff is in a
separate tree. :)

Comment 19

[Daniel (Leaf) Nunes]

fair enough, i'll keep looking for other reasons why this might not be working 
:)

Comment 20

[cls]

Interesting but disturbing factiod: 
When using the current cvs src, if I ifdef out the prtypes.h include from
zconf.h & zutil.c & use the non-MOZILLA_CLIENT define of PR_PUBLIC_API, I see
the same missing .lib problem.  Does win32 have some link-time script that looks
for specific PR_PUBLIC_API signatures or something?

Comment 21

[cls]

Attachment: zlib.dnt from zlib-1.1.3 dist

Comment 22

[cls]

Copying the zlib.dnt from the nt/ subdir of the zlib-1.1.3 dist into zlib/src
and adding DEFFILE=zlib.dnt to zlib/src/makefile.win brings back the .lib .  My
build got past necko so I'm guessing that it works.  I'll let you know if the
build actually runs.

Comment 23

[cls]

Attachment: zip of *.[ch] README ChangeLog zlib.def from 1.1.3

Comment 24

[cls]

Attachment: needed changes to existing non-zlib files

Comment 25

[cls]

So, we're back to pristine 1.1.3 src files.  The latest zip (attach 39297)
should be unzipped in a current cvs pull of modules/zlib/src .  Then apply the
patch to make the rest of the tree build.  I feel so dirty.  I need a shower.

Comment 26

[Daniel (Leaf) Nunes]

yeah, there are magic things done with linker scripts and .def files, but i 
didn't know it was based on .h preprocessing *sigh*

I'll verify the build and we can probably get this carpooled with dougt 
tomorrow, assuming we get an a= ;)

Comment 27

[cls]

We still need some mac lovin' and a project file that knows to use
macbuild/zlib.exp .

Comment 28

[Jon Granrose]

ccing jj so he's in the loop on the mac stuff.

Comment 29

[Patrick C. Beard]

Attachment: zlibProject.zip, contains zlib.mcp that uses zlip.exp (removed char!)

Comment 30

[Daniel (Leaf) Nunes]

<fume>my system's "patch" has decided that it needs to crash in the middle of 
applying any diff</fume>. Attempting to test on the verification machine.

Comment 31

[Syd Logan]

Tried on the mac, and exercised the CRC checking code that I just wrote which 
uses zlib. It worked. Not an extensive test, but it did build, and didn't break 
me (two typically rare events :-)

Comment 32

[chris hofmann]

a=chofmann branch and trunk when ready

Comment 33

[Daniel (Leaf) Nunes]

checked into the branch; spinning a verification build. once verified, i'll 
commit on the trunk

Comment 34

[Jon Granrose]

pdt+ per chofmann ([e-mail])

Comment 35

[Daniel (Leaf) Nunes]

fixed on the branch, will merge to trunk shortly, clearing from 0.9.2 milestone 
radar.

Comment 36

[Colin Blake]

This change broke the VMS build. The proto of strerror in zutil.h used to have
an #ifndef MOZILLA_CLIENT around it, now it doesn't. Why are we even trying to
prototype strerror when the correct thing to do is #include <string.h>???

For now I've fixed it in my tree, but I need to know what flavor of patch you
want back. Do you want #ifndef MOZILLA_CLIENT or #ifndef VMS around the
strerror proto?

Comment 37

[Daniel (Leaf) Nunes]

I think we want VMS, since this didn't seem to cause problems for the platforms 
that embedding is on. I'm not sure the reasoning for zlib's prototyping of 
strerror, but i'm not willing to futz with how zlib is built on the platforms 
where embedding was having an issue.

Comment 38

[Daniel (Leaf) Nunes]

colin, i'll be around for a while today (i'm merging this to the trunk), but if
i don't get to review a patch today, i'll be around monday to do so. Anyone
cc'd, i think, can give you an r= to checkin to either the 0.9.2 branch or the
trunk.

Comment 39

[Daniel (Leaf) Nunes]

ok, merged to trunk; i'll leave this open until we get vms building again.

Comment 40

[Jon Granrose]

reducing severity and removing PDT+ since this has been fixed on the 0.9.2 branch.

Comment 41

[Colin Blake]

Attachment: Patch for OpenVMS

Comment 42

[Colin Blake]

Looking for approval to checkin above OpenVMS patch.

Comment 43

[Daniel (Leaf) Nunes]

can you also include another comment line under the comment line starting with:
/* @(#) $Id: zutil.h,v

saying

/* This file has been modified from the original distribution */
otherwise, sr=leaf for the trunk

Comment 44

[Colin Blake]

Attachment: Revised patch for OpenVMS

Comment 45

[Colin Blake]

Added the comment, as requested by Leaf.

Is the trunk under driver control at the moment, or am I all clear to check in
once it opens?

Comment 46

[Colin Blake]

The patch for OpenVMS (attachment id=41785) has been checked in.

Comment 47

[cls]

marking fixed.