dreftool - rickg's phase 1 preventative crash maintenance

RESOLVED FIXED

Status

P3
enhancement
RESOLVED FIXED
20 years ago
2 years ago

People

(Reporter: chofmann, Assigned: timeless)

Tracking

({meta})

Trunk
x86
Windows 95
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 13 obsolete attachments)

20.73 KB, text/plain
Details
20.37 KB, text/plain
Details
36.81 KB, text/plain
Details
36.81 KB, text/html
Details
57.02 KB, text/html
Details
74.05 KB, patch
bernd_mozilla
: review+
Details | Diff | Splinter Review
(Reporter)

Description

20 years ago
rickg's full plan posted at
http://www.mozillazine.org/talkback.html?article=551

this deals with phase one.
we need some tools to search the source for possible
problem areas.

The Problem
  This document is intended to briefly address some of the shortcomings
  of our memory management strategy in Seamonkey. Nothing here is particularly
  new; no state of the art design ideas. But it's my
  hope that by applying a few of these patterns we can improve the overall
  state of affairs, thus raising our quality level a bit.

  The impetus for this was a quick survey that I did of the Seamonkey source
  code. A quick look  revealed that we have over 2000 memory allocation
  points in 600+ files. These numbers don't even include the number of calls
  to malloc and it's peers.

  More troubling is the fact that we regularly write code like this:

  nsresult nsMyClass::DoSomething() {
    nsresult theResult=NS_OK;
    nsFoo* aFoo = new nsFoo();  //assume this fails to alloc memory...
    aFoo->WatchMeExplode();
    return theResult;
  }

  There are several problems with this code:

      1.The memory allocation result is never conditioned for failure.
      2.Error propagation of the memory failure is nonexistent.
      3.Object aFoo is dereferenced unconditionally.

   Memory Management Proposal


  I propose that each team allocate 1 day during each Milestone for the
  explicit purpose of systematically reviewing our code in order to resolve
  and improve memory related issues.

  Step 1:

  Memory and resource cleanup and tuning. First, let's review our code and
  add pointer conditioning to make sure we're not dereferencing null pointers.
  This is especially true at memory allocation points like the example given
  above. So we'll rewrite our first example like this:

  nsresult nsMyClass::DoSomething() {
    nsresult theResult=NS_OK;
    nsFoo* aFoo = new nsFoo();
    if(aFoo) {
      theResult=aFoo->NowIWontExplode();
    }
    else theResult=NS_MEMORY_ALLOCATION_ERROR;
    return theResult;
  }

  Note that to be correct, allocation conditioning needs to deal with a few
  subtleties. For example, assume that you've been given (via an API) pointers
  to a few objects that you intend to cache. Then you go to make your cache
 (an nsDeque, for example) and the allocation fails. You have to be
 particularly careful about what you do with the objects you meant to store.
 You have to know, for example, whether you can simply drop them on the floor,
 versus RELEASING them.

 There's also the issue of benign failures, where you wanted extra memory
 (or an instance of a given class), but your algorithm would continue to
 work correctly if even if you can't get the resource you requested.
 Handling benign failures offers a level of complexity all its own.
(Reporter)

Updated

20 years ago
Blocks: 8222

Updated

20 years ago
Target Milestone: M9

Comment 1

20 years ago
Putting on M9 for now to get off Blank Target Milestone.  It this can be
implemented by M8...great...but I think rickg team is busy with M8 fixes right
now.

Comment 2

20 years ago
Created attachment 590 [details]
error log from strict output of RickG error finding utility.

Comment 3

20 years ago
where is this error utility? I'd like to run it myself on specific directories
that I often work in.

Comment 4

20 years ago
Me too!

Comment 5

20 years ago
Hey rickg, how about making the tool's output be lxr links (e.g., "line 336"
under s:/mozilla/layout/base/src/nsNameSpaceManager.cpp could link to
http://lxr.mozilla.org/seamonkey/source/layout/base/src/nsNameSpaceManager.cpp#3
36)?  That would rock for us browsers, although fixers just need to fire up
their favorite editor, not lxr.

/be

Comment 6

20 years ago
Created attachment 740 [details]
Latest report results (worse)

Comment 7

20 years ago
where can i find this error tool so that i can run it over new files before i
check in.

people can't be complaining about the report results getting worse if engineers
don't have the opportunity to check the code on their own.
(Reporter)

Updated

20 years ago
Whiteboard: 9823,9825,9826,9827,9829,9830,9832,9833,9835,9842,9843,9845,9847,9848,9849,9851,9852,9853,9854,9855,9856,9857,9858,9860,9861,9863,9864,9865,9866,9867,9859
(Reporter)

Updated

20 years ago
Whiteboard: 9823,9825,9826,9827,9829,9830,9832,9833,9835,9842,9843,9845,9847,9848,9849,9851,9852,9853,9854,9855,9856,9857,9858,9860,9861,9863,9864,9865,9866,9867,9859

Updated

20 years ago
Summary: rickg's phase 1 preventative crash maintenence. → [META] rickg's phase 1 preventative crash maintenence.

Comment 8

20 years ago
Created attachment 954 [details]
latest error report...

Comment 9

20 years ago
Created attachment 955 [details]
Latest report stored as HTML file (for LXR)

Comment 10

20 years ago
Created attachment 963 [details]
Latest version in HTML suitable for LXR
(Reporter)

Updated

20 years ago
Depends on: 10443

Comment 11

20 years ago
the tool is now availible in mozilla/tools/dreftool.  it's a window's build only
at the moment, but it someone wan't to contribute a unix makefile send it to me
and i'll check it in -- otherwise i'll try to put togeter a unix makefile
sometime down the road.
(Reporter)

Comment 12

20 years ago
Updated Report Results for each day are now being posted at
http://www.mozilla.org/projects/seamonkey/reports/memory-allocation-problems.txt
(Reporter)

Updated

20 years ago
Status: NEW → ASSIGNED
(Reporter)

Updated

20 years ago
Target Milestone: M9 → M10

Updated

20 years ago
Depends on: 9828

Comment 13

20 years ago
I have memerror working on linux, and it should be fairly portable to any
system where configure works.  The original memerror used private, out-of-date
copies of nsCRT and the like, and had some very DOS-specific code for reading
directories.  I ripped all that out and reimplemented directory-handling
using nspr.  To update the source, remove ns*.cpp, ns*.h, CDirectory.cpp,
IDirectory.h, and bufferRoutines.h from tools/dreftool.  Then install
Makefile.in and apply the seven patches that I'm about to attach.  To build
the tool:

1)	Configure & build mozilla.  The tool uses includes from dist/include
	and libs from dist/lib.
2)	Go to the parent directory of the tools directory in your build area,
	and run "/path/to/mozilla/build/autoconf/make-makefile tools/dreftool"
3)	cd to tools/dreftool and make.

memerror is linked dynamically against libxpcom and some nspr libs; I tried
static linking so you wouldn't have to fiddle around with LD_LIBRARY_PATH,
but no joy.

Comment 14

20 years ago
Created attachment 1757 [details]
tools/dreftool/Makefile.in for memerror

Comment 15

20 years ago
Created attachment 1758 [details] [diff] [review]
CCPPTokenizer.cpp patch

Comment 16

20 years ago
Created attachment 1759 [details] [diff] [review]
CCPPTokenizer.h patch

Comment 17

20 years ago
Created attachment 1760 [details] [diff] [review]
CScanner.cpp patch

Comment 18

20 years ago
Created attachment 1761 [details] [diff] [review]
CToken.h patch

Comment 19

20 years ago
Created attachment 1762 [details] [diff] [review]
SharedTypes.h patch

Comment 20

20 years ago
Created attachment 1763 [details] [diff] [review]
main.cpp patch

Comment 21

20 years ago
Created attachment 1764 [details] [diff] [review]
tokens.cpp patch

Comment 22

20 years ago
I suggest that Rick's utility, especially since it works on Linux as well now,
be checked into CVS, and that a description of it be put on Waterson's
Performance/Tools page for everyone to see.
(Reporter)

Updated

20 years ago
Target Milestone: M10 → M11

Comment 23

20 years ago
My C++ is rusty...  If I try to create a new Bar object
from a constructor of class Foo, and the 'new' operator
fails, how do I make the Foo constructor fail?  Since
constructors return void, I don't know how constructors
return a failure status.  Here is the code:

Foo::Foo()
{
    Bar* aBar = new Bar();
    if (!aBar) {
        // what do I do?
    }
}

Comment 24

20 years ago
I am looking at it from a beginners perspective, and I am probably a little more
familiar with C than C++, so take this suggestion with a grain of salt...
Anyway, here goes: why not CHANGE the return type of your function from void to
int, so you CAN return an error condition?  I would have thought that would be
the easiest solution.  After all, on Unix, even main is usually of type int,
returning an error condition back to the operating system.  Returns 0 for
success, and other values signal various errors.

Comment 25

20 years ago
Another idea--if it really isn't critical that the calling function be notified
of errors, you could also consider just saying "return" with no arguments.

Of course, I have a problem with this, because I believe strongly in the saying,
"one entry, one exit."  I don't believe in multiple return statements in a
function.  I think this makes things too confusing, and therefore less
maintainable in the end.

The better solution if you insist on making the return type void would probably
be simply to do something like this:

Foo::Foo()
{
    Bar* aBar = new Bar();
    if (aBar) {
	/* Do your thing, Foo! */
    }
}

This way, the function will silently fail if you fail to allocate aBar.

Comment 26

20 years ago
If you are a hard-core C++ person, I suppose you could also use the "try" and
"throw" keywords.  But then again, I am not much of a C++ person either, so
again, don't take this too seriously.

Anybody else out there more expert want to tell me how it should *really* be
done?
(Reporter)

Updated

19 years ago
Target Milestone: M11 → M12
(Reporter)

Updated

19 years ago
Target Milestone: M12 → M13
(Reporter)

Updated

19 years ago
Target Milestone: M13 → M14

Updated

19 years ago
Keywords: meta

Comment 27

19 years ago
Adding "crash" keyword to all known open crasher bugs.
Keywords: crash
(Reporter)

Updated

19 years ago
Target Milestone: M14 → M16

Comment 28

19 years ago
M16 has been out for a while now, these bugs target milestones need to be 
updated.

Comment 29

18 years ago
tracking bug. rtm- to drop off untriaged crash bug radar.
Keywords: rtm
Whiteboard: [rtm-]

Comment 30

18 years ago
Created attachment 19108 [details]
dreftool new version

Comment 31

18 years ago
Startting from the sources at /tools/dreftool and applying the patches, I
created a new version , which overcomes some of the deficiencies of the original
program, which is still the source of
http://www.mozilla.org/projects/seamonkey/reports/memory-allocation-problems.txt.
The following changes are incorporated:
- correct linkage to nsString.h (which has changed inbetween)
- NS_ENSURE_TRUE is now accepted as a safe method
- html works now correctly also in the deep subdirectories.
- member variables are now also screened.

The last point also warns if
if(mFoo)
{
  mFoo = new bar;
  mFoo->xy
}
where before the if(foo) would suppress a warning. The file's in the zip-file
are tested under Win98. The warning's for the member variables can be suppressed
by -s (sloppy). There is still a problem that the line number may be of by a few
lines. If you find other bogus warnings please file a bug against me.

Comment 32

18 years ago
leger is no longer @netscape. changing qa contact to component's default
QA Contact: leger → chofmann

Updated

18 years ago
No longer depends on: 9849

Comment 33

18 years ago
Milestone 0.8 has been released. We should either resolve this bug or update its
milestone.

Updated

18 years ago
Target Milestone: M16 → ---

Comment 34

18 years ago
Created attachment 30754 [details]
zip archive compiled with the new string library

Comment 35

17 years ago
Created attachment 60077 [details]
updated version
Attachment #30754 - Attachment is obsolete: true
(Assignee)

Comment 36

17 years ago
ack. there's a second nsdeque in the tree.
I'm rewriting the first one (bug 114166), so i'll probably try to remove the one that's here and make dreftool use the xpcom one.
initial nit: makefile.in should be Makefile.in
second nit: the copyright. this code is RickG's. Is he ok with modification and
redistribution? If it's going to go into the tree, it needs to have the MPL
slapped onto it and his blessing given to the licensing.
leaf: the copyright holder is almost certainly netscape.com, not rickg.

/be
timeless: why are you rewriting nsDeque.cpp, pray tell?

/be
Unless the code has "Copyright Rick Gessner" in it, I think we can assume it's
Netscape's copyright. Therefore, the MPL tri-license should be put on it,
because that's the license for all new files in the Mozilla tree (Netscape
employees included.)

Gerv

Comment 42

17 years ago
Gerv,

>Unless the code has "Copyright Rick Gessner" in it, I think we can assume it's
>Netscape's copyright.

this is exactly the problem.

/*================================================================*
	Copyright © 1998 Rick Gessner. All Rights Reserved.
 *================================================================*/

while this code at the time it was checked in was a copy of the mozilla tree.
If I understand Netscape's then-extant work-for-hire employment agreement (and
IANAL), Netscape holds copyright.  Cc'ing mitchell in case she can help, or
refer us to someone who can.

/be

Comment 44

17 years ago
The first question is whether there are licen headers in the code.  It should
have the NPL or MPL license header at the top.  If not, then whoever checked
this in made a mistake.  If there is an NPL or MPL header, then the copyright
holder's name  doesn't matter.  Almost all code in the mozilla tree has a
copyright owner, which contributes unde the terms of our licensing scheme.  So,
I'm assuming that the files don't have any license header, and hte ocpyright
notice is all there is.

It's hard to imagine anything rickg did as a netscape employee not being
Netscape's copyright.  Why there is code in the tree with his personal copyright
is beyond me:perhaps he wrote it before his emloyment?  It seems highly unlikely
he had some arrangement with Netscape where he maintained ownership, although it
is conceivable.  

So if there is an NPL or MPL liense header, I don't see a problem.  If there
isn't, then one could try a couple of different things.  One, ask Rickg himself.
 Second, ask Netscape/AOL Legal to determine whether the "copyright rickg" is
accurate, or whether Netscape held the copyright.  

Mitchell

Comment 45

17 years ago
I got the following reply from Rick, so I will go ahead and insert the triple
licence or is that email not enough?

From: "Rick Gessner" <rick@gessner.com>
To: "'Bernd Mielke'" <bernd.mielke@snafu.de>
Subject: RE: mozilla copyright issue
Date: Wed, 2 Jan 2002 12:20:45 -0800

Feel free to change the file to allow free use of the code.
Rick

-----Original Message-----
From: Bernd Mielke [mailto:bernd.mielke@snafu.de] 
Sent: Wednesday, January 02, 2002 11:20 AM
To: rick@gessner.com
Subject: mozilla copyright issue


Hey Rick,

I am asking for your help, or in other words this is your past striking 
back.  I have maintained your old code from 
http://bugzilla.mozilla.org/show_bug.cgi?id=8227. But now I am not 
allowed to modify the code as you put a rickg copyright notice in the 
file. Could you please lift your copyright so that I can change it to 
the triple licence?

Thanks a lot in advance

Bernd




Comment 46

17 years ago
This OK from Rick looks good to me.  Our form letter has preset language for
responding, but that's because consistency is easier.  Rick g's intent is clear
here, so using the tri-license looks good to me.

Mitchell

Comment 47

16 years ago
By the definitions on <http://bugzilla.mozilla.org/bug_status.html#severity> and
<http://bugzilla.mozilla.org/enter_bug.cgi?format=guided>, crashing and dataloss
bugs are of critical or possibly higher severity.  Only changing open bugs to
minimize unnecessary spam.  Keywords to trigger this would be crash, topcrash,
topcrash+, zt4newcrash, dataloss.
Severity: normal → critical
(Assignee)

Comment 48

15 years ago
Created attachment 135297 [details] [diff] [review]
updated -upN
Attachment #1757 - Attachment is obsolete: true
Attachment #1758 - Attachment is obsolete: true
Attachment #1759 - Attachment is obsolete: true
Attachment #1760 - Attachment is obsolete: true
Attachment #1761 - Attachment is obsolete: true
Attachment #1762 - Attachment is obsolete: true
Attachment #1763 - Attachment is obsolete: true
Attachment #1764 - Attachment is obsolete: true
Attachment #19108 - Attachment is obsolete: true
Attachment #60077 - Attachment is obsolete: true
(Assignee)

Comment 49

15 years ago
Created attachment 135299 [details] [diff] [review]
main.cpp -up - support bonsai (good for branched files) and indicate search root

Comment 50

15 years ago
Comment on attachment 135297 [details] [diff] [review]
updated -upN

Please, add the licence headers, this bug clearly indicates that the code can
go the normal triple licence.
(Assignee)

Comment 51

15 years ago
Created attachment 135303 [details] [diff] [review]
with licenses, bonsai output fixed to handle relative paths

false positive:
 920 aaronl    1.25	nsHTMLComboboxTextFieldAccessible* accessible = 
 921			  new nsHTMLComboboxTextFieldAccessible(this, mDOMNode,
mWeakShell);
 922 aaronl    1.31	*aFirstChild = accessible;
 923			if (! *aFirstChild)
 924 aaronl    1.24	  return NS_ERROR_FAILURE;
 925 aaronl    1.25	accessible->Init();

One might argue in favor of chnaging the code to quiet the test, but
technically the code is correct and dreftool just isn't smart enough.
(Assignee)

Updated

15 years ago
Attachment #135297 - Attachment is obsolete: true
Attachment #135299 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #135303 - Flags: review?(bernd_mozilla)
No one should be changing code to work around limitations in this tool.

/be

Comment 53

15 years ago
Comment on attachment 135303 [details] [diff] [review]
with licenses, bonsai output fixed to handle relative paths

I think the main user of this (aka timeless ;-) ) will be able to handle the
false positive. I would like to get it in now as it is, because I think it is
good enough and also to fix also the licence issue in our tree. 

Timeless you might file a bug against m for the false positive.
Attachment #135303 - Flags: review?(bernd_mozilla) → review+
(Assignee)

Comment 54

15 years ago
Let's limit the focus of this bug to landing a working dreftool. it can still be
used as a meta bug if people like.
Assignee: chofmann → timeless
Severity: critical → enhancement
Status: ASSIGNED → NEW
Keywords: crash
Summary: [META] rickg's phase 1 preventative crash maintenence. → dreftool - rickg's phase 1 preventative crash maintenance
Whiteboard: [rtm-]
(Assignee)

Comment 55

15 years ago
checked in. bug 225532 and bug 225535 filed for the two issues i found while
randomly running dreftool. I also filed and fixed one minor glitch in main.cpp's
bonsai parsing.

I will probably at times ranodmly run dreftool, and review the results before
filing bugs. other people are certainly welcome to run it, but they too should
check the referenced code before filing.

In the meantime, I'm more likely to look into other tools which need help, since
this one seems to work.
Blocks: 225532, 225535
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Assignee)

Updated

15 years ago
Blocks: 225993
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.