397103 - Rlk/Lk regression in crash reporter setup code

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect, P2)

Description

[J. Paul Reed [:preed]]

A little bit of fallout from bug 396805: looks like RLk and Lk numbers regressed a bit from deploying the new reference platform on the leaktest/debug machine:

Before:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1190404380.1190404829.7561.gz&fulltext=1

After:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1190405760.1190406702.15924.gz&fulltext=1

The compiler did change:

Before: Compiler is -- gcc (gcc (GCC) 4.1.2 20061011 (Red Hat 4.1.1-29)

After: Compiler is -- gcc (gcc (GCC) 4.1.1 20070105 (Red Hat 4.1.1-52)

bsmedberg said something about the new compiler we using doing... something differently, that might affect this, but... I unfortunately can't remember all the details.

Comment 1

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

I could understand the changes (RLk from 0 to 8 bytes, Lk from 1.5meg to 4.5meg) being due to a change in system library versions:  the former because we turned on support for something that has a string leak due to autodetection of build options in configure; the latter because a system library is really leaky.

It looks from the log like the first run showing up on tinderbox wasn't actually the first run.  Any idea where I could see the log for the actual first successful run?

Comment 2

[Benjamin Smedberg]

What was the OS before? If it was CentOS4.3 and now we're using 5, I'd suspect new features that weren't compiled in with 4.3, namely gnomevfs and desktop integration support.

Comment 3

[J. Paul Reed [:preed]]

(In reply to comment #2)
> What was the OS before? If it was CentOS4.3 and now we're using 5, I'd suspect
> new features that weren't compiled in with 4.3, namely gnomevfs and desktop
> integration support.

It was CentOS 4.4 and now we're using 5, yeah, with the new "custom" compiler. :-)

(In reply to comment #1)

> It looks from the log like the first run showing up on tinderbox wasn't
> actually the first run.  Any idea where I could see the log for the actual
> first successful run?

Yah, sorry about that; it was reporting into MozillaExperimental for a day or so beforehand:

http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaExperimental&maxdate=1190367891&hours=24&legend=0&norules=1

I can clobber this build if it would help with some debugging or some such.

Comment 4

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

Never mind -- the first build actually isn't helpful.

It wouldn't surprise me if we just picked up a leakier version of pango, actually.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: gdb instructions for figuring out what leaked

We could figure out what's being leaked by seeing what's in the leaked nsStringBuffer or by finding where it was allocated.  I've attached a set of untested gdb commands which give the contents of the string being leaked, which should be enough to figure out what's in the string being leaked.  (It's almost copy-and-paste, but you need to read some printed output at one point.)

Also, the file created by the following command (might need to pull tools/rb):

 cat /tmp/stringbuffer-leaks.txt | tools/rb/fix-linux-stack.pl > fixed-leaks.txt

will give enough info to determine where the leaked string was allocated.

Could someone with shell access to fxdbug-linux-tbox run gdb on the firefox-bin and run the commands in the file, then the shell command, from mozilla/, and report on the results?  You'll probably have to briefly stop the continuous building to do so.

Comment 6

[Reed Loden [:reed]]

Moving to Build so that they can help figure out the leak.

Comment 7

[Chris Cooper [:coop] (he/him)]

Comment on attachment 282205 [details]
gdb instructions for figuring out what leaked

Jeff: I tried your gdb script today without success. The breakpoint you provided was never hit, the program ran to completion and exited normally. When I initially set it up, I saw:

(gdb) b nsTraceRefcntImpl.c:526
No source file named nsTraceRefcntImpl.c.
Make breakpoint pending on future shared library load? (y or [n]) y

Breakpoint 1 (nsTraceRefcntImpl.c:526) pending.

Expected?

Since it ran to completion, I now have an enormous (660 MB) log file of questionable value. Let me know if you want it.

Comment 8

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

You could run

mozilla/tools/rb/find-leakers.pl log

and then

mozilla/tools/rb/make-tree.pl --object 0xADDRESS-THAT-FIND-LEAKERS-PRINTS < log | mozilla/tools/rb/fix-linux-stack.pl > tree

and then attach tree to this bug.

Comment 9

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

(And you need to run that in the directory that the build was run in, since there will be relative paths in log that fix-linux-stack needs to fix.)

Comment 10

[Jeff Walden [:Waldo]]

(In reply to comment #7)
> Expected?

Oh, blah.  That should have been nsTraceRefcntImpl.cpp (note the "pp").  But yeah, the find-leakers.pl script should still work fine.

I also tried reproducing this locally in a VM, on the guess that it was something in the crashreporter code as dbaron said he builds with it disabled, but I got nothing.  I'm trying again with what I hope is a config closer to what I see in logs, but I doubt I'll be able to reproduce it either.

Comment 11

[Chris Cooper [:coop] (he/him)]

I changed the filename, but was still unable to hit the breakpoint. I checked mxr and nsTraceRefcntImpl.cpp:526 is a blank line. I *was* able to hit the breakpoint when I switched it to line 527, the first line of the "done:" block:

http://mxr.mozilla.org/seamonkey/source/xpcom/base/nsTraceRefcntImpl.cpp#527

...however, i still couldn't grab the address using any of the commands listed:

Breakpoint 2, nsTraceRefcntImpl::DumpStatistics (
    type=nsTraceRefcntImpl::ALL_STATS, out=0x10a84c0)
    at /builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/xpcom/base/nsTraceRefcntImpl.cpp:528
528       gLogging = wasLogging;
(gdb) p (char*)(((nsStringBuffer*)ADDRESS) + 1)
No symbol "ADDRESS" in current context.
(gdb) p (wchar_t*)(((nsStringBuffer*)ADDRESS) + 1)
No symbol "ADDRESS" in current context.
(gdb) x/NUMBERch (((nsStringBuffer*)ADDRESS) + 1)
No symbol "NUMBERch" in current context.

I do have the log file, so I'll spend some time this morning running the various commands listed in the comments above and will post the results.

Comment 12

[Chris Cooper [:coop] (he/him)]

Attachment: Output from make-tree.pl, as requested by dbaron

Comment 13

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

Looks like it's:
static nsCString* crashReporterAPIData = nsnull;
in nsExceptionHandler.cpp.

Comment 14

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

I suspect this is also showing up on the new Windows leak test tinderbox.

Comment 15

[(not currently active) Ted Mielczarek]

Guess this is mine then.  Looks like we don't delete that in cleanup.

Comment 16

[(not currently active) Ted Mielczarek]

Attachment: cleanup crashReporterAPIData

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 286018 [details] [diff] [review]
cleanup crashReporterAPIData

bsmedberg is probably travelling, let's expedite this.

Comment 18

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

Comment on attachment 286018 [details] [diff] [review]
cleanup crashReporterAPIData

r=dbaron

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 286018 [details] [diff] [review]
cleanup crashReporterAPIData

Trivial leak fix, would love to get this landed.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 286018 [details] [diff] [review]
cleanup crashReporterAPIData

a=endgame drivers for M9 landing

Comment 21

[Jeff Walden [:Waldo]]

(In reply to comment #11)
> ...however, i still couldn't grab the address using any of the commands listed:

Not that it matters now, but your problem was you didn't replace ADDRESS with the hex address that should have been printed out when the breakpoint was hit.  The first command spews out enough data to show that the string contains the application settings, which would have been enough to figure out where the leak was.  (I actually thought it might be the eventual variable discovered here, just by code inspection, but I assumed my builds were actually running the relevant code when they weren't and didn't mention it. :-\ )

So what the problem came down to with respect to why I wasn't hitting this was that there's no "Enabled=1" line in Firefox's application.ini, or MOZ_CRASHREPORTER isn't set in the environment; with either of those I get the leak.  I'm *still* not sure from looking at the logs how the tinderboxen are actually getting the crash reporter code to run, because I don't see another .ini file or the variable in the envvars listing.  Is there anything we can do to make this code run in homegrown builds without actually have it submit any data, or something like that so that we get greater visibility into these problems in the future?

Comment 22

[(not currently active) Ted Mielczarek]

I'm wondering if we didn't have MOZ_CRASHREPORTER_DISABLE set on the old tinderbox, and that was why this looked like a regression due to the tinderbox change.  Also, I wonder if that isn't set on the OS X tinderbox.  The proper env var to use, IMO, is MOZ_CRASHREPORTER_NO_REPORT, which still sets the exception handler and everything, but doesn't run the crash reporter client.  This way all the setup code is still executed.

Anyway, checked in.

Comment 23

[(not currently active) Ted Mielczarek]

Didn't see Waldo's comment there, but Waldo: that Enabled=1 gets stuck in application.ini when MOZILLA_OFFICIAL is defined.  Alternatively, you can set MOZ_CRASHREPORTER=1 in your environment to force it on at runtime.  Thanks for all the debugging!

Comment 24

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

For the record, I think this bug really ended up being about the RLk regression; I suspect the Lk regression isn't worth chasing down because it's probably just leaks in system libraries (or failure to shut down).