8227 - dreftool - rickg's phase 1 preventative crash maintenance

Status: RESOLVED FIXED

(Core Graveyard :: Tracking, enhancement, P3)

Description

[chris hofmann]

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.

Comment 1

[leger]

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

[rickg]

Attachment: error log from strict output of RickG error finding utility.

Comment 3

[Alec Flett]

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

Comment 4

[Daniel Roberts]

Me too!

Comment 5

[brendan-obsolete]

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

[rickg]

Attachment: Latest report results (worse)

Comment 7

[tague]

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.

Comment 8

[rickg]

Attachment: latest error report...

Comment 9

[rickg]

Attachment: Latest report stored as HTML file (for LXR)

Comment 10

[rickg]

Attachment: Latest version in HTML suitable for LXR

Comment 11

[tague]

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.

Comment 12

[chris hofmann]

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

Comment 13

[Kenneth Herron]

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

[Kenneth Herron]

Attachment: tools/dreftool/Makefile.in for memerror

Comment 15

[Kenneth Herron]

Attachment: CCPPTokenizer.cpp patch

Comment 16

[Kenneth Herron]

Attachment: CCPPTokenizer.h patch

Comment 17

[Kenneth Herron]

Attachment: CScanner.cpp patch

Comment 18

[Kenneth Herron]

Attachment: CToken.h patch

Comment 19

[Kenneth Herron]

Attachment: SharedTypes.h patch

Comment 20

[Kenneth Herron]

Attachment: main.cpp patch

Comment 21

[Kenneth Herron]

Attachment: tokens.cpp patch

Comment 22

[Daniel Roberts]

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.

Comment 23

[Wan-Teh Chang]

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

[Daniel Roberts]

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

[Daniel Roberts]

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

[Daniel Roberts]

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?

Comment 27

[leger]

Adding "crash" keyword to all known open crasher bugs.

Comment 28

[Mike]

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

Comment 29

[Jon Granrose]

tracking bug. rtm- to drop off untriaged crash bug radar.

Comment 30

[Bernd]

Attachment: dreftool new version

Comment 31

[Bernd]

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

[Dawn Endico]

leger is no longer @netscape. changing qa contact to component's default

Comment 33

[Oliver Klee]

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

Comment 34

[Bernd]

Attachment: zip archive compiled with the new string library

Comment 35

[Bernd]

Attachment: updated version

Comment 36

[timeless]

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.

Comment 37

[Daniel (Leaf) Nunes]

initial nit: makefile.in should be Makefile.in

Comment 38

[Daniel (Leaf) Nunes]

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.

Comment 39

[Brendan Eich [:brendan]]

leaf: the copyright holder is almost certainly netscape.com, not rickg.

/be

Comment 40

[Brendan Eich [:brendan]]

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

/be

Comment 41

[Gervase Markham [:gerv]]

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

[Bernd]

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.

Comment 43

[Brendan Eich [:brendan]]

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

[Mitchell Baker]

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

[Bernd]

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" <[e-mail]>
To: "'Bernd Mielke'" <[e-mail]>
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:[e-mail]] 
Sent: Wednesday, January 02, 2002 11:20 AM
To: [e-mail]
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

[Mitchell Baker]

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

[Brant Gurganus]

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.

Comment 48

[timeless]

Attachment: updated -upN

Comment 49

[timeless]

Attachment: main.cpp -up - support bonsai (good for branched files) and indicate search root

Comment 50

[Bernd]

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.

Comment 51

[timeless]

Attachment: 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.

Comment 52

[Brendan Eich [:brendan]]

No one should be changing code to work around limitations in this tool.

/be

Comment 53

[Bernd]

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.

Comment 54

[timeless]

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.

Comment 55

[timeless]

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.