95275 - nsTraceMalloc broken after NSPR landing

Status: RESOLVED WORKSFORME

(Core :: XPCOM, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

nsTraceMalloc is broken after the NSPR landing.  It's going into infinite
recursion, I think because PR_EnterMonitor is allocating where it didn't before:

#65366 0x405dfc8e in pthread_start_thread_event (arg=0xbf7ffc00)
    at manager.c:274
274	  outcome = self->p_start_args.start_routine(THREAD_GETMEM(self,
#65365 0x405bbd7a in _pt_root (arg=0x8140470)
    at /builds/seamonkey/mozilla/nsprpub/pr/src/pthreads/ptthread.c:197
197	    rv = pthread_setspecific(pt_book.key, thred);
#65364 0x405e35ed in __pthread_setspecific (key=0, pointer=0x8140470)
    at specific.c:109
109	      return ENOMEM;
#65363 0x401ec61d in calloc (count=32, size=4)
    at /builds/seamonkey/mozilla/xpcom/base/nsTraceMalloc.c:1274
1274	        PR_EnterMonitor(tmmon);
#65362 0x405b4d07 in PR_EnterMonitor (mon=0x8081730)
    at /builds/seamonkey/mozilla/nsprpub/pr/src/pthreads/ptsynch.c:495
495	    PRThread *self = PR_GetCurrentThread();
#65361 0x405bc841 in PR_GetCurrentThread ()
    at /builds/seamonkey/mozilla/nsprpub/pr/src/pthreads/ptthread.c:619
619	    if (NULL == thred) thred = pt_AttachThread();
#65360 0x405bbf8f in pt_AttachThread ()
    at /builds/seamonkey/mozilla/nsprpub/pr/src/pthreads/ptthread.c:273
273	    thred = PR_NEWZAP(PRThread);
#65359 0x405a2500 in PR_Calloc (nelem=1, elsize=136)
    at /builds/seamonkey/mozilla/nsprpub/pr/src/malloc/prmem.c:64
64	    return calloc(nelem, elsize);
#65358 0x401ec61d in calloc (count=1, size=136)
    at /builds/seamonkey/mozilla/xpcom/base/nsTraceMalloc.c:1274
1274	        PR_EnterMonitor(tmmon);
#65357 0x405b4d07 in PR_EnterMonitor (mon=0x8081730)
    at /builds/seamonkey/mozilla/nsprpub/pr/src/pthreads/ptsynch.c:495
495	    PRThread *self = PR_GetCurrentThread();

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: avoid locking when not doing logging

Comment 2

[Wan-Teh Chang]

This is correct.  PR_EnterMonitor is allocating an NSPR thread ID
the first time it is called by a thread that is not created by NSPR.

Does your code assume that PR_EnterMonitor does not allocate?  This
is not something that even I can guarantee.  For example, I can't
guarantee that the underlying pthread_mutex_lock() call will not
allocate in future releases of the pthread library.

Comment 3

[Brendan Eich [:brendan]]

dbaron: thanks for the patch.  Only one nit, on the unnecessary change in the
patch: I prefer == 0 for non-booleans, usually.  But I'm curious to know why you
changed it, maybe there's a good reason.

Since you're patching, I'll r/sr=[e-mail].  Why don't you take the
bug, unless you'd rather I detach the patch, test it, and commit it?

/be

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That doesn't actually fix it, it just allows me to run without trace-malloc logging.

And when I noticed earlier that although it was being assigned a boolean value,
it's later used as an integer, I undid that first change...

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I have to say -- since allocators need to use locks (or in this case, monitors),
it would be nice if locks didn't allocate.  This NSPR change has broken
nsTraceMalloc, the Boehm GC glue, and Brendan mentioned something else.  So
basically we don't have any good memory debugging tools anymore.

Comment 6

[Wan-Teh Chang]

Just replace PRMonitor by PRLock or pthread_mutex_t and
you'll be fine.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm pretty sure trace-malloc depends on reentering the monitor on the same
thread (because it breaks out when it's one of its own allocations after it's
already entered the monitor).

Comment 8

[Wan-Teh Chang]

After contemplating on this problem for two days,
I decided to back out the NSPR change.  Things
should work for you now.

Comment 9

[Brendan Eich [:brendan]]

dbaron, you want me to make any changes, now that wtc has backed out the change
to make PR_EnterMonitor potentially allocate?

/be

Comment 10

[Brendan Eich [:brendan]]

I need to clean up some things in nsTraceMalloc.c, but I'm not going to abuse
this bug.  This is a WORKSFORME now that wtc has fixed things so Enter doesn't
allocate.

/be