316661 - Remove NULL-check before delete

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Hans-Andreas Engel]

Because |delete| checks if its argument is null, we can replace |if (a) delete a;| by |delete a;|.

Comment 1

[Hans-Andreas Engel]

Approximately 60 .cpp files are affected.

Comment 2

[Hans-Andreas Engel]

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]

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]

Attachment: Globally replace |if(a) delete a;| by |delete a;|

Comment 5

[Andrew Schultz]

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]

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]

> 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]

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]

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]

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]

Attachment: Globally replace |if(a) delete a;| by |delete a;|, keep '\t's

Comment 12

[Wayne Mery (:wsmwk)]

bienvenu, are you still best person to review?

Comment 13

[Wayne Mery (:wsmwk)]

Hans-Andreas is "currently not active in the mozilla projects"

Comment 14

[David :Bienvenu]

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]]

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]]

s/ocm/foo

Comment 17

[David :Bienvenu]

(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]]

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]

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]]

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]

you're right, that null out is not needed.

Comment 22

[Ed Morley [:emorley]]

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]]

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]]

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

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

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

Parts A, B & C:
http://dev.philringnalda.com/tbpl/?tree=Try&rev=f88432556f2e

Comment 28

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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]]

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

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]]

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 [:bzbarsky]]

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]]

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

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]]

@ 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) (email my personal email if necessary)]

"delete ocm" doesn't set ocm to null. The original code does.

Comment 37

[Ed Morley [:emorley]]

(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]

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]]

That makes sense, thank you for the explanation :-)

Comment 40

[David :Bienvenu]

(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]]

(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)]

Part A pushed:
http://hg.mozilla.org/mozilla-central/rev/c8d5469bed72

Comment 43

[Ed Morley [:emorley]]

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]]

Sorry spam, but should have clarified the review is just for the jsd changes in the part B patch.

Comment 45

[Brendan Eich [:brendan]]

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]]

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]]

Attachment: 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)]

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]]

(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