75796 - new leak per url load tool

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, enhancement)

Description

[Steve Adamski (gone)]

I have a patch to source for mozilla, which generates one large file, which
contains leak information. The file consists of a header, which is the name of
the url that is loaded, followed by the leaks associated with that url.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Steve, would you care to attach a diff to this bug using
http://bugzilla.mozilla.org/createattachment.cgi?id=75796 ?

Comment 2

[Steve Adamski (gone)]

Attachment: source code changes -- first prototype

Comment 3

[Steve Adamski (gone)]

The first basic question, is how do we want to implement the interface to this
functionality? In other words, should we implement it as:
1) some conditional inclusion build-time macro constant
2) a pref
3) part of the debug build
4) dependent on some environment variable
5) other, or some combination (perhaps, a pref on the debug build)

Comment 4

[Steve Adamski (gone)]

Concensus so far:
1) not part of debug build
2) only as part of boehm build
3) the rest (how to switch the functionality on or off) doesn't seem to matter
much to people. 

Comment 5

[Steve Adamski (gone)]

Attachment: The real proposed patch, with correct MACROs, tabs, etc.

Comment 6

[Steve Adamski (gone)]

Attachment: patch: better conforms to coding style (Braces, and spaces)

Comment 7

[Scott Collins]

You don't need to initialize an |nsCOMPtr| to |NULL|.  It does so by itself.
Cool C++-kitties prefer |0| over |NULL| or |nsnull| in any case.  Don't compare
with |NULL| or |0|, just test, e.g., prefer

  if ( foo )

over

  if ( foo != NULL )

...and as I mention above, prefer |0| (in general) or else |nsnull| (in mozilla)
over |NULL|.

Declare your variables right where you can first initialize them (where that is
compatible with the desired lifetime), so in your code you would move the
declaration of |currURI| into the |if (result)| clause, and the declaration of
|urlName| to inside the |if (currURI)| clause.

We have a general portability rule about not conditionally |#include|ing files

  http://www.mozilla.org/hacking/portable-cpp.html#dont_ifdef_include

This debugging case isn't explicitly mentioned as an exception, but I think it
should be, so you probably shouldn't be dissuaded from this if it comes up.

I think you'll want to double-check your auto-conf stuff with Chris Seawood, but
other than that, it looks good to me.  Fix the declarations and tests and you'll
have my sr.
  

Comment 8

[Scott Collins]

Oops.  We crossed the streams :-)  My review above refers to your second patch

  http://bugzilla.mozilla.org/showattachment.cgi?attach_id=35197

and I see you have addressed some of the concerns (notably that you don't need
to init an |nsCOMPtr| to |NULL|).  The declaration and test comments still
apply, I think.

Comment 9

[Steve Adamski (gone)]

Attachment: patch, in reponse to scc's review

Comment 10

[Steve Adamski (gone)]

Attachment: a sample of leak analysis that this feature allows

Comment 11

[Steve Adamski (gone)]

Attachment: sample graph of bytes leaked per url, created from output

Comment 12

[Steve Adamski (gone)]

Attachment: sample graph of number of leaks per url, created from output

Comment 13

[Christopher Blizzard (:blizzard)]

Spontaneous review!

Please remove the #ifdef(linux) from all of the defines and move it into the
configure test instead, assuming that it has to be linux.  Having more than one
define for the option makes it hard to get running on other platforms and
increases the fragility and maintenance down the road.

Comment 14

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

Probably there's no need to limit it further than GC_LEAK_DETECTOR, since people
will use GC_LEAK_DETECTOR where it works and won't where it doesn't work
(because it won't work :-).  I would think this code ought to work on Mac, right?

Comment 15

[Cees T.]

Shouldn't bugs like this one have patch and perf keywords?

Comment 16

[Benjamin Smedberg]

I believe that we have an equivalent of this now, and it is no longer useful code.