Replace NS_IF_ADDREF(*result = x) with x.forget(result) where possible in mailnews and remove non-standard use of Addref()/Release()

RESOLVED FIXED in Thunderbird 57.0

Status

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

Trunk
Thunderbird 57.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(22 attachments, 25 obsolete attachments)

12.71 KB, patch
aceman
: review+
Details | Diff | Splinter Review
9.56 KB, patch
aceman
: review+
Details | Diff | Splinter Review
10.71 KB, patch
aceman
: review+
Details | Diff | Splinter Review
8.05 KB, patch
aceman
: review+
Details | Diff | Splinter Review
9.74 KB, patch
aceman
: review+
Details | Diff | Splinter Review
13.00 KB, patch
aceman
: review+
Details | Diff | Splinter Review
14.25 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
1.86 KB, patch
aceman
: review+
Details | Diff | Splinter Review
14.42 KB, patch
aceman
: review+
Details | Diff | Splinter Review
30.85 KB, patch
aceman
: review+
aceman
: feedback+
Details | Diff | Splinter Review
24.20 KB, patch
aceman
: review+
aceman
: feedback+
Details | Diff | Splinter Review
8.68 KB, patch
aceman
: review+
Details | Diff | Splinter Review
34.58 KB, patch
aceman
: review+
Details | Diff | Splinter Review
14.60 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
10.84 KB, patch
aceman
: review+
Details | Diff | Splinter Review
37.35 KB, patch
aceman
: review+
Details | Diff | Splinter Review
17.61 KB, patch
jorgk
: review+
aceman
: review+
Details | Diff | Splinter Review
12.09 KB, patch
aceman
: review+
Details | Diff | Splinter Review
57.17 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
2.72 KB, patch
Details | Diff | Splinter Review
3.68 KB, patch
Details | Diff | Splinter Review
1.79 KB, patch
Details | Diff | Splinter Review
Replace NS_IF_ADDREF(*result = x) with x.forget(result) where possible in mailnews:

Historically .forget() was introduced for nsCOMPtr quite late 2007 in bug 392493. Bug 190746 already made a case for it in 2003.

The replacement can take place where x is destroyed when the function returns. It must not take place where x is the member variable of another object, so:

Replace:
nsCOMPtr<nsIXXX> x;
...
NS_IF_ADDREF(*result = x);

Do not replace
NS_IF_ADDREF(*result = mMember);

This is of course a tedious exercise since many call sites need to be inspected. Of course, things like |NS_IF_ADDREF(*_retval = mURL);| do not need to be inspected, since from the naming it is clear that we have a "no-replace" case.

Others, like |NS_IF_ADDREF(*aSelection);| do need to be inspected, since one can't see in DXR what is assigned. Perhaps it would be good to move the previous assignment into the parenthesis, so in this example from nsMsgDBView.cpp#1023: |NS_IF_ADDREF(*aSelection = mTreeSelection);|

There are plenty of candidates for replacement, for example:
https://dxr.mozilla.org/comm-central/rev/567601fc64d9e8cd4f2e417cf35d90d4cc48140f/mailnews/base/search/src/nsMsgFilterService.cpp#236
https://dxr.mozilla.org/comm-central/rev/567601fc64d9e8cd4f2e417cf35d90d4cc48140f/mailnews/addrbook/src/nsAbMDBDirectory.cpp#324

Aryx, I read somewhere that you do such jobs for relaxation, so perhaps you're interested ;-)
Flags: needinfo?(aryx.bugmail)
Posted patch 1340972-addref-compose.patch (obsolete) — Splinter Review
Here's my new playground to create a stream of patches ;-)
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Flags: needinfo?(aryx.bugmail)
Attachment #8906810 - Flags: review?(acelists)
Comment on attachment 8906810 [details] [diff] [review]
1340972-addref-compose.patch

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

Yeah, can be nice cleanup, but let's not create new crashes or leaks ;)
Attachment #8906810 - Flags: review?(acelists) → review+
Posted patch 1340972-addref-local.patch (obsolete) — Splinter Review
Attachment #8906815 - Flags: review?(acelists)
Sorry, forgot to visit the NS_ADDREF()s as well.
Attachment #8906815 - Attachment is obsolete: true
Attachment #8906815 - Flags: review?(acelists)
Attachment #8906819 - Flags: review?(acelists)
Comment on attachment 8906815 [details] [diff] [review]
1340972-addref-local.patch

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

::: mailnews/local/src/nsLocalUtils.cpp
@@ +110,3 @@
>    nsCOMPtr<nsIMsgIncomingServer> server;
>    rv = nsGetMailboxServer(uriStr, getter_AddRefs(server));
> +  server.forget(aResult);

Does this also work if server is null? Is that null inside the container nsCOMPtr object which will have the .forget() method always and it will work?

::: mailnews/local/src/nsNoneService.cpp
@@ +63,5 @@
>      {
>          rv = NS_SetPersistentFile(PREF_MAIL_ROOT_NONE_REL, PREF_MAIL_ROOT_NONE, localFile);
>          NS_ASSERTION(NS_SUCCEEDED(rv), "Failed to set root dir pref.");
>      }
>          

Those ugly spaces :)

::: mailnews/local/src/nsRssService.cpp
@@ +43,2 @@
>      return NS_OK;
>  

Kill this empty line and the spaces above.
Attachment #8906815 - Attachment is obsolete: false
Comment on attachment 8906819 [details] [diff] [review]
1340972-addref-local.patch (v2).

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

So there surely is a difference between NS_ADDREF and NS_IF_ADDREF.
And no touching of patches while review is in progress ;)
Sorry for doing this for you, now also visiting NS_ADDREF().
Attachment #8906810 - Attachment is obsolete: true
Attachment #8906815 - Attachment is obsolete: true
Attachment #8906824 - Flags: review?(acelists)
Only white-space fixes as requested. I'll answer your questions in the next comment.
Attachment #8906819 - Attachment is obsolete: true
Attachment #8906819 - Flags: review?(acelists)
Attachment #8906825 - Flags: review?(acelists)
Yes, there is a difference between NS_ADDREF and NS_IF_ADDREF:

http://searchfox.org/mozilla-central/source/xpcom/base/nsISupportsUtils.h#22
#define NS_ADDREF(_ptr) \
  (_ptr)->AddRef()

http://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/xpcom/base/nsISupportsUtils.h#50
Basically it only AddRef's if the pointer isn't null, or else, we crash.

So you can safely replace all NS_ADDREF with NS_IF_ADDREF and won't notice the difference. OK so far?

Forget is here:
http://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/mfbt/RefPtr.h#269

  forget(I** aRhs)
  // Set the target of aRhs to the value of mRawPtr and null out mRawPtr.
  // Useful to avoid unnecessary AddRef/Release pairs with "out"
  // parameters where aRhs bay be a T** or an I** where I is a base class
  // of T.
  {
    MOZ_ASSERT(aRhs, "Null pointer passed to forget!");
    *aRhs = mRawPtr;
    mRawPtr = nullptr;
  }

So where we had
nsCOMPtr<nsIXYZ> local;
NS_IF_ADDREF(*aResult = local);
what happened was that *aResult got assigned the value of 'local', and then the refcount was increased. 'local' went out of scope and it's refcount was decreased by the destructor, but the object it pointed to lived on since *aResult held a reference.

If you ware sure that 'local' wasn't null, you could use NS_ADDREF() alone.

Now with forget() is doesn't matter whether the object being transferred was null or not since it's raw pointer was moved into *aRhs. No Addref() happens.

So we can just mechanically replace all NS_ADDREF(*aResult = local); with local.forget(aResult); Same goes for NS_IF_ADDREF().
I know you love Addref()/Release() reviews, so here's another one ;-)
Attachment #8906857 - Flags: review?(acelists)
Comment on attachment 8906824 [details] [diff] [review]
Part 1: 1340972-addref-compose.patch (v2) [Landed: comment #23]

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

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +1742,5 @@
>  }
>  
>  nsresult nsMsgCompose::GetCompFields(nsIMsgCompFields * *aCompFields)
>  {
> +  NS_IF_ADDREF(*aCompFields = (nsIMsgCompFields*)m_compFields);

I wonder why this needs a cast.
Yeah, because m_compFields is a nsMsgComFields, not nsCOMPtr<nsIMsgCompFields> . Maybe we need to access some internal members not visible via nsIMsgCompFields interface.
Meat for another patch/bug :)

::: mailnews/compose/src/nsMsgQuote.cpp
@@ +48,5 @@
>    nsresult rv = NS_OK;
>    if (aMsgQuote)
>    {
>      nsCOMPtr<nsIMsgQuote> msgQuote = do_QueryReferent(mMsgQuote);
> +    NS_IF_ADDREF(*aMsgQuote = msgQuote);

This one is tricky. This isn't a .forget() because msgQuote is some link to mMsgQuote ? Is it a link/pointer or the object itself or what? But msgQuote variable can be destroyed here.
Attachment #8906824 - Flags: review?(acelists) → review+
Attachment #8906825 - Flags: review?(acelists) → review+
Comment on attachment 8906857 [details] [diff] [review]
1340972-addref-refptr.patch (v1).

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

::: mailnews/compose/src/nsMsgCompose.cpp
@@ -1803,2 @@
>      m_compFields = reinterpret_cast<nsMsgCompFields*>(compFields);
> -    NS_ADDREF(m_compFields);

Does RefPtr now handle this automatically? This is Release/Addref backwards. And it does not assign a new object.

::: mailnews/compose/src/nsMsgSendLater.cpp
@@ +456,5 @@
>  SendOperationListener::OnStopSending(const char *aMsgID, nsresult aStatus, const char16_t *aMsg, 
>                                       nsIFile *returnFile)
>  {
> +  if (mSendLater)
> +    mSendLater->OnSendStepFinished(aStatus);

Why is this change safe? We released the mSendLater when OnSendStepFinished was false. Now it get's released automatically somewhere else (at destruction), later than here.

@@ -652,5 @@
>    PR_FREEIF(mLeftoverBuffer);
>  
>    // Now, get our stream listener interface and plug it into the DisplayMessage
>    // operation
> -  AddRef();

So why can these be removed? What is the replacement?

::: mailnews/local/src/nsMailboxService.cpp
@@ +432,5 @@
>    {
>      rv = protocol->Initialize(aMailboxUrl);
>      if (NS_FAILED(rv))
>      {
>        delete protocol;

This failed to compile now. Is the line needed?
(In reply to :aceman from comment #12)
> > +  NS_IF_ADDREF(*aCompFields = (nsIMsgCompFields*)m_compFields);
Let me look at this further in the next patch.

> >      nsCOMPtr<nsIMsgQuote> msgQuote = do_QueryReferent(mMsgQuote);
> > +    NS_IF_ADDREF(*aMsgQuote = msgQuote);
> This one is tricky. This isn't a .forget() because msgQuote is some link to
> mMsgQuote ? Is it a link/pointer or the object itself or what? But msgQuote
> variable can be destroyed here.
msgQuote is a local variable going out of scope, so I can transfer it's pointer instead of doing extra Addref()'ing.
Comment on attachment 8906857 [details] [diff] [review]
1340972-addref-refptr.patch (v1).

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

::: mailnews/compose/src/nsMsgCompose.cpp
@@ -1803,2 @@
>      m_compFields = reinterpret_cast<nsMsgCompFields*>(compFields);
> -    NS_ADDREF(m_compFields);

You first need to count down the ref on the old than add a ref to the new. Terrible stuff, using RefPtr will do it for you.

::: mailnews/compose/src/nsMsgSendLater.cpp
@@ +456,5 @@
>  SendOperationListener::OnStopSending(const char *aMsgID, nsresult aStatus, const char16_t *aMsg, 
>                                       nsIFile *returnFile)
>  {
> +  if (mSendLater)
> +    mSendLater->OnSendStepFinished(aStatus);

Right. RefPtr takes care of the deletion.

@@ -652,5 @@
>    PR_FREEIF(mLeftoverBuffer);
>  
>    // Now, get our stream listener interface and plug it into the DisplayMessage
>    // operation
> -  AddRef();

This was there because the object was pointed to by a raw pointer. Imagine passing that to the rest of the code which does refcounting. If the last reference in that code goes, the object is automatically destroyed. Oops, we lost our object. So to grab it, a ref was added and later removed. Terrible stuff now replaced by RefPtr.

::: mailnews/local/src/nsMailboxService.cpp
@@ +432,5 @@
>    {
>      rv = protocol->Initialize(aMailboxUrl);
>      if (NS_FAILED(rv))
>      {
>        delete protocol;

Yes, my mistake, should be protocol = nullptr. We remove the (last) ref and it the object gets destroyed for us.
Attachment #8906857 - Flags: review?(acelists)
All I've done here was to fix the "delete protocol".

I tried
-  RefPtr<nsMsgCompFields>                   m_compFields;
+  nsCOPPtr<nsIMsgCompFields>                m_compFields;
but this gives 100 compile errors since too many things are hacked in nsMsgCompFields.cpp, like:
'SplitRecipientsEx': is not a member of 'nsIMsgCompFields'
So the interface nsIMsgCompFields.idl defines some methods, and the class implementation defines some more not in the interface.

So I left it at introducing RefPtr, and we need the cast we talked about.
Try here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fe944a1daa49b88164b8b91c18a1e0dfaabf7490
Attachment #8906857 - Attachment is obsolete: true
Attachment #8906940 - Flags: review?(acelists)
Sorry, I put this into the wrong patch.
Attachment #8906940 - Attachment is obsolete: true
Attachment #8906940 - Flags: review?(acelists)
Attachment #8906941 - Flags: review?(acelists)
Posted patch 1340972-addref-db.patch (v1) (obsolete) — Splinter Review
Here's the clean-up in mailnews/db. Another patch to follow for the ugly Addref(0 business.
Attachment #8906994 - Flags: review?(acelists)
Attachment #8906941 - Attachment description: 1340972-addref-refptr.patch (v2) → 1340972-addref-refptr-compose-local.patch (v2)
Attachment #8906941 - Attachment filename: 1340972-addref-refptr.patch → 1340972-addref-refptr-compose-local.patch
Sorry, previous patch wasn't quite right. Release also nulls, so we need to do this here.
Attachment #8906941 - Attachment is obsolete: true
Attachment #8906941 - Flags: review?(acelists)
Attachment #8907008 - Flags: review?(acelists)
Sorry, typo.
Attachment #8907008 - Attachment is obsolete: true
Attachment #8907008 - Flags: review?(acelists)
Attachment #8907010 - Flags: review?(acelists)
One more ugly thing to remove.
Attachment #8906994 - Attachment is obsolete: true
Attachment #8906994 - Flags: review?(acelists)
Attachment #8907011 - Flags: review?(acelists)
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c58474b55551
Part 1: Fix use of NS_ADDREF() and NS_IF_ADDREF() in mailnews/compose. r=aceman
https://hg.mozilla.org/comm-central/rev/7ea1a92f3070
Part 2: Fix use of NS_ADDREF() and NS_IF_ADDREF() in mailnews/local. r=aceman
Comment on attachment 8906824 [details] [diff] [review]
Part 1: 1340972-addref-compose.patch (v2) [Landed: comment #23]

Sorry, I misunderstood, this is possible as well:
https://hg.mozilla.org/comm-central/rev/c58474b55551#l5.14
-    *aMsgQuote = msgQuote;
-    NS_IF_ADDREF(*aMsgQuote);
+    msgQuote.forget(aMsgQuote);
Attachment #8906824 - Attachment description: 1340972-addref-compose.patch (v2) → 1340972-addref-compose.patch (v2) [Landed: comment #23]
Attachment #8906825 - Attachment description: 1340972-addref-local.patch (v2b). → 1340972-addref-local.patch (v2b) [Landed: comment #23]
Attachment #8906824 - Attachment description: 1340972-addref-compose.patch (v2) [Landed: comment #23] → Part 1: 1340972-addref-compose.patch (v2) [Landed: comment #23]
Attachment #8906825 - Attachment description: 1340972-addref-local.patch (v2b) [Landed: comment #23] → Part 2: 1340972-addref-local.patch (v2b) [Landed: comment #23]
Attachment #8907010 - Attachment description: 1340972-addref-refptr-compose-local.patch (v2c) → Part 3: 1340972-addref-refptr-compose-local.patch (v2c)
Attachment #8907011 - Attachment description: 1340972-addref-db.patch (v2) → Part 4: 1340972-addref-db.patch (v2)
Sorry about the noise, found some more ugliness.
Attachment #8907011 - Attachment is obsolete: true
Attachment #8907011 - Flags: review?(acelists)
Attachment #8907074 - Flags: review?(acelists)
Attachment #8907074 - Attachment description: 1340972-addref-db.patch (v3) → Part 4: 1340972-addref-db.patch (v3)
This concludes 1) compose, 2) local and 3) db.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=82f75d9dc2293f3b5cad337187f985f50792f18f

Next victims: 4) import, 5) extensions, 6) build, 7) mime, 8) imap, 9) base, 10) addrbook, 11) mapi, 12) news.
There is nothing in test, intl and jsaccount is already good. So six done, nine to go, import, extensions and build being trivial.
Attachment #8907092 - Flags: review?(acelists)
Four easy directories in this patch: extensions, build, mapi, mime. Only clean-up.

So now we're left with five: import, imap, base, addrbook, news.
Attachment #8907118 - Flags: review?(acelists)
Oops, that didn't compile. Here a new one with all parts so far:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c59f926d81ac35f51307c0acdc3e1cbe379c9d36
Attachment #8907010 - Attachment is obsolete: true
Attachment #8907010 - Flags: review?(acelists)
Attachment #8907121 - Flags: review?(acelists)
Summary: Replace NS_IF_ADDREF(*result = x) with x.forget(result) where possible in mailnews → Replace NS_IF_ADDREF(*result = x) with x.forget(result) where possible in mailnews and remove non-standard use of Addref()/Release()
Attachment #8907074 - Flags: review?(acelists) → review+
Comment on attachment 8907092 [details] [diff] [review]
Part 5: 1340972-addref-refptr-db.patch (v1)

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

::: mailnews/db/msgdb/src/nsMailDatabase.cpp
@@ +285,1 @@
>          if (offlineOp)

Can offlineOp be null here ?

::: mailnews/db/msgdb/src/nsMsgThread.cpp
@@ -689,5 @@
>  }
>  
>  nsMsgThreadEnumerator::~nsMsgThreadEnumerator()
>  {
> -    NS_RELEASE(mThread);

You convert some of the other NS_IF_RELEASE(ptr) into ptr=nullptr. Is that not needed here, because the containing object is going away anyway?
Attachment #8907092 - Flags: review?(acelists) → review+
Attachment #8907156 - Flags: review?(acelists)
Attachment #8907156 - Attachment is obsolete: true
Attachment #8907156 - Flags: review?(acelists)
Attachment #8907160 - Flags: review?(acelists)
(In reply to :aceman from comment #29)
> Can offlineOp be null here ?
No, it can't, fixed.

> You convert some of the other NS_IF_RELEASE(ptr) into ptr=nullptr. Is that
> not needed here, because the containing object is going away anyway?
Yes.
Attachment #8907092 - Attachment is obsolete: true
Attachment #8907168 - Flags: review+
That fixes news. Imap, base, addrbook and import left, and import has some realy beauties, I've already looked.

Kindly do part 3 so I can check them in in order.
Attachment #8907170 - Flags: review?(acelists)
Comment on attachment 8907118 [details] [diff] [review]
Part 6: 1340972-addref-build-extensions-mapi-mime.patch (v1) [Landed: comment #49]

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

::: mailnews/mime/src/nsStreamConverter.cpp
@@ +801,5 @@
>  
>  NS_IMETHODIMP
>  nsStreamConverter::GetIdentity(nsIMsgIdentity * *aIdentity)
>  {
>    if (!aIdentity) return NS_ERROR_NULL_POINTER;

If you need a stream of patches next time, you could start changing these to NS_ENSURE_ARG_POINTER(aIdentity);. But some people may not like that much so it must first be discussed in the bug.
Attachment #8907118 - Flags: review?(acelists) → review+
Comment on attachment 8907121 [details] [diff] [review]
Part 3: 1340972-addref-refptr-compose-local.patch (v2d) [Landed: comment #49]

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

::: mailnews/local/src/nsMailboxService.cpp
@@ +431,5 @@
>    {
>      rv = protocol->Initialize(aMailboxUrl);
>      if (NS_FAILED(rv))
>      {
> +      protocol = nullptr;

Will this deallocate the memory taken by the 'new' ?
Attachment #8907121 - Flags: review?(acelists) → review+
Yes. The last reference dies, the object is destroyed.
Comment on attachment 8907160 [details] [diff] [review]
Part 7: 1340972-addref-news.patch (v1b) [Landed: comment #56]

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

::: mailnews/news/src/nsNewsFolder.cpp
@@ +667,5 @@
>    }
> +
> +  NS_ADDREF(*db = mDatabase);
> +
> +  if (NS_SUCCEEDED(openErr))

It is a strange case if there would be a non-null mDatabase, but openErr would not be a success. Can that happen?
If not, this could maybe be simplified.

::: mailnews/news/src/nsNntpService.cpp
@@ +299,3 @@
>    if (aURL)
> +    url.forget(aURL);
> +  return rv;

Why is this better?

@@ +884,5 @@
>  nsNntpService::ConstructNntpUrl(const char *urlString, nsIUrlListener *aUrlListener, nsIMsgWindow *aMsgWindow, const char *originalMessageUri, int32_t action, nsIURI ** aUrl)
>  {
>    nsresult rv = NS_OK;
>  
> +  nsCOMPtr<nsINntpUrl> nntpUrl = do_CreateInstance(NS_NNTPURL_CONTRACTID,&rv);

I think there uses to be a space after comma.
(In reply to Jorg K (GMT+2) from comment #37)
> Yes. The last reference dies, the object is destroyed.

I mean, does the RefPtr somehow call delete internally?
Yes. That's the idea of refcounting. The last one closes the door.
(In reply to :aceman from comment #38)
> It is a strange case if there would be a non-null mDatabase, but openErr
> would not be a success. Can that happen?
> If not, this could maybe be simplified.
I don't know, I'm not here to change behaviour.

> > +    url.forget(aURL);
> > +  return rv;
> Why is this better?
You mean this:
   if (aURL)
-    NS_IF_ADDREF(*aURL = url);
-
-  return GetMessageFromUrl(url, aMsgWindow, aDisplayConsumer);
to
+  rv = GetMessageFromUrl(url, aMsgWindow, aDisplayConsumer);
   if (aURL)
+    url.forget(aURL);
+  return rv;

Well, in the code above, if I do
if (aURL)
  url.forget(aURL)
I don't know now whether my pointer is still in url or in aURL. Hence I cannot call GetMessageFromUrl().
So I need to call that first.

> > +  nsCOMPtr<nsINntpUrl> nntpUrl = do_CreateInstance(NS_NNTPURL_CONTRACTID,&rv);
> I think there uses to be a space after comma.
I didn't remove it, but I can add one:
-  nsCOMPtr <nsINntpUrl> nntpUrl = do_CreateInstance(NS_NNTPURL_CONTRACTID,&rv);
+  nsCOMPtr<nsINntpUrl> nntpUrl = do_CreateInstance(NS_NNTPURL_CONTRACTID,&rv);
That shouldn't stop the review.
(In reply to Jorg K (GMT+2) from comment #41)
> You mean this:
>    if (aURL)
> -    NS_IF_ADDREF(*aURL = url);
> -
> -  return GetMessageFromUrl(url, aMsgWindow, aDisplayConsumer);
> to
> +  rv = GetMessageFromUrl(url, aMsgWindow, aDisplayConsumer);
>    if (aURL)
> +    url.forget(aURL);
> +  return rv;
> 
> Well, in the code above, if I do
> if (aURL)
>   url.forget(aURL)
> I don't know now whether my pointer is still in url or in aURL. Hence I
> cannot call GetMessageFromUrl().

I thought about 'return GetMessageFromUrl(aUrl, aMsgWindow, aDisplayConsumer);' but if aUrl can be null, it may not work.
So yes, running it on url is safer.

> do_CreateInstance(NS_NNTPURL_CONTRACTID, &rv);
> That shouldn't stop the review.

Sure.
Attachment #8907160 - Flags: review?(acelists) → review+
(In reply to :aceman from comment #42)
> I thought about 'return GetMessageFromUrl(aUrl, aMsgWindow,
> aDisplayConsumer);' but if aUrl can be null, it may not work.
> So yes, running it on url is safer.
The out parameter being null is a real pain which Kent fixed in bug 1229649 but sadly he didn't cover everything, like news or calls to FetchMessage().

So I can't do my elimination work in IMAP. I'll have to see.
This is IMAP. Base, addressbook and import left.
Attachment #8907211 - Flags: review?(acelists)
Comment on attachment 8907211 [details] [diff] [review]
Part 10: 1340972-addref-refptr-imap.patch (v1).

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

I see a mistake.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ -4307,5 @@
>    }
>    if (!GetServerStateParser().LastCommandSuccessful())
>      GetServerStateParser().ResetFlagInfo();
> -
> -  NS_IF_RELEASE(new_spec);

Here we release it.

@@ -6755,5 @@
>    {
>      nsImapMailboxSpec *new_spec = GetServerStateParser().CreateCurrentMailboxSpec(mailboxName);
>      if (new_spec && m_imapMailFolderSink)
>        m_imapMailFolderSink->UpdateImapMailboxStatus(this, new_spec);
> -    NS_IF_RELEASE(new_spec);

And here.

::: mailnews/imap/src/nsImapServerResponseParser.cpp
@@ -3245,5 @@
> -  {
> -    HandleMemoryFailure();
> -    return  nullptr;
> -  }
> -  NS_ADDREF(returnSpec);

This was particularly ugly since the caller need to release this, see above.
Attachment #8907211 - Flags: review?(acelists)
Attachment #8907211 - Attachment is obsolete: true
Attachment #8907221 - Flags: review?(acelists)
Attachment #8907221 - Attachment description: 1340972-addref-refptr-imap.patch (v2). → Part 10: 1340972-addref-refptr-imap.patch (v2).
Here's a try run up to part 10:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ea86e191aad879de591077027bd51a945c9ee487

Previous try to part 6 was OK, so new here are news and imap.
Attachment #8907170 - Flags: review?(acelists) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/040f15fdc501
Part 3: Replace error-prone Addref()/Release() with using RefPtr in mailnews/compose+local. r=aceman
https://hg.mozilla.org/comm-central/rev/91ed5528ef11
Part 4: Fix use of NS_ADDREF() and NS_IF_ADDREF() in mailnews/db. r=aceman
https://hg.mozilla.org/comm-central/rev/3365d035c78d
Part 5: Replace error-prone Addref()/Release() with using RefPtr in mailnews/db. r=aceman
https://hg.mozilla.org/comm-central/rev/827578490366
Part 6: Fix use of NS_ADDREF() and NS_IF_ADDREF() in mailnews/build+extensions+mapi+mime. r=aceman
Attachment #8907074 - Attachment description: Part 4: 1340972-addref-db.patch (v3) → Part 4: 1340972-addref-db.patch (v3) [Landed: comment #49]
Attachment #8907118 - Attachment description: Part 6: 1340972-addref-build-extensions-mapi-mime.patch (v1) → Part 6: 1340972-addref-build-extensions-mapi-mime.patch (v1) [Landed: comment #49]
Attachment #8907121 - Attachment description: Part 3: 1340972-addref-refptr-compose-local.patch (v2d) → Part 3: 1340972-addref-refptr-compose-local.patch (v2d) [Landed: comment #49]
Attachment #8907168 - Attachment description: Part 5: 1340972-addref-refptr-db.patch (v1b) → Part 5: 1340972-addref-refptr-db.patch (v1b) [Landed: comment #49]
The crash was a stunning example of how dangerous and unmaintainable that stuff is. Allocate/addref here, release six times |NS_IF_RELEASE(adoptedBoxSpec);| somewhere else. Just deadly :-(

Leaving it to the RefPtr's and removing the releases fixes the problem.
Try up to part 10:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=391a763068509ae7a09efb754e78d7368d25d5e8
Attachment #8907221 - Attachment is obsolete: true
Attachment #8907221 - Flags: review?(acelists)
Attachment #8907290 - Flags: review?(acelists)
Looking at this again, kDiscoverBaseFolderInProgress didn't release the adoptedBoxSpec. Turns out that wasn't used, so it's gone now. Technical debt.
Attachment #8907290 - Attachment is obsolete: true
Attachment #8907290 - Flags: review?(acelists)
Attachment #8907301 - Flags: review?(acelists)
Comment on attachment 8907210 [details] [diff] [review]
Part 9: 1340972-addref-imap.patch (v1) [Landed: comment #58]

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

::: mailnews/imap/src/nsAutoSyncState.cpp
@@ +560,5 @@
>    
>    nsresult rv;
>    nsCOMPtr <nsIMsgFolder> ownerFolder = do_QueryReferent(mOwnerFolder, &rv);
>    NS_ENSURE_SUCCESS(rv, rv);
>    

You could kill the spaces :)

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +147,5 @@
>    // its a header we haven't really got and the caller has done something
>    // wrong.
>    NS_ENSURE_TRUE(hdrIndex < m_nextFreeHdrInfo, NS_ERROR_NULL_POINTER);
>  
> +  NS_IF_ADDREF(*aResult = m_hdrInfos.SafeObjectAt(hdrIndex));

'Nice', assigning to *aResult without NS_ENSURE_ARG_POINTER(aResult). Yeah, I see the other methods have the same problem.
Attachment #8907210 - Flags: review?(acelists) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d6ffd03d673c
Part 7: Fix use of NS_ADDREF() and NS_IF_ADDREF() in mailnews/news. r=aceman
https://hg.mozilla.org/comm-central/rev/97635cae0d2f
Part 8: Replace error-prone Addref()/Release() with using RefPtr in mailnews/news. r=aceman
Comment on attachment 8907160 [details] [diff] [review]
Part 7: 1340972-addref-news.patch (v1b) [Landed: comment #56]

I've added the space as requested:
https://hg.mozilla.org/comm-central/rev/d6ffd03d673c#l4.56
Attachment #8907160 - Attachment description: Part 7: 1340972-addref-news.patch (v1b) → Part 7: 1340972-addref-news.patch (v1b) [Landed: comment #56]
Attachment #8907170 - Attachment description: Part 8: 1340972-addref-refptr-news.patch (v1) → Part 8: 1340972-addref-refptr-news.patch (v1) [Landed: comment #56]
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b41092b15ade
Part 9: Fix use of NS_ADDREF() and NS_IF_ADDREF() in mailnews/imap. r=aceman
Attachment #8907210 - Attachment description: Part 9: 1340972-addref-imap.patch (v1) → Part 9: 1340972-addref-imap.patch (v1) [Landed: comment #58]
Comment on attachment 8907301 [details] [diff] [review]
Part 10: 1340972-addref-refptr-imap.patch (v3b) [Landed: comment #66]

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

::: mailnews/imap/src/nsImapProtocol.h
@@ +670,5 @@
>    int32_t m_uidValidity; // stored uid validity for the selected folder.
>  
>    enum EMailboxHierarchyNameState {
> +      kNoOperationInProgress,
> +      // kDiscoverBaseFolderInProgress, - Unused. Keeping for historical reasons.

Won't this change the internal values of all the following enum constants? Is that a problem or not? Is the EMailboxHierarchyNameState value stored in some external files?

::: mailnews/imap/src/nsImapServerResponseParser.cpp
@@ -795,5 @@
> -  NS_ADDREF(boxSpec);
> -  bool needsToFreeBoxSpec = true;
> -  if (!boxSpec)
> -    HandleMemoryFailure();
> -  else

Nice that all this can go.

@@ +3269,5 @@
>      returnSpec->mFlagState = fFlagState; //copies flag state
>    else
>      returnSpec->mFlagState = nullptr;
>  
> +  return returnSpec.forget();

Can't this function be converted to having a normal out-argument? There are about 2 users. Does it have to return this special already_AddRefed<nsImapMailboxSpec> type?

::: mailnews/imap/src/nsImapServerResponseParser.h
@@ +76,5 @@
>    void ResetSearchResultSequence() {fSearchResults->ResetSequence();}
>  
>    // create a struct mailbox_spec from our info, used in
>    // libmsg c interface
> +  already_AddRefed<nsImapMailboxSpec> CreateCurrentMailboxSpec(const char *mailboxName = nullptr);

This is a new one:) What does it do?
Attachment #8907301 - Flags: feedback+
Comment on attachment 8907335 [details] [diff] [review]
Part 11: 1340972-addref-addrbook.patch (v1) [Landed: comment #66]

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

::: mailnews/addrbook/src/nsAbQueryStringToExpression.cpp
@@ +326,5 @@
>          NS_ENSURE_SUCCESS(rv, rv);
>          rv = cs->SetValue (valueUCS2.get ());
>          NS_ENSURE_SUCCESS(rv, rv);
>      }
>              

Whole line to be killed :)

::: mailnews/addrbook/src/nsAddbookProtocolHandler.cpp
@@ +61,5 @@
>                                                 nsIURI *aBaseURI,
>                                                 nsIURI **_retval)
>  {
>    nsresult rv;
> +	nsCOMPtr<nsIAddbookUrl> addbookUrl = do_CreateInstance(NS_ADDBOOKURL_CONTRACTID, &rv);

Fix the indent?

::: mailnews/addrbook/src/nsAddrDatabase.cpp
@@ -1284,5 @@
>      }
>  
> -    nsCOMPtr<nsIAbCard> newCard;
> -    CreateABCard(pCardRow, 0, getter_AddRefs(newCard));
> -    NS_IF_ADDREF(*aPNewCard = newCard);

So where is the addref in the new code?

@@ +2580,1 @@
>      m_dbDirectory = do_GetWeakReference(directory);

Interesting code that there is no direct relation inside this function between the 'directory' and the new enumerator.

::: mailnews/addrbook/src/nsDirPrefs.cpp
@@ +235,1 @@
>    

Spaces.
Attachment #8907335 - Flags: feedback+
(In reply to :aceman from comment #59)
> > +      // kDiscoverBaseFolderInProgress, - Unused. Keeping for historical reasons.
> Won't this change the internal values of all the following enum constants?
> Is that a problem or not? Is the EMailboxHierarchyNameState value stored in
> some external files?
Yes, but they are just some enum driving the program logic.
https://dxr.mozilla.org/comm-central/search?q=m_hierarchyNameState
If you prefer, I can add a dummy line there son the numbers don't change.

> @@ +3269,5 @@
> >      returnSpec->mFlagState = fFlagState; //copies flag state
> >    else
> >      returnSpec->mFlagState = nullptr;
> >  
> > +  return returnSpec.forget();
> 
> Can't this function be converted to having a normal out-argument? There are
> about 2 users. Does it have to return this special
> already_AddRefed<nsImapMailboxSpec> type?
It could, but what's wrong with this approach?

> > +  already_AddRefed<nsImapMailboxSpec> CreateCurrentMailboxSpec(const char *mailboxName = nullptr);
> This is a new one:) What does it do?
Not new at all. Here are some simple examples of how it's done:
https://dxr.mozilla.org/comm-central/search?q=GetMsgComposeForContext&redirect=false
https://dxr.mozilla.org/comm-central/search?q=GetAddressBookFromUri&redirect=false
And here are plenty more:
https://dxr.mozilla.org/comm-central/search?q=already_AddRefed%3C&redirect=false

The simple usage is: Function returns some RefPtr/nsCOMPtr.forget(), the receiver assigns is again to the same pointer type.
The forget() tells the pointer to forget about the reference, so it doesn't get counted down (and the object destroyed) when the function returns. The receiver simple picks it up.

If you have no other issues, this should be an r+ ;-)
(In reply to :aceman from comment #60)
> > -    nsCOMPtr<nsIAbCard> newCard;
> > -    CreateABCard(pCardRow, 0, getter_AddRefs(newCard));
> > -    NS_IF_ADDREF(*aPNewCard = newCard);
> 
> So where is the addref in the new code?
Right here: CreateABCard(pCardRow, 0, aPNewCard);
It happens in the caller, if I'm not mistaken, that's here:
https://dxr.mozilla.org/comm-central/rev/aa24e45307629490b15ad5165aa8b584cdb758c0/mailnews/addrbook/src/nsAddrDatabase.cpp#2721

> >      m_dbDirectory = do_GetWeakReference(directory);
> Interesting code that there is no direct relation inside this function
> between the 'directory' and the new enumerator.
Yes, some ugly side-effect of the function. It was there.

> Spaces.
I can fix all the spaces and indentation issues you detected, but that shouldn't stop the review.

You didn't complain about the
already_AddRefed<nsAddrDatabase> nsAddrDatabase::FindInCache(nsIFile *dbName) ;-)
Just the same as above.

This equally should be an r+.
(In reply to Jorg K (GMT+2) from comment #62)
> (In reply to :aceman from comment #60)
> > > -    nsCOMPtr<nsIAbCard> newCard;
> > > -    CreateABCard(pCardRow, 0, getter_AddRefs(newCard));
> > > -    NS_IF_ADDREF(*aPNewCard = newCard);
> > 
> > So where is the addref in the new code?
> Right here: CreateABCard(pCardRow, 0, aPNewCard);
> It happens in the caller, if I'm not mistaken, that's here:
> https://dxr.mozilla.org/comm-central/rev/
> aa24e45307629490b15ad5165aa8b584cdb758c0/mailnews/addrbook/src/
> nsAddrDatabase.cpp#2721

So was there double addrefing that you now remove?
Attachment #8907335 - Flags: review?(acelists) → review+
Comment on attachment 8907301 [details] [diff] [review]
Part 10: 1340972-addref-refptr-imap.patch (v3b) [Landed: comment #66]

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

OK.
Attachment #8907301 - Flags: review?(acelists) → review+
(In reply to :aceman from comment #63)
> So was there double addrefing that you now remove?

The original code ...
> -    nsCOMPtr<nsIAbCard> newCard;
> -    CreateABCard(pCardRow, 0, getter_AddRefs(newCard));
> -    NS_IF_ADDREF(*aPNewCard = newCard);
... created a new card with pointer 'newCard'. The refcount was increased since *aPNewCard also held a reference, and when newCard went out of scope, it was decreased. So the object went from 1 to 2 to 1. Now the object is created and the pointer to it is held were we want it straight away. So we save two operations on the refcount.

Every unnecessary NS_IF_ADDREF() removes saves two calls to Addref() and Release(). If you replace with .forget(), only the pointer gets transferred and no recount increase/decrease takes place.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fc81f4f432e3
Part 10: Replace error-prone Addref()/Release() with using RefPtr in mailnews/imap. r=aceman
https://hg.mozilla.org/comm-central/rev/16285e6b6a08
Part 11: Fix use of NS_ADDREF() and NS_IF_ADDREF() in mailnews/addrbook. r=aceman
Attachment #8907301 - Attachment description: Part 10: 1340972-addref-refptr-imap.patch (v3b) → Part 10: 1340972-addref-refptr-imap.patch (v3b) [Landed: comment #66]
Comment on attachment 8907335 [details] [diff] [review]
Part 11: 1340972-addref-addrbook.patch (v1) [Landed: comment #66]

I removed the spaces as requested and fixed indentation which was in fact a tab.
Attachment #8907335 - Attachment description: Part 11: 1340972-addref-addrbook.patch (v1) → Part 11: 1340972-addref-addrbook.patch (v1) [Landed: comment #66]
Nothing interesting here ;-)
Attachment #8907776 - Flags: review?(acelists)
Nothing interesting here.
Attachment #8907791 - Flags: review?(acelists)
Comment on attachment 8907776 [details] [diff] [review]
Part 12: 1340972-addref-base-search.patch (v1)

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

::: mailnews/base/search/src/nsMsgFilterService.cpp
@@ +895,3 @@
>    NS_ADDREF(*aFilterList = filterList);
>    (*aFilterList)->SetFolder(aFolder);
>    filterList->m_temporaryList = true;

This one is interesting:) Are both objects the same now but we use one in one call and another in the second call? Can this be merged?
Also what about .forget() here?
Attachment #8907776 - Flags: review?(acelists) → review+
Comment on attachment 8907791 [details] [diff] [review]
Part 13: 1340972-addref-base-util.patch (v1) [Landed: comment #80]

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

::: mailnews/base/util/nsMsgTxn.cpp
@@ +248,5 @@
>  nsresult nsMsgTxn::GetMsgWindow(nsIMsgWindow **msgWindow)
>  {
>      if (!msgWindow || !m_msgWindow)
>          return NS_ERROR_NULL_POINTER;
> +    NS_ADDREF (*msgWindow = m_msgWindow);

The other occurrences do not have a space.
Attachment #8907791 - Flags: review?(acelists) → review+
Can't be forget() since it comes from a new. Can't be merged since they are two different classes and (*aFilterList)->m_temporaryList gives a compile error. I tried :-(
Yeah, because nsIMsgFilterList does not expose the internal member. I then wonder why you can do *aFilterList = filterList without a cast here (they are different types). At some other places the cast was needed.
Like
nsIMsgSearchTerm **aResult;
nsMsgSearchTerm *term = new nsMsgSearchTerm;	
NS_ADDREF(*aResult = static_cast<nsIMsgSearchTerm*>(new nsMsgSearchTerm));
Removed unneeded static cast. Happy?
Attachment #8907776 - Attachment is obsolete: true
Attachment #8907817 - Flags: review+
(In reply to Jorg K (GMT+2) from comment #72)
> Can't be forget() since it comes from a new. Can't be merged since they are
> two different classes and (*aFilterList)->m_temporaryList gives a compile
> error. I tried :-(

What about this:
>    filterList->SetFolder(aFolder);
>    filterList->m_temporaryList = true;
>    NS_ADDREF(*aFilterList = filterList);
The mailnews/base/src patch is coming. I saw some really ugly stuff which will be addressed in another patch:
.\nsMessenger.cpp:    NS_ADDREF(saveListener);
.\nsMsgGroupThread.cpp:  NS_ADDREF(thread);
.\nsMsgPrintEngine.cpp:          NS_ADDREF(wpl);
.\nsMsgTagService.cpp:            NS_ADDREF(tagArray[currentTagIndex++] = newMsgTag);

.\nsMsgFolderCacheElement.cpp:  NS_IF_ADDREF(m_mdbRow = row);
That's the straight forward stuff, the hard stuff is coming.
Attachment #8907830 - Flags: review?(acelists)
(In reply to :aceman from comment #75)
> What about this:
> >    filterList->SetFolder(aFolder);
> >    filterList->m_temporaryList = true;
> >    NS_ADDREF(*aFilterList = filterList);
Yes.
Attachment #8907817 - Attachment is obsolete: true
Attachment #8907831 - Flags: review+
That's getting rid of all the ugly stuff. No idea what happened in the print engine, looks like a left-over unused variable and a reference that leaks.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fe32bc2419671794c8c4831eac2a9dbe2b303b25
Attachment #8907843 - Flags: review?(acelists)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/95c5d61a2c34
Part 12: Fix use of NS_ADDREF() and NS_IF_ADDREF() in mailnews/base/search. r=aceman
https://hg.mozilla.org/comm-central/rev/87fec3bfd645
Part 13: Fix use of NS_ADDREF() and NS_IF_ADDREF() in mailnews/base/util. r=aceman
Comment on attachment 8907791 [details] [diff] [review]
Part 13: 1340972-addref-base-util.patch (v1) [Landed: comment #80]

I removed the unwanted space.
Attachment #8907791 - Attachment description: Part 13: 1340972-addref-base-util.patch (v1) → Part 13: 1340972-addref-base-util.patch (v1) [Landed: comment #80]
Attachment #8907831 - Attachment description: Part 12: 1340972-addref-base-search.patch (v1c) → Part 12: 1340972-addref-base-search.patch (v1c) [Landed: comment #80]
Try up to part 15 is "green":
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fe32bc2419671794c8c4831eac2a9dbe2b303b25

Sadly lots of crashes (asserts) in debug mode due to bug 1399746.
I noticed that the crashes from bug 1399746 are Mac only for some reason. So here's a try with the 14 and 15 on Windows together with some stuff from bug 1368786, I hope it doesn't interfere:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ecb54ea446274101ea438d0a2353ad82d2c14f51
This is green, apart from the usual failures. I re-ran opt X, with luck it will be green as debug X.
Comment on attachment 8907843 [details] [diff] [review]
Part 15: 1340972-addref-refptr-base.patch (v1) [Landed: comment #98]

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

::: mailnews/base/src/nsMsgPrintEngine.cpp
@@ -374,5 @@
>          if (showProgressDialog) 
>          {
> -          nsIWebProgressListener* wpl = static_cast<nsIWebProgressListener*>(mPrintProgressListener.get());
> -          NS_ASSERTION(wpl, "nsIWebProgressListener is NULL!");
> -          NS_ADDREF(wpl);

Unused? Where is the matching release? Is print progress still working after this?
Looks unused, no? I haven't had the time to test printing and saving where I also cleaned out with a steel brush (nsMessenger.cpp).
Comment on attachment 8907830 [details] [diff] [review]
Part 14: 1340972-addref-base-src.patch (v1) [Landed: comment #98]

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

::: mailnews/base/src/nsMessengerWinIntegration.cpp
@@ +389,5 @@
>    NS_ENSURE_TRUE(bundleService, NS_ERROR_UNEXPECTED);
>    nsCOMPtr<nsIStringBundle> bundle;
>    bundleService->CreateBundle("chrome://messenger/locale/messenger.properties",
>                                getter_AddRefs(bundle));
> +  bundle.forget(aBundle);

I checked why you do not fix the other nsMessenger*Integration.cpp versions of this GetStringBundle function.
They use bundle.swap(*aBundle) instead of NS_IF_ADDREF(*aBundle = bundle) so that is why you skipped those.

So if you'd like some meat for another cleanup patch, find out what is the difference between bundle.swap(*aBundle) and bundle.forget(aBundle) and where you can convert to the latter one. .forget may be better for 'out' arguments.

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +1982,5 @@
>        // stop on first find; cache for next time
>        if (!aRealFlag)
>          SetLastServerFound(server, hostname, username, port, type);
>  
> +      NS_ADDREF(*aResult = server);  // Was polulated from member variable.

populated

::: mailnews/base/src/nsMsgFolderDataSource.cpp
@@ +1233,5 @@
>  
> +  NS_IF_ADDREF(*target = (isServer || MsgLowerCaseEqualsLiteral(serverType, "none") ||
> +                                      MsgLowerCaseEqualsLiteral(serverType, "pop3")) ?
> +              kTrueLiteral : kFalseLiteral
> +  );

This is getting a bit too long ;)

::: mailnews/base/src/nsMsgMailSession.cpp
@@ +492,2 @@
>  
>    return rv;

I'm a bit torn whether the empty line before 'return' is needed. Many functions have it, many do not.

::: mailnews/base/src/nsSubscribableServer.cpp
@@ +330,5 @@
>  NS_IMETHODIMP
>  nsSubscribableServer::GetSubscribeListener(nsISubscribeListener **aListener)
>  {
>  	if (!aListener) return NS_ERROR_NULL_POINTER;
> +	NS_IF_ADDREF(*aListener = mSubscribeListener);

This seems to change the behaviour slightly, you null out aListener even if mSubscribeListener is null.
I don't know if that is a problem. It does not seem to be an error condition, we return NS_OK. So maybe in that case returning null (the real value of mSubscribeListener) may be the right thing to do. I understand most callers will pass a clean (null) aListener into the function, but some may not (reuse a variable) in which case it would preserve its value and the caller could think that preserved value is the current value of mSubscribeListener .

::: mailnews/base/src/nsSubscribeDataSource.cpp
@@ +167,5 @@
>      else if (property == kNC_Subscribable.get()) {
>          bool isSubscribable;
>          rv = server->IsSubscribable(relativePath, &isSubscribable);
>          NS_ENSURE_SUCCESS(rv,rv);
>          

Spaces :)
Attachment #8907830 - Flags: review?(acelists) → review+
(In reply to Jorg K (GMT+2) from comment #86)
> Looks unused, no? I haven't had the time to test printing and saving where I
> also cleaned out with a steel brush (nsMessenger.cpp).

Sure, but this change is suspect so we could test it. I did and the progress dialog comes up fine.
Can you just check if there are enough references to mPrintProgressListener in the file so that this removed code wasn't the single one addreffing it and keeping it alive?
(In reply to :aceman from comment #87)
> So if you'd like some meat for another cleanup patch, find out what is the
> difference between bundle.swap(*aBundle) and bundle.forget(aBundle) and
> where you can convert to the latter one. .forget may be better for 'out'
> arguments.
Swap is here:
http://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/mfbt/RefPtr.h#239
So the 'empty' pointer in *aBundle goes into bundle and aBundle takes over the pointer from bundle.
We might follow up be inspecting the swap() usage. But here is no gain to be had from changing swap() to forget() since both save addref'ing. "getter_AddRefs just nulls out the nsCOMPtr" (quote :bz), so the called function gets a fresh pointer and swapping the null into the local variable that goes out of scope is OK. forget() is still the cleaner solution.

> > +      NS_ADDREF(*aResult = server);  // Was polulated from member variable.
> populated
Will fix.

> >    return rv;
> I'm a bit torn whether the empty line before 'return' is needed. Many
> functions have it, many do not.
I didn't add it, it was there, also in many other functions.

> This seems to change the behaviour slightly, you null out aListener even if
> mSubscribeListener is null.
See comment above. If called via getter_AddRefs() there will always be a "clean" parameter. So there shouldn't be a change in behaviour. I have checked the callers and they are cool. Also, I have checked all call sites where a member variable is addref'ed and assigned with NS_ADDREF (without the IF_) and didn't see any similar case. So three good reasons to leave it this way.

> Spaces :)
Will fix, but I have a new bug going to remove them wholesale: Bug 1399756. I've already started since I desperately needed a patch this morning after two huge M-C merges in the morning.

So in summary, all I will change are some spaces somewhere and the typo. I can do that :-)
I've looked at nsMessenger.cpp again. Three function have 'saveListener':

nsMessenger::SaveAttachment(): Already RefPtr, no extra addref/release. Cool.

nsMessenger::SaveAs(): Has this variable twice, once at function level as raw pointer, once in an if-block as RefPtr. At the end of the block, that goes out of scope, so no problem. The other one at function level, used in the else-block, is released at the end of the function despite never having been addref'ed. I guess releasing something with zero references won't do anything. Or maybe it serves as delete. Hmm. Using a RefPtr here will also do the "right thing"(TM).

nsMessenger::SaveMessages(): Using a raw pointer, doing Addref, but releasing only in the error case. So unless I'm missing something, that object leaks. Using a RefPtr here won't do harm, since the thing will do the "right thing"(TM).

Do you know how to trigger those last two functions? Can you test that? I guess SaveAs() is just the thing on the menu, and the else-block is "save as template". But then SaveMessages() is in the IDL and called from mail/base/content/mailCommands.js.

I guess I have to add some debug to see what is run.
File > Save As > Template runs nsMessenger::SaveAs(), else block, template. Still working.
(single message) File > Save As > File runs nsMessenger::SaveAs(), if block, unchanged, still working.
(multi message)  File > Save As > File runs nsMessenger::SaveMessages(), still working.
I don't understand the architecture behind all this, it it all still works.

Turning to printing next.
Printing. Put a breakpoint where that code got removed. mPrintProgressListener has a refcount of 4, but starting printing a few times increases to 5, 6, 7, 8. So instead of addref'ing additionally, there seems to be some release missing somewhere.

So I guess taking the dead code out, that was imported in 2008 according to blame, won't to any harm. The harm done is being there, confusing people and costing extra time.

So r+, please ;-)
Comment on attachment 8907843 [details] [diff] [review]
Part 15: 1340972-addref-refptr-base.patch (v1) [Landed: comment #98]

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

Thanks :)
Attachment #8907843 - Flags: review?(acelists) → review+
(In reply to Jorg K (GMT+2) from comment #92)
>So instead of addref'ing additionally, there
> seems to be some release missing somewhere.
> So I guess taking the dead code out, that was imported in 2008 according to
> blame, won't to any harm. The harm done is being there, confusing people and
> costing extra time.

That's what I asked for. Only addreffing and never releasing seems buggy.

(In reply to Jorg K (GMT+2) from comment #89)
> We might follow up be inspecting the swap() usage. But here is no gain to be
> had from changing swap() to forget() since both save addref'ing.
> "getter_AddRefs just nulls out the nsCOMPtr" (quote :bz), so the called
> function gets a fresh pointer and swapping the null into the local variable
> that goes out of scope is OK. forget() is still the cleaner solution.

Yes, so when you'll need a no-harm/style-cleanup function, you can do this.
There are only 58 uses of swap() in mailnews/ most of the form xxx.swap(*aXyz) *result or *_retval or so. Those can be done with a simple sed. I looked at them, and *all* 55 of then which have xxx.swap(*xyz) can be replaced.

I'll do that as an interlude here tomorrow ;-)
find mailnews -name *.cpp -exec sed -i -e 's/\.swap(\*/\.forget(/' {} \;
Attachment #8908380 - Flags: review?(acelists)
Oops, forgot the Mac file.
Attachment #8908380 - Attachment is obsolete: true
Attachment #8908380 - Flags: review?(acelists)
Attachment #8908382 - Flags: review?(acelists)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/254747a8daa5
Part 14: Fix use of NS_ADDREF() and NS_IF_ADDREF() in mailnews/base/src. r=aceman
https://hg.mozilla.org/comm-central/rev/ebe355c1d925
Part 15: Replace error-prone Addref()/Release() with using RefPtr in mailnews/base. r=aceman
Attachment #8907830 - Attachment description: Part 14: 1340972-addref-base-src.patch (v1) → Part 14: 1340972-addref-base-src.patch (v1) [Landed: comment #98]
Attachment #8907843 - Attachment description: Part 15: 1340972-addref-refptr-base.patch (v1) → Part 15: 1340972-addref-refptr-base.patch (v1) [Landed: comment #98]
Comment on attachment 8908382 [details] [diff] [review]
Part 16: 1340972-swap-forget.patch (v1b)

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

Thanks.

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +571,3 @@
>    if (NS_SUCCEEDED(rv))
>    {
>      nsCOMPtr<nsIMsgFolder> child = *aNewFolder;

Here we dereference aNewFolder...

@@ +574,5 @@
>      //we need to notify explicitly the flag change because it failed when we did AddSubfolder
>      child->OnFlagChange(mFlags);
>      child->SetPrettyName(folderName);  //because empty trash will create a new trash folder
>      NotifyItemAdded(child);
>      if (aNewFolder)

Here we check if it is non-null? Can that happen?
Yes, not your fault, but if you could look at that. I don't know why we even need child, *aNewFolder should have the valid created folder already (and be non-null).
https://dxr.mozilla.org/comm-central/rev/aa24e45307629490b15ad5165aa8b584cdb758c0/mailnews/local/src/nsLocalMailFolder.cpp#550
Attachment #8908382 - Flags: review?(acelists) → review+
Goodness gracious, what was that ugly thing?
Attachment #8908382 - Attachment is obsolete: true
Attachment #8908622 - Flags: review?(acelists)
Comment on attachment 8908622 [details] [diff] [review]
Part 16: 1340972-swap-forget.patch (v2) [Landed: comment #111]

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

Yes, let's try it ;)
Attachment #8908622 - Flags: review?(acelists) → review+
OK, this is the easy import stuff.
Attachment #8908665 - Flags: review?(acelists)
Comment on attachment 8908665 [details] [diff] [review]
Part 17: 1340972-addref-import.patch (v1)

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

::: mailnews/import/winlivemail/nsWMSettings.cpp
@@ +77,5 @@
>  nsresult nsWMSettings::Create(nsIImportSettings** aImport)
>  {
> +  NS_PRECONDITION(aImport != nullptr, "null ptr");
> +  if (! aImport)
> +    return NS_ERROR_NULL_POINTER;

Merge both lines into NS_ENSURE_ARG_POINTER(aImport);
They check the same thing. It may be that NS_PRECONDITION (=NS_ASSERTION) crashes in debug build when the condition is not met, but why should this place be so special? We return a proper error via nsresult.
Attachment #8908665 - Flags: review?(acelists) → review+
OK, I fixed all the preconditions in the vicinity I touched, but there are many more.
Attachment #8908665 - Attachment is obsolete: true
Attachment #8908685 - Flags: review+
Found some more.
Attachment #8908685 - Attachment is obsolete: true
Attachment #8908706 - Flags: review+
While doing the "ugly" stuff for import, I saw some bits I had missed elsewhere. So here they are.
Attachment #8908715 - Flags: review?(acelists)
Attachment #8908715 - Attachment description: Part 19: 1340972-addref-refptr-more.patch (v1) → Part 18: 1340972-addref-refptr-more.patch (v1)
OK, that's the end of the series, my friend ;-)

I've done some serious slashing here.
Attachment #8908722 - Flags: review?(acelists)
Comment on attachment 8908706 [details] [diff] [review]
Part 17: 1340972-addref-import.patch (v1c) [Landed: comment #111]

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

Thanks.
Attachment #8908706 - Flags: review+
Needed more slashing, sorry about the noise.
Attachment #8908722 - Attachment is obsolete: true
Attachment #8908722 - Flags: review?(acelists)
Attachment #8908726 - Flags: review?(acelists)
Comment on attachment 8908722 [details] [diff] [review]
Part 19: 1340972-addref-refptr-import.patch (v1)

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

I'm not sure I really understand all of these.

::: mailnews/import/src/nsImportMail.cpp
@@ +84,5 @@
>    static void ReportError(int32_t id, const char16_t *pName, nsString *pStream, nsIStringBundle* aBundle);
>  
>  private:
> +  nsString                  m_pName;  // module name that created this interface
> +  nsCOMPtr<nsIMsgFolder>    m_pDestFolder;

Why not RefPtr as the others?

@@ +242,4 @@
>      return NS_ERROR_NULL_POINTER;
>  
>    if (!PL_strcasecmp(dataId, "mailInterface")) {
>      NS_IF_RELEASE(m_pInterface);

Why are the other releases of m_pInterface gone, but not this one?

::: mailnews/import/text/src/nsTextAddress.h
@@ +46,5 @@
>    char16_t m_delim;
>    int32_t m_LFCount;
>    int32_t m_CRCount;
> +  nsCOMPtr<nsIAddrDatabase>   m_database;
> +  nsCOMPtr<nsIImportFieldMap> m_fieldMap;

Don't you need RefPtr for these 2 if you got rid of the ADDREFs on them?
Attachment #8908722 - Attachment is obsolete: false
Attachment #8908722 - Attachment is obsolete: true
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/97d9755a68a3
Part 16: replace xxx.swap(*yyy) with xxx.forget(yyy) in mailnews. r=aceman
https://hg.mozilla.org/comm-central/rev/3704436e2afb
Part 17: Fix use of NS_ADDREF() and NS_IF_ADDREF() in mailnews/import. r=aceman
Sorry, you commented on the superseded patch.

(In reply to :aceman from comment #110)
> > +  nsCOMPtr<nsIMsgFolder>    m_pDestFolder;
> Why not RefPtr as the others?
For interface classes, I can use nsCOMPtr. Same deal.

> >      NS_IF_RELEASE(m_pInterface);
> Why are the other releases of m_pInterface gone, but not this one?
I forgot it.

> > +  nsCOMPtr<nsIAddrDatabase>   m_database;
> > +  nsCOMPtr<nsIImportFieldMap> m_fieldMap;
> Don't you need RefPtr for these 2 if you got rid of the ADDREFs on them?
nsCOMPtr is also refcounted.
Attachment #8908622 - Attachment description: Part 16: 1340972-swap-forget.patch (v2) → Part 16: 1340972-swap-forget.patch (v2) [Landed: comment #111]
Attachment #8908706 - Attachment description: Part 17: 1340972-addref-import.patch (v1c) → Part 17: 1340972-addref-import.patch (v1c) [Landed: comment #111]
Attachment #8908715 - Flags: review?(acelists) → review+
Attachment #8908726 - Flags: review?(acelists) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2660f4337132
Part 18: Replace more error-prone Addref()/Release() with using RefPtr in mailnews/. r=aceman
Comment on attachment 8908715 [details] [diff] [review]
Part 18: 1340972-addref-refptr-more.patch (v1) [Landed: comment #113]

Landed with one harmless additional hunk:
https://hg.mozilla.org/comm-central/rev/2660f4337132dfcb6d9d4ccefd365f4353eff28c#l7.12

-  if (mBundle)
-    mBundle->Release();
-  mBundle = nullptr;
+  NS_IF_RELEASE(mBundle);
Attachment #8908715 - Attachment description: Part 18: 1340972-addref-refptr-more.patch (v1) → Part 18: 1340972-addref-refptr-more.patch (v1) [Landed: comment #113]
Don't be shocked. I ripped out a lot of stuff and made all pointers smart pointers.
Attachment #8908726 - Attachment is obsolete: true
Attachment #8908873 - Flags: review?(acelists)
Comment on attachment 8908873 [details] [diff] [review]
1340972-addref-refptr-import.patch (v2)

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

Thanks for the cleanup. Nice that the code can be simplified and reduced.

::: mailnews/import/outlook/src/nsOutlookImport.cpp
@@ +218,5 @@
>            nsCOMPtr<nsISupportsString> nameString (do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID, &rv));
>            if (NS_SUCCEEDED(rv)) {
>              nameString->SetData(name);
>              pGeneric->SetData("name", nameString);
> +            nsCOMPtr<nsISupports> pInterface = do_QueryInterface(pGeneric);

You can keep the rv here, like = do_QueryInterface(pGeneric, &rv); in multiple places.

::: mailnews/import/outlook/src/nsOutlookStringBundle.cpp
@@ +23,2 @@
>  
> +  char*  propertyURL = OUTLOOK_MSGS_URL;

Why 2 spaces? And why is this variable even needed? E.g. becky does not use it and passes *_MSGS_URL to CreateBundle() directly.

::: mailnews/import/vcard/src/nsVCardImport.cpp
@@ +156,3 @@
>          if (NS_SUCCEEDED(rv)) {
>            pGeneric->SetData("addressInterface", pAddress);
> +          nsCOMPtr<nsISupports> pInterface = do_QueryInterface(pGeneric);

&rv
Attachment #8908873 - Flags: review?(acelists) → review+
(In reply to :aceman from comment #116)
> > +            nsCOMPtr<nsISupports> pInterface = do_QueryInterface(pGeneric);
> You can keep the rv here, like = do_QueryInterface(pGeneric, &rv); in
> multiple places.
If I can't QI down to the very base class nsISupports, I'm in serious trouble. The original code didn't check either. You are right that in cases like
nsCOMPtr<nsISupportsString> nameString = do_QueryInterface(item);
I have to either check the return code or the result, which is null if it fails. I fixed that in this version.

> Why 2 spaces? And why is this variable even needed? E.g. becky does not use
> it and passes *_MSGS_URL to CreateBundle() directly.
Done that.

Please use interdiff. Later today I will give this some thorough testing before landing it. I'll report back.
Attachment #8908873 - Attachment is obsolete: true
Attachment #8908963 - Flags: review+
mozilla/mach xpcshell-test mailnews/import/test/unit/test_becky_addressbook.js
mozilla/mach xpcshell-test mailnews/import/test/unit/test_becky_filters.js
still working. Are there no tests for Becky Mail and Becky Settings. That's offered in the menus? Is there some test data? Aceman, you worked on Becky, can you point me to it or run some tests for me?

I'll exercise all the other menu items. Here on Windows I have:
Address Books: Becky - hopefully covered by test, Text, vCard - will do manually.
Mail: Becky - how to test?
Feeds: I don't think I touched this.
Settings: Becky - how to test? Windows Live Mail, already fails in Daily.
Filters: Becky - hopefully covered by test.

So I'll run some address book stuff and make sure WLM still fails the same ;-)
Aceman, can you assist with Becky Mail and Settings?

BTW, I didn't touch Apple import, which is broken anyway, bug 688166.
(In reply to Jorg K (GMT+2) from comment #117)
> (In reply to :aceman from comment #116)
> > > +            nsCOMPtr<nsISupports> pInterface = do_QueryInterface(pGeneric);
> > You can keep the rv here, like = do_QueryInterface(pGeneric, &rv); in
> > multiple places.
> If I can't QI down to the very base class nsISupports, I'm in serious
> trouble. The original code didn't check either.

It didn't check directly, but it returned rv out of the function.

(In reply to Jorg K (GMT+2) from comment #118)
> Aceman, can you assist with Becky Mail and Settings?

Becky import isn't officially exposed on Linux. But I have a hacky patch to expose it and I think I can import ABs and filters.
I will do that.
(In reply to :aceman from comment #119)
> It didn't check directly, but it returned rv out of the function.
Yes, some passed it out (4x), others ignored it and returned NS_OK anyway (3x). I don' think there is a problem QI'ing to nsISupports, but I can restore it if it makes you happy.

> Becky import isn't officially exposed on Linux. But I have a hacky patch to
> expose it and I think I can import ABs and filters.
> I will do that.
Thanks, these two are covered by tests, I would like to cover mail and settings.
OK, you can ignore the nsISupports, but please do the others.
OK, I'll revise it later today. Meanwhile I spotted a bug in the current patch which escaped review in nsWMImport.cpp ;-(

   if (!strcmp(pImportType, "settings")) {
-    nsIImportSettings *pSettings = nullptr;
-    rv = nsWMSettings::Create(&pSettings);
+    nsCOMPtr<nsIImportSettings> pSettings;
+    rv = nsWMSettings::Create(getter_AddRefs(pSettings));
     if (NS_SUCCEEDED(rv)) {
+      nsCOMPtr<nsISupports> pInterface = do_QueryInterface(pSettings);
+      pInterface.forget(ppInterface);
       pSettings->QueryInterface(kISupportsIID, (void **)ppInterface); <<==== delete this line.
     }
-    NS_IF_RELEASE(pSettings);
     return rv;
   }
(In reply to :aceman from comment #121)
> OK, you can ignore the nsISupports, but please do the others.

Coming back to this, let's do some bean counting: I added 17 do_QueryInterface() calls, of those 9 are to nsISupports, and 8 are not. Of those 8, two have an rv check, leaving these six:

m_pInterface = do_QueryInterface(item);
m_Books = do_QueryInterface(item);
m_pFieldMap = do_QueryInterface(item);
m_pInterface = do_QueryInterface(item);
m_pMailboxes = do_QueryInterface(item);
m_pDestFolder = do_QueryInterface(item);

These six replace code which didn't do error checking, so I'm not going to add it now.

So conclusion: Apart from fixing the issue mentioned in comment #122, I can no finish polishing mostly dead code and proceed to testing the bit that's still in use.
Removed left-over line in dead WLM code as per comment #122.

One day I'll get Outlook import working again and then we'll see whether it all still works. For now, I'll do some address book import as per comment #118.
Attachment #8908963 - Attachment is obsolete: true
Attachment #8909027 - Flags: review+
Address book import works fine, so this is going to land now.
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0c7ce1b538aa
Part 19: Replace error-prone Addref()/Release() with using smart pointers in mailnews/import. r=aceman
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
This was meant as an easy bug to mechanically change NS_IF_ADDREF(*result = x) to x.forget(result) ... and look what happened. Next I'll remove trailing spaces, that's easier.
Target Milestone: --- → Thunderbird 57.0
Attachment #8909027 - Attachment description: Part 19: 1340972-addref-refptr-import.patch (v2c) → Part 19: 1340972-addref-refptr-import.patch (v2c) [Landed: comment #126]
I think you expanded the scope :)
Anyway, it may be you fixed some real problems.
Now Wayne can observe the crash rates if they changed in any direction.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d5448dde5660
Part 20: Follow-up: Remove unused variable kISupportsIID. rs=bustage-fix DONTBUILD
Jolly good, that caused a test failure:
mozilla/mach xpcshell-test  mailnews/import/test/unit/test_bug_437556.js
Set pointers to null were a NS_IF_RELEASE() was removed.
This makes the test pass.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6e7d1615fa6a
Part 21: Follow-up: set pointers to null were a NS_IF_RELEASE() was removed. r=me DONTBUILD
Interesting.
Attachment #8909056 - Attachment description: Part 21: 1340972-null.patch - follow up. → Part 21: 1340972-null.patch - follow up [landed: comment #132]
(In reply to :aceman from comment #133)
> Interesting.
I'd call it annoying. NS_IF_RELEASE(x) is |if(x) { x->Release(); x = nullptr; }|. I killed many NS_IF_RELEASE()s since they are not required as the smart pointers take care of that, for example in DTORs, at the end of functions when local variables go out of scope or before assigning a new value. However, some of them had algorithmic significance. I've reviewed all the 54 I removed. Apart from the ones added in again in part 21, I found two more I will restore.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c6bf7317944f13bc9a712190ea84aab8b7daae0b
Part 22: Follow-up: set more pointers to null were a NS_IF_RELEASE() was removed. r=me DONTBUILD

Looks like Pulsebot is on strike on Sundays :-(
You need to log in before you can comment on or make changes to this bug.