new leak per url load tool




18 years ago
5 years ago


(Reporter: thesteve, Unassigned)



Firefox Tracking Flags

(Not tracked)



(7 attachments)



18 years ago
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.
Steve, would you care to attach a diff to this bug using ?
Ever confirmed: true

Comment 2

18 years ago
Created attachment 30840 [details]
source code changes -- first prototype

Comment 3

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

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


18 years ago
Blocks: 71874

Comment 5

18 years ago
Created attachment 35197 [details] [diff] [review]
The real proposed patch, with correct MACROs, tabs, etc.

Comment 6

18 years ago
Created attachment 35198 [details] [diff] [review]
patch: better conforms to coding style (Braces, and spaces)

Comment 7

18 years ago
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 )


  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

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

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

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

18 years ago
Created attachment 36870 [details] [diff] [review]
patch, in reponse to scc's review

Comment 10

18 years ago
Created attachment 36871 [details]
a sample of leak analysis that this feature allows

Comment 11

18 years ago
Created attachment 36872 [details]
sample graph of bytes leaked per url, created from output

Comment 12

18 years ago
Created attachment 36873 [details]
sample graph of number of leaks per url, created from output
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.
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?
Assignee: thesteve → nobody
QA Contact: scc → xpcom

Comment 15

12 years ago
Shouldn't bugs like this one have patch and perf keywords?
I believe that we have an equivalent of this now, and it is no longer useful code.
Last Resolved: 7 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.