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

RESOLVED FIXED in mozilla0.9.3

Status

SeaMonkey
Build Config
P3
normal
RESOLVED FIXED
17 years ago
13 years ago

People

(Reporter: edxu@hotmail.com, Assigned: Daniel (Leaf) Nunes)

Tracking

Trunk
mozilla0.9.3
x86
Windows NT

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [needed for embedding])

Attachments

(8 attachments)

(Reporter)

Description

17 years ago
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.
(Reporter)

Comment 1

17 years ago
Created attachment 38860 [details]
Patch from Todd Brannum
(Reporter)

Comment 2

17 years ago
Patch attached, save it as .zip file. It has new files and diff.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

17 years ago
Assignee: adamlock → leaf
Component: Embedding: Docshell → Build Config

Comment 3

17 years ago
-> Leaf

Comment 4

17 years ago
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.

Updated

17 years ago
Whiteboard: [needed for embedding]
Target Milestone: --- → mozilla0.9.2
(Assignee)

Comment 5

17 years ago
reviewing/building now.
Status: NEW → ASSIGNED
(Assignee)

Comment 6

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

Updated

17 years ago
Priority: -- → P2
(Assignee)

Comment 7

17 years ago
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

17 years ago
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.
(Reporter)

Comment 9

17 years ago
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

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

Updated

17 years ago
Summary: mozilla using none standard zlib.dll conflicts with third party product in Windows platform → mozilla using non-standard zlib.dll conflicts with third party product in Windows platform

Comment 11

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
Maybe 30 minutes of someone's time.

Comment 15

17 years ago
Created attachment 39243 [details]
zip of modules/zlib updated to zlib 1.1.3

Comment 16

17 years ago
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?

(Assignee)

Comment 17

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

Comment 18

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

Comment 19

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

Comment 20

17 years ago
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

17 years ago
Created attachment 39279 [details]
zlib.dnt from zlib-1.1.3 dist

Comment 22

17 years ago
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

17 years ago
Created attachment 39297 [details]
zip of *.[ch] README ChangeLog zlib.def from 1.1.3

Comment 24

17 years ago
Created attachment 39298 [details] [diff] [review]
needed changes to existing non-zlib files

Comment 25

17 years ago
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.
(Assignee)

Comment 26

17 years ago
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

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

Comment 28

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

Comment 29

17 years ago
Created attachment 39776 [details]
zlibProject.zip, contains zlib.mcp that uses zlip.exp (removed char!)
(Assignee)

Comment 30

17 years ago
<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

17 years ago
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

17 years ago
a=chofmann branch and trunk when ready
(Assignee)

Comment 33

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

Comment 34

17 years ago
pdt+ per chofmann (pdt2@netscape.com)
Whiteboard: [needed for embedding] → [PDT+][needed for embedding]
(Assignee)

Comment 35

17 years ago
fixed on the branch, will merge to trunk shortly, clearing from 0.9.2 milestone 
radar.
Target Milestone: mozilla0.9.2 → mozilla0.9.3

Comment 36

17 years ago
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?
(Assignee)

Comment 37

17 years ago
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.
(Assignee)

Comment 38

17 years ago
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.
(Assignee)

Comment 39

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

Comment 40

17 years ago
reducing severity and removing PDT+ since this has been fixed on the 0.9.2 branch.
Severity: blocker → normal
Priority: P2 → P3
Whiteboard: [PDT+][needed for embedding] → [needed for embedding]

Comment 41

17 years ago
Created attachment 41776 [details] [diff] [review]
Patch for OpenVMS

Comment 42

17 years ago
Looking for approval to checkin above OpenVMS patch.
(Assignee)

Comment 43

17 years ago
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

17 years ago
Created attachment 41785 [details] [diff] [review]
Revised patch for OpenVMS

Comment 45

17 years ago
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

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

Comment 47

17 years ago
marking fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Product: Browser → Seamonkey
Attachment #38860 - Attachment description: Patch from Todd Brannum: the changes made are primarily in the "makefile.win" to set the correct flags for zlib exports - it's important to use the correct calling convention otherwise the 'return' call in a zlib function could blow up with a stack proble → Patch from Todd Brannum
You need to log in before you can comment on or make changes to this bug.