Open Bug 390971 Opened 12 years ago Updated 11 years ago

leak-gauge.pl should be automatedly invokable and give more output

Categories

(Testing :: General, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

People

(Reporter: ray, Assigned: ray)

Details

Attachments

(1 file, 4 obsolete files)

The leak-gauge.pl script it designed to be used interactively. It seemed to me that one could take a list of executables and a list of urls and invoke this script automatically. Memory leak testing would be easier to accomplish if it is automatable.

The one proviso in this is that one cannot get a Firefox instance to quit. One can launch from the command-line but one cannot get the instance to quit without using the UI. Unless one has installed an extension that I wrote (http://www.wykiwyk.com/mozilla/loadAndDie/) which waits for one page to finish loading and then quits the browser.
Attachment #275283 - Flags: review?(sayrer)
Comment on attachment 275283 [details] [diff] [review]
adds a script that calls leak-gauge.pl repeatedly and makes backwards-compatible changes to leak-guage.pl

dbaron should review this, not me.
Attachment #275283 - Flags: review?(sayrer) → review?(dbaron)
Comment on attachment 275283 [details] [diff] [review]
adds a script that calls leak-gauge.pl repeatedly and makes backwards-compatible changes to leak-guage.pl

Why did you put the code to call the app inside leak-gauge.pl ?  I'd think if you didn't do that, you wouldn't have to copy so much code (which makes leak-gauge harder to maintain in the future), and you also wouldn't have to break the principle that leak-gauge.pl operates on standard input and standard output like many other command-line programs.
Attachment #275283 - Flags: review?(dbaron) → review-
(And, for what it's worth, I'd find something like this especially useful for the XPCOM_MEM_LEAK_LOG output; leak-gauge.pl already deals with the what-is-what-page problem somewhat decently.  Not putting running the app inside leak-gauge.pl would make it easier to reuse for that, or for both-logs-at-once.)
I could pull more of the code that I have added to leak-gauge.pl out into
call-leak-gauge.pl.

Actually, I think I could pull out all of it into the call-leak-gauge.pl, now
that I am looking at it. Earlier, I was concentrating more on getting it to
work.

I suspect it would be most re-usable if the definitions on the call subroutine
and the handlers structure were in their own file, by themselves. The "default
dump block" you put at the bottom of the script could be wrapped in a "if
(scalar(@ARGV) == 0)". Then anyone could include it, have any parameter on the
command-line and override the dump block.

First, I will put in a patch, if I can, that only adds the new script. Let us
see that and then there might be something else to do or not....
Status: NEW → ASSIGNED
I only had to do 3 things to the leak-gauge.pl script itself. First was to end the script with "1;" so that I could "do" the script. Second was to remove the "my" in front of the handler definition, so that I could see the handler in another file. Third was to wrap the default handling in a check. If a single parameter is passed in and it is a file, as dbaron describes in the documentation, the script will behave as it always has.

But it can be included in call-leak-gauge.pl, which can test different application executables and different urls and get a leak report for each.
Attachment #275283 - Attachment is obsolete: true
Attachment #275540 - Flags: review?(dbaron)
Attachment #275540 - Attachment is obsolete: true
Attachment #275542 - Flags: review?(dbaron)
Attachment #275540 - Flags: review?(dbaron)
Comment on attachment 275542 [details] [diff] [review]
very minor fix, was not finding leak-gauge.pl if not called in directory...

Have to fix something. Once it finds a bug, that information is kept. Not clearing all the handler structures yet.
Attachment #275542 - Flags: review?(dbaron)
Rather than try to figure out how to clear the handlers data, I turned this script into a two part script. The script gets a list of apps and a list of urls and calls itself repeatedly, with an extra parameter. Seeing that parameter, the script does a leak-gauge analysis on a single URL and then shuts down.
Attachment #275542 - Attachment is obsolete: true
Attachment #275739 - Flags: review?(dbaron)
What's wrong with actually calling leak-gauge.pl repeatedly, and not modifying it?
I can. Here it is.
Attachment #275739 - Attachment is obsolete: true
Attachment #277194 - Flags: review?(dbaron)
Attachment #275739 - Flags: review?(dbaron)
Assignee: nobody → ray
Status: ASSIGNED → NEW
Comment on attachment 277194 [details] [diff] [review]
call-leak-gauge.pl

>+# Software distributed under the License is distributed on an "AS IS" basis,
>+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+# for the specific language governing rights and limitations under the
>+# License.
>+#
>+# Portions created by the Initial Developer are Copyright (C) 2007
>+# the Initial Developer. All Rights Reserved.

You took out a decent-sized chunk of the license header here -- parts that you're supposed to fill in.

>+# installed the extension found at http://www.wykiwyk.com/mozilla/loadAndDie into the profile

Seems a little odd to have code in our tree depend on code on your personal site.

>+if (($ARGV[0] eq "-h") || ($ARGV[0] eq "-help") || ($ARGV[0] eq "--help")) {
>+    print "usage: perl ./call-leak-gauge.pl <appList> <profile> <urlList>\n";
>+    exit(0);
>+}
>+
>+$LGPATH = $0;
>+$LGPATH =~ s/call-leak-gauge.pl$//;

You're better off using dirname($0) here.  It requires a "use File::Basename;" at the top.  (There are other, potentially-more-windows-compatible things here too, I think, but dirname is probably good enough.)  (It probably produces a result without the trailing /, though.)

>+for ($idx = 0; $idx < scalar(@apps); $idx++) {

$#apps is probably better than scalar(@apps).

>+        $cmd = $0." ".$app." ".$profile." ".$url;

Probably cleaner to put all the variables inside a ""-ed string.

>+        $nspr = "/tmp/nspr_".$$."_".$jdx."_log.txt";

Better to use something from http://perldoc.perl.org/File/Temp.html to create the filename.  And you should delete it when you're done.

>+        $cmd = "NSPR_LOG_MODULES=DOMLeak:5,DocumentLeak:5,nsDocShellLeak:5 NSPR_LOG_FILE=".$nspr;

This is out of date now.

>+        $cmd .= " EM_RESTART=1";

NO_EM_RESTART=1 ?

>+        $cmd .= " ".$app;
>+        $cmd .= " -profile ".$profile;
>+        $cmd .= " '".$url."'";


>+        $cmd .= " 2>&1 >/tmp/leaksgauge_".$$."_".$jdx;

same thing about temp files.  But why not /dev/null if you're not doing anything with it?

Also leak-gauge, not leaksgauge.

And putting 2>&1 before the redirection rather than after means stderr won't be redirected.

Also, if you're running shell commands, you should at least single-quote the file names (like you do for the URL), although really it's better practice not to build up strings for execution (though that doesn't really matter here).

>+        print `$cmd`;
>+
>+        print "Application: ".$app."\n";
>+        print "URL: ".$url."\n";

Why not just put the variables inside the ""-ed strings?

>+        $cmd = "perl ".$LGPATH."/leak-gauge.pl /tmp/nspr_".$$."_".$jdx."_log.txt";
>+        print `$cmd`;

Same for the filename here.


Sorry for the long delay getting to this.
Attachment #277194 - Flags: review?(dbaron) → review-
(In reply to comment #11)
> (From update of attachment 277194 [details] [diff] [review])
> >+# Software distributed under the License is distributed on an "AS IS" basis,
> >+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
> >+# for the specific language governing rights and limitations under the
> >+# License.
> >+#
> >+# Portions created by the Initial Developer are Copyright (C) 2007
> >+# the Initial Developer. All Rights Reserved.
> 
> You took out a decent-sized chunk of the license header here -- parts that
> you're supposed to fill in.

I'll see what I need to add. The mechanics of adding what needed to be added must not have been clear to me. I'll see.

> >+# installed the extension found at http://www.wykiwyk.com/mozilla/loadAndDie into the profile
> 
> Seems a little odd to have code in our tree depend on code on your personal
> site.

It seems odd to me also. I am not sure if there is an alternative. The testing relies on the fact that after the page finishes loading, the app quits, so that it can re-launch. I have not found another extension which does this, or which can be made to do this. And it seems one has to add this as an extension in order to have this behavior be possible.

I think that the current version of this extension waits for the load event and I am recalling that there is another event which would be better, though it is a bit more complicated to catch the right occurrence of the right event.

> >+if (($ARGV[0] eq "-h") || ($ARGV[0] eq "-help") || ($ARGV[0] eq "--help")) {
> >+    print "usage: perl ./call-leak-gauge.pl <appList> <profile> <urlList>\n";
> >+    exit(0);
> >+}
> >+
> >+$LGPATH = $0;
> >+$LGPATH =~ s/call-leak-gauge.pl$//;
> 
> You're better off using dirname($0) here.  It requires a "use File::Basename;"
> at the top.  (There are other, potentially-more-windows-compatible things here
> too, I think, but dirname is probably good enough.)  (It probably produces a
> result without the trailing /, though.)
> 

Ok. I'll take care of that.

> >+for ($idx = 0; $idx < scalar(@apps); $idx++) {
> 
> $#apps is probably better than scalar(@apps).
> 

ok++

> >+        $cmd = $0." ".$app." ".$profile." ".$url;
> 
> Probably cleaner to put all the variables inside a ""-ed string.
> 

ok++

> >+        $nspr = "/tmp/nspr_".$$."_".$jdx."_log.txt";
> 
> Better to use something from http://perldoc.perl.org/File/Temp.html to create
> the filename.  And you should delete it when you're done.
> 
> >+        $cmd = "NSPR_LOG_MODULES=DOMLeak:5,DocumentLeak:5,nsDocShellLeak:5 NSPR_LOG_FILE=".$nspr;
> 

Thanks for the suggestion. Will do.

> This is out of date now.
> 
> >+        $cmd .= " EM_RESTART=1";
> 
> NO_EM_RESTART=1 ?
> 
> >+        $cmd .= " ".$app;
> >+        $cmd .= " -profile ".$profile;
> >+        $cmd .= " '".$url."'";
> 
> 
> >+        $cmd .= " 2>&1 >/tmp/leaksgauge_".$$."_".$jdx;
> 
> same thing about temp files.  But why not /dev/null if you're not doing
> anything with it?
> 
> Also leak-gauge, not leaksgauge.
> 
> And putting 2>&1 before the redirection rather than after means stderr won't be
> redirected.
> 

I think that the stderr getting swallowed up was on purpose. I believe my thinking was that, if the test fails, then it should just fail and not dump huge gobs of information. Tests can be re-run for more nuanced reporting. Actually, this was a while ago, so I am not positive about this. Hm. I will probably just accept your suggestion here.

> Also, if you're running shell commands, you should at least single-quote the
> file names (like you do for the URL), although really it's better practice not
> to build up strings for execution (though that doesn't really matter here).
> 
> >+        print `$cmd`;
> >+
> >+        print "Application: ".$app."\n";
> >+        print "URL: ".$url."\n";
> 
> Why not just put the variables inside the ""-ed strings?
> 

A long time ago, back in the mists of time, I got in the habit of keeping variable references outside of the "". No idea why now. I have observed that other people put the variable references inside quotes. I just have not seen a reason to change.

> >+        $cmd = "perl ".$LGPATH."/leak-gauge.pl /tmp/nspr_".$$."_".$jdx."_log.txt";
> >+        print `$cmd`;
> 
> Same for the filename here.
> 
> 
> Sorry for the long delay getting to this.
> 

I appreciate your time. I will take your suggestions and submit a new patch.
Does anyone have suggestions the license text? As I looked at what I had done, I saw that I had used a copy of the license header that is found in mozilla/tools/footprint/leak-gauge.pl and I confirmed that this seems to be what is suggested in http://www.mozilla.org/MPL/MPL-1.1.html. Is that page out of date? Is there other info that I should be using?

thanx.
I opened http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-sh in one window and http://www.mozilla.org/MPL/MPL-1.1.html in another and pages down to Exhibit A in the second page and stared at them and I do not see the difference.

And the second URL says:

    NOTE: The text of this Exhibit A may differ slightly from
    the text of the notices in the Source Code files of the
    Original Code. You should use the text of this Exhibit A
    rather than the text found in the Original Code Source Code
    for Your Modifications.

So, I guess my questions is.... Well, I guess there is no question. The license references are just duplicated. Just so everyone is aware of them.
(In reply to comment #15)
> I opened http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-sh in one
> window and http://www.mozilla.org/MPL/MPL-1.1.html in another and pages down to
> Exhibit A in the second page and stared at them and I do not see the
> difference.

The former is specifically the MPG/GPL/LGPL tri-license boilerplate we use for the majority of files shipped in Mozilla products. The latter is a generic multi-license MPL boilerplate.

Neither of them match the license header you have in attachment 277194 [details] [diff] [review] (missing the "original code" line, the "Initial Developer" text is changed, etc.).
Component: Testing → General
Product: Core → Testing
QA Contact: testing → general
You need to log in before you can comment on or make changes to this bug.