1334104 - 'if' statement is unnecessary; deleting null pointer has no effect

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

This is a very easy good first bug for a beginner.

The following if declarations are useless as delete foo; doesn't have any effect if foo is null.

https://dxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp?q=nsPluginHost.cpp&redirect_type=direct#3138
https://dxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp?q=nsPluginHost.cpp&redirect_type=direct#3153

We should just remove the if

Comment 1

[Jason Tran]

Hi Sylvestre. I'm a beginner and I thought this would be a good experience for me to try and have a crack at this. Please let me know if it's still available!

Comment 2

[Muhammed Nagy]

i'm working on it i'm a beginner too.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

I will assign it to the first submitting a patch.
Fyi, bug 1334265 is also a good first bug for a beginner

Comment 4

[Cliff]

Attachment: Bug 1334104 - 'if' statement is unnecessary

Removed unnecessary `if` statements intended to safely delete pointers. Deleting a null pointer has no effect, and the behavior is defined.

Review commit: https://reviewboard.mozilla.org/r/108114/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108114/

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8831582 [details]
Bug 1334104 - 'if' statement is unnecessary

https://reviewboard.mozilla.org/r/108114/#review109192

::: dom/plugins/base/nsPluginHost.cpp:3151
(Diff revision 1)
>        version,
>        (const char* const*)mimetypes,
>        (const char* const*)mimedescriptions,
>        (const char* const*)extensions,
>        mimetypecount, lastmod, fromExtension, true);
> -    if (heapalloced)
> +

You should remove this line

::: dom/plugins/base/nsPluginHost.cpp:3152
(Diff revision 1)
>        (const char* const*)mimetypes,
>        (const char* const*)mimedescriptions,
>        (const char* const*)extensions,
>        mimetypecount, lastmod, fromExtension, true);
> -    if (heapalloced)
> +
>        delete [] heapalloced;

The indentation level is incorrect here

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Jason, Muhammed, if you want another good first bug to start with, please send me an email and I will create and assign one for you.

Comment 7

[Cliff]

Attachment: Bug 1334104 - 'if' statement is unnecessary

Removed unnecessary `if` statements intended to safely delete pointers. Deleting a null pointer has no effect, and the behavior is defined.

Review commit: https://reviewboard.mozilla.org/r/108394/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108394/

Comment 8

[Cliff]

Sylvestre, could you let me know if I properly updated my patch? I see there are now 2 commits. Is there a way I could have just updated my first commit, or is this right?

Comment 9

[Cliff]

Comment on attachment 8831582 [details]
Bug 1334104 - 'if' statement is unnecessary

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108114/diff/1-2/

Comment 10

[Cliff]

(In reply to cliffplaysdrums from comment #8)
> Sylvestre, could you let me know if I properly updated my patch? I see there
> are now 2 commits. Is there a way I could have just updated my first commit,
> or is this right?

Note: I got some help and used histedit to combine the patches. Hopefully it all turned out okay.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8831582 [details]
Bug 1334104 - 'if' statement is unnecessary

Good, now, you have to find a reviewer for this:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction#Step_4_-_Get_your_code_reviewed

Comment 12

[Cliff]

(In reply to Sylvestre Ledru [:sylvestre] from comment #11)
> Comment on attachment 8831582 [details]
> Bug 1334104 - 'if' statement is unnecessary
> 
> Good, now, you have to find a reviewer for this:
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Introduction#Step_4_-_Get_your_code_reviewed

Are you the right person to ask for a review, or is it someone else?

Comment 13

[Jason Tran]

(In reply to Sylvestre Ledru [:sylvestre] from comment #6)
> Jason, Muhammed, if you want another good first bug to start with, please
> send me an email and I will create and assign one for you.

Hi, apologies for the late response if there still is another GFB you could point me to I would like to try tackling it. Thanks again!

Comment 14

[Sylvestre Ledru [:Sylvestre]]

(In reply to cliffplaysdrums from comment #12)
> Are you the right person to ask for a review, or is it someone else?
Should be someone else :)

Comment 15

[Cliff]

(In reply to Sylvestre Ledru [:sylvestre] from comment #14)
> (In reply to cliffplaysdrums from comment #12)
> > Are you the right person to ask for a review, or is it someone else?
> Should be someone else :)

I see the name "Benjamin" frequently around this area of the code. I'm guessing this could be the triage owner, but I'm having trouble finding out what a triage owner is, and in this case, the triage owner is ":bsmedberg" and not "Benjamin" so I don't know if that's a coincidence or a display setting. How do you suggest finding the right reviewer? The first step *does say* to ask your mentor if it's a mentored bug :)

Comment 16

[Sylvestre Ledru [:Sylvestre]]

this is the same person :)
Just ask to bsmedberg
and you should be fine

Comment 17

[Benjamin Smedberg]

Comment on attachment 8831582 [details]
Bug 1334104 - 'if' statement is unnecessary

https://reviewboard.mozilla.org/r/108114/#review113806

Look good to me! thank you. I'll trigger a try build and then autoland if it works.

Comment 18

[Benjamin Smedberg]

NI?myself to keep this on my radar to check try results.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1fe9a4b7b037
'if' statement is unnecessary r=bsmedberg

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1fe9a4b7b037