Last Comment Bug 316661 - Remove NULL-check before delete
: Remove NULL-check before delete
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: mozilla7
Assigned To: Ed Morley [:emorley]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-11-15 20:33 PST by Hans-Andreas Engel
Modified: 2011-06-07 07:51 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Globally replace |if(a) delete a;| by |delete a;| (66.98 KB, patch)
2005-11-15 20:39 PST, Hans-Andreas Engel
no flags Details | Diff | Splinter Review
Globally replace |if(a) delete a;| by |delete a;|, keep '\t's (66.91 KB, patch)
2005-11-16 11:01 PST, Hans-Andreas Engel
no flags Details | Diff | Splinter Review
Part A: For mozilla-central (20.97 KB, patch)
2011-05-08 05:04 PDT, Ed Morley [:emorley]
no flags Details | Diff | Splinter Review
Part B: Changes to js/, for landing on TM (3.33 KB, patch)
2011-05-08 05:06 PDT, Ed Morley [:emorley]
paul.biggar: review+
brendan: review+
bzbarsky: review+
Details | Diff | Splinter Review
Part C: Unnecessary null outs; for m-c (1.63 KB, patch)
2011-05-08 05:50 PDT, Ed Morley [:emorley]
benjamin: review-
Details | Diff | Splinter Review
Part A: For mozilla-central (21.06 KB, patch)
2011-05-08 16:14 PDT, Ed Morley [:emorley]
benjamin: review+
Details | Diff | Splinter Review
Part A: For mozilla-central [checked-in] (21.07 KB, patch)
2011-05-16 07:47 PDT, Ed Morley [:emorley]
emorley: review+
Details | Diff | Splinter Review
Part B: Changes to js/, for landing on TM [checked in] (2.87 KB, patch)
2011-06-01 03:07 PDT, Ed Morley [:emorley]
emorley: review+
Details | Diff | Splinter Review

Description Hans-Andreas Engel 2005-11-15 20:33:12 PST
Because |delete| checks if its argument is null, we can replace |if (a) delete a;| by |delete a;|.
Comment 1 Hans-Andreas Engel 2005-11-15 20:33:36 PST
Approximately 60 .cpp files are affected.
Comment 2 Hans-Andreas Engel 2005-11-15 20:34:36 PST
I stumbled over this when I wanted to use |if (a) delete a;| but David pointed out that |delete| already checks for null (Bug 315975 Comment 12).
Comment 3 Hans-Andreas Engel 2005-11-15 20:38:05 PST
Basically, this change can be implemented by running

find . -name \*.cpp | perl -0 -e '@ARGV = split(/\s+/,<>);  while(<>){ '      \
 -e 'if(s/if\s*\(\s*([_a-zA-Z0-9]+)\s*\)\s*delete\s+\1[ \t]*/delete $1/gs  )' \
 -e '{ print "$ARGV\n"; open OUT,">",$ARGV or die "cannot write to $ARGV\n";' \
 -e 'print OUT;} }' | tee changed_files
cvs diff -up8N `cat changed_files`
Comment 4 Hans-Andreas Engel 2005-11-15 20:39:32 PST
Created attachment 203220 [details] [diff] [review]
Globally replace |if(a) delete a;| by |delete a;|
Comment 5 Andrew Schultz 2005-11-15 22:11:04 PST
Comment on attachment 203220 [details] [diff] [review]
Globally replace |if(a) delete a;| by |delete a;|

watch out for tabs (also in most of the mailnews files and nsStorageStream.cpp)

>RCS file: /cvsroot/mozilla/gfx/src/photon/nsRenderingContextPh.cpp,v
>-	if( mTranMatrix ) 
>-		delete mTranMatrix;
>+  delete mTranMatrix;
Comment 6 Hans-Andreas Engel 2005-11-15 22:32:00 PST
In an extra effort, I removed the tabs the modified lines and replaced them according to the emacs-parameters of the corresponding .cpp-file.

For example, |gfx/src/photon/nsRenderingContextPh.cpp| defines
   Mode: C++; tab-width: 2; indent-tabs-mode: nil;  (...)
thus, I used 2 spaces for each tab and replaced |\tdelete mTranMatrix;| by |  delete mTranMatrix;|.  Indeed, the patch file might look somewhat weird when opened in mozilla, because mozilla typically displays tabs as 6 spaces.

I thought that new code lines should not contain any tabs, such that all tabs are gradually removed from the code.  Please correct me if I am wrong; I would be happy to produce a version with tabs (this is much easier to automatically generate anyway).
Comment 7 Karsten Düsterloh 2005-11-15 22:38:25 PST
> I thought that new code lines should not contain any tabs, such that all tabs
> are gradually removed from the code.

Correct. Shoot 'em where ye stumble upon 'em!
Comment 8 Hans-Andreas Engel 2005-11-15 22:58:13 PST
On the "Mozilla Coding Style Guide,"  
   http://www.mozilla.org/hacking/mozilla-style-guide.html
I found a link to "JST Review Simulacrum,"
   http://www.johnkeiser.com/cgi-bin/jst-review-cgi.pl
which lists the error "W - (...) tabs on the line (...)."

Thus, the Florida rule for tabs (shoot 'em, see comment above) also seems to apply there.
Comment 9 Andrew Schultz 2005-11-16 07:25:05 PST
also in http://www.mozilla.org/hacking/mozilla-style-guide.html

> Whitespace: No tabs. No whitespace at the end of a line.

but right before that is:
> Use the prevailing style in a file or module, or ask the owner, if you are on 
> someone else's turf. Module owner rules all.

Practically speaking, most of those files could be converted to no-tabs and no one would complain, but if you only convert one line then it looks bad and is hard to read.
Comment 10 Hans-Andreas Engel 2005-11-16 10:57:29 PST
Adrew, thank you very much from clarifying the tab issue and thereby saving me from the wrath of the file/module owners.

The only file where I should remove tabs is xpcom/io/nsStorageStream.cpp, and indeed I should keep the tabs in all other files.
Comment 11 Hans-Andreas Engel 2005-11-16 11:01:24 PST
Created attachment 203286 [details] [diff] [review]
Globally replace |if(a) delete a;| by |delete a;|, keep '\t's
Comment 12 Wayne Mery (:wsmwk, NI for questions) 2009-03-11 09:36:48 PDT
bienvenu, are you still best person to review?
Comment 13 Wayne Mery (:wsmwk, NI for questions) 2009-03-16 04:00:44 PDT
Hans-Andreas is "currently not active in the mozilla projects"
Comment 14 David :Bienvenu 2009-03-16 12:52:03 PDT
Comment on attachment 203286 [details] [diff] [review]
Globally replace |if(a) delete a;| by |delete a;|, keep '\t's

many of the mailnews ones were already fixed; I can't review the mozilla ones
Comment 15 Ed Morley [:emorley] 2011-05-07 15:59:37 PDT
The existing patch had bitrotted significantly, so am starting from scratch. The perl/regex incantations in comment 3 proved invaluable, thanks!

However, it only covers:
|if (foo) delete foo;|
...and not 
|if (foo) { delete foo; }|

The following covers the second case (only; I ran after the comment 3 version to cover both cases):

find . -name \*.cpp | perl -0 -e '@ARGV = split(/\s+/,<>);  while(<>){ '      \
 -e 'if(s/if\s*\(\s*([_a-zA-Z0-9]+)\s*\)\s*\{\s*delete\s+\1(.).*?\}[ \t]*/delete $1\2/gs  )' \
 -e '{ print "$ARGV\n"; open OUT,">",$ARGV or die "cannot write to $ARGV\n";' \
 -e 'print OUT;} }'

This new regex also catches anything else that is inside the { delete foo; } block (which I didn't imagine would contain anything else, hence simplifying the regex) - but in fact there are several instances of:
> if (foo) {
>   delete ocm;
>   foo = nsnull;
> }

eg: http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/src/nsOperaProfileMigrator.cpp#630
and
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFontconfigUtils.cpp#63
...plus many more.

Isn't the |foo = nsnull| redundant? (And so safe to be removed). Or am I misunderstanding what delete does and so these cases need to be left as is?

Thanks!
Comment 16 Ed Morley [:emorley] 2011-05-07 16:00:33 PDT
s/ocm/foo
Comment 17 David :Bienvenu 2011-05-07 16:02:09 PDT
(In reply to comment #15)

> Isn't the |foo = nsnull| redundant? (And so safe to be removed). Or am I
> misunderstanding what delete does and so these cases need to be left as is?
thx for continuing this work. No, foo = null; is not redundant; nulling out the pointer is the way a lot of code knows that it needs to re-allocate the pointer. Otherwise, we could end up using freed memory, which leads to lots of crashes and potential security exploits.
Comment 18 Ed Morley [:emorley] 2011-05-07 16:24:49 PDT
Thanks for the clarification, I'm kind of learning c/c++ as I go here (something that comment 15 no doubt made apparent!) and I thought delete might possibly be nulling out the pointer at the same time, which it's obviously not.

For these cases then:

> if (foo) {
>   delete ocm;
>   foo = nsnull;
> }

Should they be left verbatim, or changed to:

> delete ocm;
> foo = nsnull;

I'm presuming the former? (Since the potentially unnecessary null check is outweighed by the chance of doing unnecessary |foo = nsnull|s ?)
Comment 19 David :Bienvenu 2011-05-07 16:30:10 PDT
I think the example would be

if (foo) {
  delete foo;
  foo = nsnull
}

I assume we're talking about foo the whole time;

I don't particularly care which way we go - probably the former is more efficient, though the latter makes the code easier to read.
Comment 20 Ed Morley [:emorley] 2011-05-07 16:44:52 PDT
Sorry bad copy and paste from my original typo in comment 15, yeah foo all the time.

Again, at the risk of sounding stupid (but I'd rather learn why, so I'm going to say this anyway), but why aren't all the instances of delete followed by nulling out the pointer? 

My first thought was that for cases where the variable is never used again, it was not deemed necessary - but then there are places like this:
http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/src/nsOperaProfileMigrator.cpp#630

...which still nulls out the pointer even though the next line is a return, and the variable isn't used again...?
Comment 21 David :Bienvenu 2011-05-07 17:47:55 PDT
you're right, that null out is not needed.
Comment 22 Ed Morley [:emorley] 2011-05-07 18:18:50 PDT
So would you like me to remove it (and any others that are not needed either) in this patch as well?
Comment 23 Ed Morley [:emorley] 2011-05-08 04:58:30 PDT
Ok, on the advice of Hans-Andreas Engel, I will remove ones I can, but I'll separate out into a second patch for easier review.

The regex above missed a few cases (eg arrays, since the [i] didn't match the [_a-zA-Z0-9]+ ), so I've revised them slightly...

1st pass: (for the |if (foo) delete foo;| case)
s/if[ \t]*\([ \t]*([_a-zA-Z0-9\[\]]+)[ \t]*\)\s*delete[ \t]+\1/delete $1/gs

2nd pass: (for the |if (foo) { delete foo; }| case)
s/if[ \t]*\([ \t]*([_a-zA-Z0-9\[\]]+)[ \t]*\)\s*\{\s*delete[ \t]+\1;\s*\}/delete $1;/gs

After manually reviewing the output, I reverted a false positive to an if-else delete block in xpcom/ds/nsCheapSets.cpp. A double check using regex that deliberately catches the whole if {} block, even if it contains other lines, showed up a few with comments that were safe to include as well.

There were changes made to files in hunspell and ANGLE; but had to revert given from upstream. Do you think it's worth filing bugs for them upstream?
Comment 24 Ed Morley [:emorley] 2011-05-08 05:04:16 PDT
Created attachment 530910 [details] [diff] [review]
Part A: For mozilla-central

Remove null check before delete in everything other than js/; for landing on m-c.
Comment 25 Ed Morley [:emorley] 2011-05-08 05:06:36 PDT
Created attachment 530911 [details] [diff] [review]
Part B: Changes to js/, for landing on TM

Remove null check before delete from js/*; for landing on tracemonkey.
Comment 26 Ed Morley [:emorley] 2011-05-08 05:50:55 PDT
Created attachment 530914 [details] [diff] [review]
Part C: Unnecessary null outs; for m-c

Remove unnecessary null outs from:
- browser/components/migration/src/nsOperaProfileMigrator.cpp
http://mxr.mozilla.org/mozilla-central/search?string=ocm&find=/browser/components/migration/
- xpcom/build/nsXPComInit.cpp
http://mxr.mozilla.org/mozilla-central/search?string=sIOThread&find=/xpcom/
Comment 27 Ed Morley [:emorley] 2011-05-08 06:43:25 PDT
Parts A, B & C:
http://dev.philringnalda.com/tbpl/?tree=Try&rev=f88432556f2e
Comment 28 :Ms2ger 2011-05-08 10:03:42 PDT
Comment on attachment 530910 [details] [diff] [review]
Part A: For mozilla-central

>diff --git a/layout/mathml/nsMathMLChar.cpp b/layout/mathml/nsMathMLChar.cpp
>--- a/layout/mathml/nsMathMLChar.cpp
>+++ b/layout/mathml/nsMathMLChar.cpp
>@@ -1767,17 +1767,17 @@ nsMathMLChar::ComposeChildren(nsPresCont
>   nsMathMLChar* last = this;
>   while ((i < count) && last->mSibling) {
>     i++;
>     last = last->mSibling;
>   }
>   while (i < count) {
>     child = new nsMathMLChar(this);
>     if (!child) {
>-      if (mSibling) delete mSibling; // don't leave a dangling list ...
>+      delete mSibling; // don't leave a dangling list ...
>       mSibling = nsnull;
>       return NS_ERROR_OUT_OF_MEMORY;
>     }

new can't fail, so please remove the entire |if (!child)| block.
Comment 29 Ed Morley [:emorley] 2011-05-08 16:14:22 PDT
Created attachment 530960 [details] [diff] [review]
Part A: For mozilla-central

Part A updated for comment 28 feedback.

Just going to have to pick random people to review these, since not really sure who best for multi-component changes.
Comment 30 Paul Biggar 2011-05-08 17:07:35 PDT
Comment on attachment 530911 [details] [diff] [review]
Part B: Changes to js/, for landing on TM

Review of attachment 530911 [details] [diff] [review]:
-----------------------------------------------------------------

We don't actually own half this stuff, so this r+ is only for jsweakmap.cpp.

(See https://wiki.mozilla.org/Modules for hints about reviewers - if you're lucky it'll be in date ;). I'm not sure if jsd uses the tm tree, so ask that too.)

::: js/jsd/jsd_xpc.cpp
@@ -1020,5 @@
>  {
>      DEBUG_DESTROY ("jsdScript", gScriptCount);
> -    if (mFileName)
> -        delete mFileName;
> -    if (mFunctionName)

JS team owns js/src, not js/jsd. Try brendan or timeless.

::: js/src/jsweakmap.cpp
@@ -296,5 @@
>  WeakMap::finalize(JSContext *cx, JSObject *obj)
>  {
>      WeakMap *table = fromJSObject(obj);
> -    if (table)
> -        delete table;

I thought from reading this that fromJSObject might be fallible in some way and that the check had some semantic meaning, but it doesn't look like it. So yes, remove this check.

::: js/src/xpconnect/src/xpcjsruntime.cpp
@@ -248,5 @@
>  {
> -    if (waiverWrapperMap)
> -        delete waiverWrapperMap;
> -    if (expandoMap)
> -        delete expandoMap;

And this isn't owned by JS either - xpconnect is it's own module. Try gal, mrbkap or bz.
Comment 31 Ed Morley [:emorley] 2011-05-09 05:51:11 PDT
Comment on attachment 530911 [details] [diff] [review]
Part B: Changes to js/, for landing on TM

Brendan, please may you review the jsd part of this patch. Also, do jsd changes land on m-c or TM?

Boris, please may you review the xpconnect part. As above, m-c or TM?

Thanks!
Comment 32 Boris Zbarsky [:bz] 2011-05-09 08:21:11 PDT
Comment on attachment 530911 [details] [diff] [review]
Part B: Changes to js/, for landing on TM

r=me on the xpconnect parts; I don't think it matters where they land.
Comment 33 Ed Morley [:emorley] 2011-05-16 07:47:31 PDT
Created attachment 532637 [details] [diff] [review]
Part A: For mozilla-central [checked-in]

Only changes are updating to tip and adding r=bsmedberg to commit message.
Carrying forwards r+.

Part A ready to check in.
Comment 34 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-05-16 09:54:17 PDT
Comment on attachment 530914 [details] [diff] [review]
Part C: Unnecessary null outs; for m-c

>diff --git a/browser/components/migration/src/nsOperaProfileMigrator.cpp b/browser/components/migration/src/nsOperaProfileMigrator.cpp

>-  if (ocm) {
>-    delete ocm;
>-    ocm = nsnull;
>-  }
>+  delete ocm;

That's not equivalent, and might cause a double-delete...

>diff --git a/xpcom/build/nsXPComInit.cpp b/xpcom/build/nsXPComInit.cpp

>-    if (sIOThread) {
>-        delete sIOThread;
>-        sIOThread = nsnull;
>-    }
>+    delete sIOThread;

Here also!
Comment 35 Ed Morley [:emorley] 2011-05-16 13:22:37 PDT
@ bsmedberg

(In reply to comment #34)
> That's not equivalent, and might cause a double-delete...

The changes in the part C patch deliberately remove the =nsnull part, since they appeared unneeded - which then just leaves the null check before delete, which is removed like in the part A patch.

Or have I misunderstood comment 20 through to comment 23? (If so, sorry for the confusion, I'm still quite new to C++!)
Comment 36 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-16 16:10:59 PDT
"delete ocm" doesn't set ocm to null. The original code does.
Comment 37 Ed Morley [:emorley] 2011-05-17 03:39:49 PDT
(In reply to comment #36)
> "delete ocm" doesn't set ocm to null. The original code does.

Yeah I realise delete doesn't set ocm to null, but comment 21 implied that particular null out wasn't needed, since the variable was never used again, which is why I deliberately removed it. Or is comment 21 mistaken?
Comment 38 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-05-17 05:50:05 PDT
In general I prefer to null out members when they are deleted whether it is necessary or not: the null costs almost nothing and is good defensive practice.
Comment 39 Ed Morley [:emorley] 2011-05-17 06:10:19 PDT
That makes sense, thank you for the explanation :-)
Comment 40 David :Bienvenu 2011-05-17 06:53:22 PDT
(In reply to comment #39)
> That makes sense, thank you for the explanation :-)

Except that ocm is a local variable, not a member variable. But someone could add code later in that function that tried to use the local variable, I guess.
Comment 41 Jeff Walden [:Waldo] (remove +bmo to email) 2011-05-17 10:53:42 PDT
(In reply to comment #40)
> Except that ocm is a local variable, not a member variable.

And in that case defense-in-depth hurts even less, because any decent optimizing compiler will remove the null-write for you.  (Although if that's the case, nulling it out seems purely nugatory, because it won't /actually/ be nulled out.)  Not that I actually much care one way or the other, to be clear.
Comment 42 Mounir Lamouri (:mounir) 2011-05-18 02:49:47 PDT
Part A pushed:
http://hg.mozilla.org/mozilla-central/rev/c8d5469bed72
Comment 43 Ed Morley [:emorley] 2011-05-23 12:35:45 PDT
Brendan, ping for review (four line change) - thanks :-)

(In reply to comment #31)
> Comment on attachment 530911 [details] [diff] [review] [review]
> Part B: Changes to js/, for landing on TM


Also:
> do jsd changes land on m-c or TM?
Comment 44 Ed Morley [:emorley] 2011-05-23 12:38:02 PDT
Sorry spam, but should have clarified the review is just for the jsd changes in the part B patch.
Comment 45 Brendan Eich [:brendan] 2011-05-31 13:51:29 PDT
Comment on attachment 530911 [details] [diff] [review]
Part B: Changes to js/, for landing on TM

>diff --git a/js/src/jsweakmap.cpp b/js/src/jsweakmap.cpp
>--- a/js/src/jsweakmap.cpp
>+++ b/js/src/jsweakmap.cpp
>@@ -291,18 +291,17 @@ WeakMap::sweep(JSContext *cx)
> 
>     rt->gcWeakMapList = NULL;
> }
> 
> void
> WeakMap::finalize(JSContext *cx, JSObject *obj)
> {
>     WeakMap *table = fromJSObject(obj);
>-    if (table)
>-        delete table;
>+    delete table;
> }

This hunk is no longer correct -- see bug 655368.

/be
Comment 46 Ed Morley [:emorley] 2011-05-31 14:50:03 PDT
Thanks for the heads up - looks like that landed in the 3 weeks since review was requested. I'll take that bit out when I update to tip :-)
Comment 47 Ed Morley [:emorley] 2011-06-01 03:07:51 PDT
Created attachment 536555 [details] [diff] [review]
Part B: Changes to js/, for landing on TM [checked in]

Updated to TM tip, removed the now not applicable hunk (comment 45 and comment 46).

No other changes, carrying forwards r+.

For whomever pushes this, it needs to land on tracemonkey + it has passed try (comment 27).

NB:
Comment 48 Phil Ringnalda (:philor, back in August) 2011-06-05 09:57:28 PDT
Comment on attachment 536555 [details] [diff] [review]
Part B: Changes to js/, for landing on TM [checked in]

http://hg.mozilla.org/tracemonkey/rev/38f994208a46
Comment 49 Ed Morley [:emorley] 2011-06-07 07:51:05 PDT
(In reply to comment #48)
> Comment on attachment 536555 [details] [diff] [review] [review]
> Part B: Changes to js/, for landing on TM [checked in]
> 
> http://hg.mozilla.org/tracemonkey/rev/38f994208a46

http://hg.mozilla.org/mozilla-central/rev/38f994208a46

Note You need to log in before you can comment on or make changes to this bug.