Closed Bug 1298709 Opened 8 years ago Closed 8 years ago

Redendant smart ptr

Categories

(Core :: General, defect)

51 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: Sylvestre, Assigned: gmoore, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][lang=C++])

Attachments

(2 files, 1 obsolete file)

Hi,

I would like to take this bug if that's okay. I already made the changes and am now trying to rebuild and create a patch. This is my first bug. :) Thanks.
Assignee: nobody → olucafont6
Okay, I created the patches and uploaded them. 
I built Firefox and it seemed like it was working fine. I'm not really sure what tests I should run though. I tried running Mochitest and the xpcshell tests, but both were taking forever and I wasn't sure if they were the right tests anyway. Let me know, thanks.
Comment on attachment 8787398 [details] [diff] [review]
Removal of redundant call to .get() on smart pointer in nsCategoryManager.cpp

I am not the owner of this code, therefor, you need to find the person you can r+ this.
The doc explains well how to get that.

Just a small comment: calls should be call here because there is only one ;)
Attachment #8787398 - Flags: review?(sledru) → feedback+
Attachment #8787397 - Flags: review?(sledru) → review+
Attachment #8787398 - Attachment description: Removal of redundant calls to .get() on smart pointers in nsCategoryManager.cpp → Removal of redundant call to .get() on smart pointer in nsCategoryManager.cpp
Comment on attachment 8787398 [details] [diff] [review]
Removal of redundant call to .get() on smart pointer in nsCategoryManager.cpp

># HG changeset patch
># User Gregory Moore <olucafont6@yahoo.com>
># Date 1472767516 25200
>#      Thu Sep 01 15:05:16 2016 -0700
># Node ID 41692eeb082c892fa9bc8afe31910546f89424f4
># Parent  12bfd043eb571aece8db0f658b0fe4b0a8965aff
>Removal of redundant call to .get() on smart pointer in nsCategoryManager.cpp
>
>diff --git a/xpcom/components/nsCategoryManager.cpp b/xpcom/components/nsCategoryManager.cpp
>--- a/xpcom/components/nsCategoryManager.cpp
>+++ b/xpcom/components/nsCategoryManager.cpp
>@@ -466,17 +466,17 @@ nsCategoryManager::SizeOfIncludingThis(m
> {
>   size_t n = aMallocSizeOf(this);
> 
>   n += PL_SizeOfArenaPoolExcludingPool(&mArena, aMallocSizeOf);
> 
>   n += mTable.ShallowSizeOfExcludingThis(aMallocSizeOf);
>   for (auto iter = mTable.ConstIter(); !iter.Done(); iter.Next()) {
>     // We don't measure the key string because it's a non-owning pointer.
>-    n += iter.Data().get()->SizeOfExcludingThis(aMallocSizeOf);
>+    n += iter.Data()->SizeOfExcludingThis(aMallocSizeOf);
>   }
> 
>   return n;
> }
> 
> namespace {
> 
> class CategoryNotificationRunnable : public Runnable
you have to upload a new accouchement with the fix and mark the previous one as obsolete
Oh okay, I see. Thanks for the feedback.
Comment on attachment 8787789 [details] [diff] [review]
Removal of redundant call of get() on smart pointer in nsCategoryManager.cpp

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

Thanks for the patch!
Attachment #8787789 - Flags: review?(nfroyd) → review+
Gregory, now, you can add the keyword "checkin-needed" and a sheriff will land the code for you!
Ok cool, sounds good. Do I need to test the build at all first though? Or would it be possible for someone to run it on the try server? Let me know, thanks.
No need, this is a pretty simple change!
Keywords: checkin-needed
Okay great, thanks. I added the keyword.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b00b37cb64ef
Remove redundant calls to .get() on smart pointers in nsView.cpp. r=sledru
https://hg.mozilla.org/integration/mozilla-inbound/rev/6433dde43078
Remove redundant call of get() on smart pointer in nsCategoryManager.cpp. r=froydnj
Keywords: checkin-needed
(In reply to Sylvestre Ledru [:sylvestre] from comment #13)
> No need, this is a pretty simple change!

Remember how while you were typing that, there was an earthquake, a lightning strike, and a tornado, all at once, and you thought "wow, what a coincidence!"? Not a coincidence.

Backed out the nsView.cpp patch in https://hg.mozilla.org/integration/mozilla-inbound/rev/0f431005c078 for static analysis' complaint in https://treeherder.mozilla.org/logviewer.html#?job_id=35341252&repo=mozilla-inbound
(In reply to Phil Ringnalda (:philor) from comment #16)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #13)
> > No need, this is a pretty simple change!
> 
> Remember how while you were typing that, there was an earthquake, a
> lightning strike, and a tornado, all at once, and you thought "wow, what a
> coincidence!"? Not a coincidence.
> 
> Backed out the nsView.cpp patch in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0f431005c078 for
> static analysis' complaint in
> https://treeherder.mozilla.org/logviewer.html#?job_id=35341252&repo=mozilla-
> inbound

Definitely this patch shouldn't be added to our code. Our static analysis tool has a checker that forbids the use of Addref and Release on return addresses or references. This is the case here, the pointer that's used is:

>>nsCOMPtr<nsIWidget> mWindow;

The wrapper for nsIWidget is nsCOMPtr that it's definition for operator-> is:

>>  nsISupports* operator->() const MOZ_NO_ADDREF_RELEASE_ON_RETURN
>>  {
>>    MOZ_ASSERT(mRawPtr != 0,
>>               "You can't dereference a NULL nsCOMPtr with operator->().");
>>    return get();
>>  }

In this context the annotation MOZ_NO_ADDREF_RELEASE_ON_RETURN forbids calling AddRef or Release. These functions are called on pointer object inside the container, like Release gets called in destructor of container or in overloaded assignment operator. in  In general get() is used to obtain a castable pointer T* or to resolve ambiguity.
https://hg.mozilla.org/mozilla-central/rev/6433dde43078
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Ah ah philor, indeed, shame on me :p

I marked the nsView.cpp change as false positive. cf comment #17
You need to log in before you can comment on or make changes to this bug.