243079 - Crash on startup of debug build (from cvs) (in nsStandardURL - mixed allocators)

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040113
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040113

On startup, I get an assertion (block is not valid). The stacktrace points to a
line in nsStandardURL (line 1668). I'll attach a patch that seems to fix this
problem for me.

Reproducible: Always
Steps to Reproduce:
1. Start a debug build of mozilla.
Actual Results:  
crash

Expected Results:  
starts up.

Comment 1

[Justin Wood (:Callek)]

as a side-note, crashes for me too, win32, debug build, 1.8a pull...

Blame?  (pulled somepoint before midnight EST friday night, though I waited for
a new pull/build before talking to blake about it, who already had figured out
most of the issues)

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: fixes this crash for me

This changes a call of |free| to |nsCRT::free|. *result was set earlier with
|nsCRT::strdup|.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 148030 [details] [diff] [review]
fixes this crash for me

darin, could you review this (and check in if it's good)?

Comment 4

[Andrew Schultz]

==> networking

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 148030 [details] [diff] [review]
fixes this crash for me

On re-examining the file, I'm not sure if this is correct anymore. I didn't see
that |*result| is also set by a call to |AppendToSubstring| when I wrote this,
which uses normal |malloc|. I'll attach another patch that also fixes this bug,
and looks safer to me.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Only use |strdup|/|malloc| in Resolve()

This is another option (that looks safer to me, but I've only started looking
at this code).

Comment 7

[Darin Fisher]

Blake: can you please provide a stack trace for this crash?  i'm asking for that
because i'd like to understand how it is that NSPR (PL_strdup) is using an
allocator that is not malloc.  nsCRT::strdup calls PL_strdup, and that calls
malloc.  i know that i am peeking under the hood here, and technically you're
doing the right thing by trying to make the allocators match, but why are you
seeing a crash when in fact the allocators are not mismatched?

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

A note: It seems to me what is happening is that "nsCRT.h" is including
<stdlib.h>. This brings in the standard library (compiler provided) version of
both malloc and free. The calls to malloc in the file are calling this version.
I'm a bit hazy about this, but it seems to me that it's possible that
nsCRT::strdup() is using the malloc provided in "prmem.c" which seems to be
doing its own memory management stuff (thread safe) as opposed to using the
compiler provided version. Then, a call to "standard" |free| (which uses its own
memory management stuff) in nsStandardURL.cpp will crash (in debug builds, where
there are asserts to catch this, in non-debug builds, I'll bet this is quietly
failing, or something similar) because it's using different memory. That's all
just guesses, stack trace follows:

_free_dbg_lk(void * 0x00c51fa0, int 1) line 1044 + 48 bytes
_free_dbg(void * 0x00c51fa0, int 1) line 1001 + 13 bytes
free(void * 0x00c51fa0) line 956 + 11 bytes
nsStandardURL::Resolve(nsStandardURL * const 0x00000000, const nsACString &
{...}, nsACString & {...}) line 1668 + 7 bytes
NS_MakeAbsoluteURI(nsAString & {...}, const nsAString & {...}, nsIURI *
0x01e29f40, nsIIOService * 0x00000000) line 279 + 46 bytes
rdf_MakeAbsoluteURI(const nsString & {...}, nsString & {...}) line 112 + 19 bytes
RDFContentSinkImpl::GetIdAboutAttribute(RDFContentSinkImpl * const 0x10261558,
const unsigned short * * 0x00c32e90, nsIRDFResource * * 0x0012e168, int *
0x00000000) line 960 + 50 bytes
RDFContentSinkImpl::OpenObject(RDFContentSinkImpl * const 0x10261558, const
unsigned short * 0x00c81320, const unsigned short * * 0x00c74578) line 1187 + 27
bytes
RDFContentSinkImpl::HandleStartElement(RDFContentSinkImpl * const 0x01dd889c,
const unsigned short * 0x00c81320, const unsigned short * * 0x00c74578, unsigned
int 4, int -1, unsigned int 76) line 520
nsExpatDriver::HandleStartElement(nsExpatDriver * const 0x10261558, const
unsigned short * 0x00c81320, const unsigned short * * 0x00c74578) line 359 + 38
bytes
Driver_HandleStartElement(void * 0x01e28df0, const char * 0x00c81320, const char
* * 0x00c74578) line 71
doContent(void * 0x00c750b0, int 0, const encoding * 0x00c8550c, const char *
0x00c85354, const char * 0x00c86b32, const char * * 0x00c74fbc) line 1453 + 14 bytes
contentProcessor(void * 0x00c74fb0, const char * 0x00c84790, const char *
0x00c86b32, const char * * 0x00c74fbc) line 1112 + 27 bytes
MOZ_XML_ParseBuffer(void * 0x00c74fb0, int 8192, int 0) line 970 + 39 bytes
MOZ_XML_Parse(void * 0x00c74fb0, const char * 0x01e2ea38, int 8192, int 0) line
959 + 10 bytes
nsExpatDriver::ParseBuffer(nsExpatDriver * const 0x10261558, const char *
0x01e2ea38, unsigned int 8192, int 0) line 836 + 11 bytes
nsExpatDriver::ConsumeToken(nsExpatDriver * const 0x01e28df4, nsScanner & {...},
int & 0) line 959
nsParser::Tokenize(nsParser * const 0x10261558, int 0) line 2563 + 19 bytes
nsParser::ResumeParse(nsParser * const 0x10261558, int 1, int 0, int 1) line
1759 + 17 bytes
nsParser::OnDataAvailable(nsParser * const 0x00001000, nsIRequest * 0x01e28ba8,
nsISupports * 0x00000000, nsIInputStream * 0x01e28d50, unsigned int 4096,
unsigned int 4096) line 2425 + 13 bytes
RDFXMLDataSourceImpl::OnDataAvailable(RDFXMLDataSourceImpl * const 0x01dd9730,
nsIRequest * 0x01e28ba8, nsISupports * 0x00000000, nsIInputStream * 0x01e28d50,
unsigned int 4096, unsigned int 4096) line 1143 + 62 bytes
RDFXMLDataSourceImpl::BlockingParse(RDFXMLDataSourceImpl * const 0x10261558,
nsIURI * 0x00000000, nsIStreamListener * 0x00001000) line 599
RDFXMLDataSourceImpl::Refresh(RDFXMLDataSourceImpl * const 0x00000000, int 1)
line 954
LocalStoreImpl::LoadData(LocalStoreImpl * const 0x10261558) line 470 + 16 bytes
LocalStoreImpl::Init(LocalStoreImpl * const 0x10261558) line 381
NS_NewLocalStore(nsISupports * 0x00000000, const nsID & {...}, void * *
0x0012f698) line 271 + 7 bytes
nsGenericFactory::CreateInstance(nsGenericFactory * const 0x01d515d8,
nsISupports * 0x00000000, const nsID & {...}, void * * 0x0012f698) line 82 + 14
bytes
nsComponentManagerImpl::CreateInstance(nsComponentManagerImpl * const
0x003a8f78, const nsID & {...}, nsISupports * 0x00000000, const nsID & {...},
void * * 0x01d515d8) line 1876 + 16 bytes
nsComponentManagerImpl::GetService(nsComponentManagerImpl * const 0x00000000,
const nsID & {...}, const nsID & {...}, void * * 0x0012f6d0) line 2097 + 49 bytes
nsGetServiceByCID::operator()(const nsGetServiceByCID * const 0x10261558, const
nsID & {...}, void * * 0x003a8f7c) line 101 + 13 bytes
nsCOMPtr<nsIRDFDataSource>::assign_from_helper(nsCOMPtr<nsIRDFDataSource> *
const 0x10261558, const nsCOMPtr_helper & {...}, const nsID & {...}) line 1050 +
14 bytes
nsCOMPtr<nsIRDFDataSource>::operator=(nsCOMPtr<nsIRDFDataSource> * const
0x10261558, const nsCOMPtr_helper & {...}) line 643
nsXULDocument::Init(nsXULDocument * const 0x10261558) line 2069 + 33 bytes
NS_NewXULDocument(nsIXULDocument * * 0x0012f738) line 463 + 10 bytes
CreateXULDocument(nsISupports * 0x00000000, const nsID & {...}, void * *
0x0012f7a0) line 588 + 33 bytes
nsGenericFactory::CreateInstance(nsGenericFactory * const 0x01d94ba0,
nsISupports * 0x00000000, const nsID & {...}, void * * 0x0012f7a0) line 82 + 14
bytes
nsComponentManagerImpl::CreateInstance(nsComponentManagerImpl * const
0x003a8f78, const nsID & {...}, nsISupports * 0x00000000, const nsID & {...},
void * * 0x01d94ba0) line 1876 + 16 bytes
nsCreateInstanceByCID::operator()(const nsCreateInstanceByCID * const
0x10261558, const nsID & {...}, void * * 0x0012f7a0) line 55 + 18 bytes
nsCOMPtr<nsIDocument>::assign_from_helper(nsCOMPtr<nsIDocument> * const
0x10261558, const nsCOMPtr_helper & {...}, const nsID & {...}) line 1050 + 14 bytes
nsContentDLF::CreateRDFDocument(nsContentDLF * const 0x10261558, nsISupports *
0x00000000, nsCOMPtr<nsIDocument> * 0x0012f7e4, nsCOMPtr<nsIDocumentViewer> *
0x0012f7e8) line 479 + 51 bytes
nsContentDLF::CreateRDFDocument(nsContentDLF * const 0x10261558, const char *
0x01c12590 `string', nsIChannel * 0x01d6fa28, nsILoadGroup * 0x00bad248, const
char * 0x01d50818, nsISupports * 0x00ba842c, nsISupports * 0x00000000,
nsIStreamListener * * 0x01d6fb78, nsIContentViewer * * 0x0012f93c) line 505 + 25
bytes
nsContentDLF::CreateInstance(nsContentDLF * const 0x01d59878, const char *
0x01c12590 `string', nsIChannel * 0x01d6fa28, nsILoadGroup * 0x00bad248, const
char * 0x01d50818, nsISupports * 0x00ba842c, nsISupports * 0x00000000,
nsIStreamListener * * 0x01d6fb78, nsIContentViewer * * 0x0012f93c) line 259 + 30
bytes
nsDocShell::NewContentViewerObj(nsDocShell * const 0x00ba8408, const char *
0x01d50818, nsIRequest * 0x01d6fa28, nsILoadGroup * 0x00bad248,
nsIStreamListener * * 0x01d6fb78, nsIContentViewer * * 0x0012f93c) line 4639 +
50 bytes
nsDocShell::CreateContentViewer(nsDocShell * const 0x00ba8408, const char *
0x01d50818, nsIRequest * 0x01d6fa28, nsIStreamListener * * 0x01d6fb78) line 4505
+ 35 bytes
nsDSURIContentListener::DoContent(nsDSURIContentListener * const 0x01d6fa28,
const char * 0x01d50818, int 0, nsIRequest * 0x01d6fa28, nsIStreamListener * *
0x01d6fb78, int * 0x00090000) line 127
nsDocumentOpenInfo::TryContentListener(nsDocumentOpenInfo * const 0x10261558,
nsIURIContentListener * 0x00ba87f0, nsIChannel * 0x01d6fa28) line 708 + 39 bytes
nsDocumentOpenInfo::DispatchContent(nsDocumentOpenInfo * const 0x10261558,
nsIRequest * 0x01d6fa28, nsISupports * 0x00000000) line 450 + 18 bytes
nsDocumentOpenInfo::OnStartRequest(nsDocumentOpenInfo * const 0x01d6fb68,
nsIRequest * 0x01d6fa28, nsISupports * 0x00000000) line 321 + 13 bytes
nsJARChannel::OnStartRequest(nsJARChannel * const 0x01d6fa30, nsIRequest *
0x01d6fe18, nsISupports * 0x00000000) line 673 + 52 bytes
nsInputStreamPump::OnStateStart(nsInputStreamPump * const 0x10261558) line 382
nsInputStreamPump::OnInputStreamReady(nsInputStreamPump * const 0x10026836,
nsIAsyncInputStream * 0x01d6fe1c) line 344
nsInputStreamReadyEvent::EventHandler(PLEvent * 0x01d5e634) line 118 + 12 bytes
PL_HandleEvent(PLEvent * 0x01d5e634) line 693
PL_ProcessPendingEvents(PLEventQueue * 0x1003e500) line 628
_md_EventReceiverProc(HWND__ * 0x002a0112, unsigned int 49443, unsigned int 0,
long 12700120) line 1434
USER32! 77d43a50()
USER32! 77d43b1f()
USER32! 77d43d79()
USER32! 77d43ddf()
nsAppShellService::Run(nsAppShellService * const 0x00b82400) line 523 + 48 bytes
main1(int 0, char * * 0x003a24c8, nsISupports * 0x00000000) line 1304
main(int 1, char * * 0x003a24c8) line 1779 + 22 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e814c7()

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

is strdup available on the mac these days?

Comment 10

[Darin Fisher]

strdup is available on OSX.

> I'm a bit hazy about this, but it seems to me that it's possible that
> nsCRT::strdup() is using the malloc provided in "prmem.c" which seems to be
> doing its own memory management stuff (thread safe) as opposed to using the
> compiler provided version.

huh?  here's nsCRT::strdup:

http://lxr.mozilla.org/mozilla/source/xpcom/ds/nsCRT.h#171

which calls PL_strdup.  and, here's the implementation of PL_strdup:

http://lxr.mozilla.org/mozilla/source/nsprpub/lib/libc/src/strdup.c#43

which calls malloc.

prmem.h does not redefine malloc to mean PR_Malloc.  prmem.h #include's
stdlib.h, so it appears that nsCRT::strdup is in fact using the same malloc.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 148030 [details] [diff] [review]
fixes this crash for me

I'm pretty sure now that this patch is less correct than the 2nd one, so I'll
request review on that one.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 148031 [details] [diff] [review]
Only use |strdup|/|malloc| in Resolve()

[sorry for the bugspam]

I've been looking at this for a while, and I'm completely confused as to what
the problem is. There is definately a problem with different pools of memory
being allocated/deallocated, but I can't find it. A breakpoint placed in the
'standard' malloc doesn't get hit from PL_strdup, though it clearly calls
malloc. This solution avoids the whole problem, by avoiding nsCRT (as far as
allocations go). I don't think this is a big issue anyway.

Comment 13

[Darin Fisher]

Is it possible that NSPR is using the non-debug Win32 heap?  Or, could it be
that you are somehow loading a release version of NSPR into your debug Mozilla
build?  I would recommend putting a breakpoint on PL_strdup, and then follow the
call to malloc to see where it leads.  Perhaps you will find that one is using
the debug heap and the other the release heap.  Also, it should be apparent that
there is something peculiar about your build because the code in question hasn't
changed in ages, and this is the first I've heard of this issue.  Lots of other
folks successfully build debug under Win32... including me! :-)

Comment 14

[Justin Wood (:Callek)]

Well darin, I personally do a clean then build each time I build my nightlies.

MSVC.net (2003?) (MSVC 7) Educational Version.

Suite with
 --enable-calendar --enable-official-branding --enable-crypto --enable-svg
--enable-svg-renderer-gdiplus

Same results with me and this build, debug used to work, perhaps something in
the makefile?

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

For no reason that I can gather, nspr is, indeed, linking against the non-debug
versions of malloc and free. In fact, nspr is using the wrong flags for MSVC in
debug mode (it tries to use -g to generate debug information, which is a GCC
flag where it should be using -Zi -- there are linker options that also affect
this, but changing those doesn't appear to help). I'm at a loss as to how to
tell it to link against the debug version of the libraries.

Does anybody have any other ideas (e.g. how to tell it to link against the debug
versions of the libraries or even fix up nspr's Makefile in debug mode)?

Comment 16

[Darin Fisher]

Hmm... I wonder if you are somehow building NSPR using MingW and Moz using MSVC?
 Is that possible?  Or maybe our configure script is just getting confused,
thinking that it needs to pass -g to CL.exe?  cc'ing some folks who might be
able to help unravel this mystery.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

I'm starting to think that configure/autoconf (or whatever generates
autoconf.mk) is getting confused. If, in /mozilla/nsprpub/, I run: 'sh configure
--enable-debug'. I get all valid MSVC debug flags (this doesn't fix the crash,
but it makes me feel better about building :). However, if I run: 'sh configure
--enable-debug --enable-optimize' which is coincidentally what running
'configure' in /mozilla/ does when it kicks off 'configure' in nsprpub, I get
'-g' as a compile flag, and no debug linking flags (i.e., what gcc would
expect). Not sure if this helps any.

Comment 18

[cls]

This sounds like fallout from bug 54828 though I'm at a loss to explain the -g
since the default _DEBUG_FLAGS is overridden in a win32 !GNU_CC check. 

Can you attach the complete output from running mozilla/configure and both
mozilla/config/autoconf.mk & mozilla/nsprpub/config/autoconf.mk ?

Comment 19

[Justin Wood (:Callek)]

crash happens for me also, I'm going to attach my autoconf.mk files...

Build flags:

 --enable-calendar --enable-official-branding --enable-crypto --enable-svg
--enable-svg-renderer-gdiplus

(yes --enable-official-branding really isnt needed when not building FF, but I
keep it there incase official branding is ever for Suite too, just so I don't
have to worry about it.)

Comment 20

[Justin Wood (:Callek)]

Attachment: mozilla/config/autoconf.mk

Comment 21

[Justin Wood (:Callek)]

Attachment: mozilla/nsprpub/config/autoconf.mk

As a side note, I run autoconfig locally, after every co.

Comment 22

[cls]

Is that nsprpub/config/autoconf.mk the result of running mozilla/configure?  Can
you attach the mozilla/configure output as well?

Comment 23

[Justin Wood (:Callek)]

Attachment: Build Output (cut at configure stuff)

This is from the config output from when I did a 'clean' (not config'd when I
do the build...I do them seperate invokes)

I Divert all build output to a file, so heres the snip with just config's

Comment 24

[Darin Fisher]

-> Build Config

Comment 25

[Justin Wood (:Callek)]

IMHO, should not be a Build->Config bug, since yes we noticed a config prob
here, though as someone said it does not fix the crash on start-up...which the
stack-trace is here for...

Comment 26

[cls]

This is a bug in NSPR's configure and we just exposed it when we changed the
Mozilla defaults.  As you can see by the block starting at
http://lxr.mozilla.org/seamonkey/source/nsprpub/configure.in#1278 , configure.in
is only setting the MSVC _DEBUG_FLAGS when MOZ_OPTIMIZE is not set. 
_DEBUG_FLAGS should be getting set unconditionally as it's only used when
MOZ_DEBUG is set (per
http://lxr.mozilla.org/seamonkey/source/nsprpub/configure.in#2378).

Comment 27

[cls]

Attachment: v1.0

Here's a quick cleanup fix.  I don't have MSVC so I can't test it.

Comment 28

[Justin Wood (:Callek)]

Attachment: v1.0 (configure)

No autoconf required if you apply this patch. (created from previous
attachment)

I'll be building DBG with msvc 7 (.NET) now, when Its done, I'll report.

Comment 29

[Wan-Teh Chang]

Comment on attachment 148543 [details] [diff] [review]
v1.0

cls, thanks for the patch.  Your patch has two
problems.

The first problem is subtle. -Od means "disable
all optimizations".  It belongs in neither
_DEBUG_FLAGS nor _OPTIMIZE_FLAGS.  It needs to
be added to CFLAGS when MOZ_OPTIMIZE is not
defined.

The second problem is that the define and undefine
of the _DEBUG macro were accidentally removed.

I will attach a new version of the patch with
these two problems fixed.  My patch will also
remove the unused GLOWCODE and PROFILE code.
(I am using MOZ_PROFILE with optimized builds.)

Comment 30

[Wan-Teh Chang]

Attachment: Proposed patch v1.1

Comment 31

[cls]

Comment on attachment 148555 [details] [diff] [review]
Proposed patch v1.1

The removal of the _DEBUG define wasn't accidental though it was apparently the
wrong fix.  We define -DDEBUG for everyone on line 341.  Why are we only
defining -D_DEBUG for win32?

Comment 32

[Wan-Teh Chang]

_DEBUG is a macro used by MSVC's libc (C runtime)
header files, so it only matters for Win32.

However, I just found that _DEBUG is defined by
the MSVC compiler if the -MDd flag is specified.
So I am wondering if we need to define or undefine
_DEBUG explicitly.  Until we figure this out, it
is safer to leave it alone.

Comment 33

[Wan-Teh Chang]

Just to be complete:

If -MD, -MT, or -ML is used, the MSVC compiler does not
define _DEBUG.

If -MDd, -MTd, or -MLd is used, the MSVC compiler defines
_DEBUG.

Comment 34

[Justin Wood (:Callek)]

as a note, the 1.0 patch built fine for me, the 1.1 patch have not tested yet.

Comment 35

[Wan-Teh Chang]

Comment on attachment 148031 [details] [diff] [review]
Only use |strdup|/|malloc| in Resolve()

Memory allocated by nsCRT::strdup must be
freed by nsCRT::free.

Memory allocated by PL_strdup must be
freed by PL_strfree.

Since *result may be allocated by either
nsCRT::strdup or malloc (via AppendToSubstring),
it is tricky to free *result properly.

Fixing nsprpub/configure.in hides this
problem, which is okay.  However, I recommend
you fix this problem.

A proper fix is to implement a string
duplicate function in this file that
calls malloc, and replace nsCRT::strdup
by the local string duplicate function.
(This string duplicate function is only
six lines of C code.)

Another fix is to replace nsCRTL::strdup(relpath)
by AppendToSubstring(0, 0, relpath).  This
avoids the local string duplicate function
but seems to be an abuse of AppendToSubstring.

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

Why not just use regular 'strdup' (i.e., my second patch) and not re-write a
function that is already provided by the library?

Comment 37

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: described patch

This duplicates the strdup function. (aka the suggested patch).

Comment 38

[Wan-Teh Chang]

Re: comment 36

strdup is not a Standard C library function.  However,
it seems to be a very common extension these days, and
it is in Single Unix Specification (Version 2).  So
perhaps it is okay to use it now.

Comment 39

[cls]

FYI, the same change (patch v1.1) needs to be applied to the LDAP tree as well.
 Has anyone verified that v1.1 works?

Comment 40

[Justin Wood (:Callek)]

Just ran after applied patch v1.1, though I was stupid enough not to run
autoconf before applying the build instructions, will re-compile overnight,
comment tomorrow.

Comment 41

[Blake Kaplan (:mrbkap) (inactive)]

patch v1.1 appears to fix this for me (the build runs without crashing, and the
right compiler/linker flags are generated).

Comment 42

[Justin Wood (:Callek)]

same here, I say ready to checkin, and please double check if it should be
applied to the 1.7 branch(es) [such as Aviary]

Comment 43

[Wan-Teh Chang]

Comment on attachment 148555 [details] [diff] [review]
Proposed patch v1.1

cls, if you like this patch, could you check
it in?	Thanks.

Comment 44

[Dan Mosedale (:dmosedale, :dmose)]

Using strdup() sounds like a good plan to me.  People used to occasionally check
this sort of thing in, and the only problems that I remember it causing were on
MacOS 9.  

Comment 45

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 244133 has been marked as a duplicate of this bug. ***

Comment 46

[Brodie]

Please do not create more implementations of strdup et al, there are already 
too many (thus causing this problem). For a patch to fix the mixing of clib and 
NSPR allocation routines, please see my patch attached to bug 244133.

However I would love it if you fixed NSPR to correctly build a debug version 
when mozilla is building debug, so I would like to see the NSPR patch go in in 
addition. 

Comment 47

[Brodie]

Attachment: patch for nsStandardURL

This is the patch from my duplicate bug.

Comment 48

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 148560 [details] [diff] [review]
described patch

I'm not really sure why I made this in the first place.

Comment 49

[cls]

The v1.1 patch has been checked in on the NSPR & LDAP client branches & trunks.

Comment 50

[Wan-Teh Chang]

Comment on attachment 148965 [details] [diff] [review]
patch for nsStandardURL

This patch is still using two allocators:

1. PR_Malloc and PR_Free
2. nsCRT::strdup and nsCRT::free

By examining the nsCRT source code, you can
determine that the second allocator is the
same as:

2'. PL_strdup and PL_strfree

There is no guarantee that 1 and 2' are the
same allocator.  So you haven't reduced the
number of allocators.

We can do two things:
- modify PL_strdup and PL_strfree to use
  PR_Malloc and PR_Free.
- declare that it is okay to use strdup in
  Mozilla.

We probably should do both, but I think we
should definitely consider allowing strdup.

Comment 51

[Darin Fisher]

I believe strdup should now be valid in Mozilla on all platforms.  It was only a
problem in Mac OS9, and we don't support that platform anymore.

Let's just use malloc, free, and strdup :-)

Comment 52

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Only use malloc/strdup/free

This nukes all uses of 'nsCRT::strdup'. I think there is still a slight chance
of mismatched allocators - but it's outside the scope of this bug (if you care,
the problem is that mHostA assigned using both 'strdup' [nsCRT::strdup before
this patch] and ToNewCString(). ToNewCString() calls PR_Malloc(), which, if
NSPR_USE_ZONE_ALLOCATOR is defined to 1 in the environment, calls
pr_ZoneMalloc. Otherwise, it calls regular malloc, and everything is happy).

Comment 53

[Darin Fisher]

> the problem is that mHostA assigned using both 'strdup' [nsCRT::strdup before
> this patch] and ToNewCString(). ToNewCString() calls PR_Malloc(), which, if
> NSPR_USE_ZONE_ALLOCATOR is defined to 1 in the environment, calls
> pr_ZoneMalloc. Otherwise, it calls regular malloc, and everything is happy).

I think nsMemory::Alloc should just call malloc, but yeah.. that deserves a
separate bug.  Thanks for the patch, Blake!

Comment 54

[Darin Fisher]

Comment on attachment 149000 [details] [diff] [review]
Only use malloc/strdup/free

>+    free(mHostA);

>+    free(mHostA);
>+    mHostA = 0;

might be good to wrap these with "if (mHostA) { ..."

I know that free is supposed to handle null valued input, but alecf
swears he fought with bad implementations of free in the past.	also,
it is very unlikely that mHostA will be non-null, so it might make
sense to avoid the function call overhead on average.

Comment 55

[Wan-Teh Chang]

If you are worried about free that can't handle null
valued input, you should worry about the portability
of strdup more.

Comment 56

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: use null-checks

Same as above - but with Darin's comments addressed.

Comment 57

[Darin Fisher]

Comment on attachment 149003 [details] [diff] [review]
use null-checks

r+sr=darin

Comment 58

[Darin Fisher]

fixed-on-trunk

wtc: yeah, but i think i prefer to error on the side of caution here.  the null
checks were there already...

Comment 59

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #58)
> fixed-on-trunk

hmm, are you sure? I don't see a checkin for this patch...

Comment 60

[cls]

*** Bug 244336 has been marked as a duplicate of this bug. ***

Comment 61

[timeless]

just a reminder, on windows each dll can and likely will have its own heap, so 
malloc in one dll is not compatible w/ free in another dll, hence the need to 
use something like nspr (where all allocs and frees are done in nspr.dll's 
heap).

Comment 62

[Wan-Teh Chang]

timeless's warning is correct.  We should try very hard
to ensure that all the DLLs are linked with the same
libc.  This may not be possible if some of the DLLs come
from third parties.  So it is a good idea to follow
timeless's advice.

In this particular case, the use of malloc, strdup, and
free are within the same file (nsStandardURL.cpp), so
only one heap is involved, and therefore we are okay.