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

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Jorg K (CEST = GMT+2)]

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 ;-)

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1340972-addref-compose.patch

Here's my new playground to create a stream of patches ;-)

Comment 2

[:aceman]

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 ;)

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1340972-addref-local.patch

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1340972-addref-local.patch (v2).

Sorry, forgot to visit the NS_ADDREF()s as well.

Comment 5

[:aceman]

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.

Comment 6

[:aceman]

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 ;)

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: Part 1: 1340972-addref-compose.patch (v2) [Landed: comment #23]

Sorry for doing this for you, now also visiting NS_ADDREF().

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: Part 2: 1340972-addref-local.patch (v2b) [Landed: comment #23]

Only white-space fixes as requested. I'll answer your questions in the next comment.

Comment 9

[Jorg K (CEST = GMT+2)]

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

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1340972-addref-refptr.patch (v1).

I know you love Addref()/Release() reviews, so here's another one ;-)

Comment 11

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ccfc9ca7048d231b52ed0b57641e3b7e67c5dd5a

Comment 12

[:aceman]

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.

Comment 13

[:aceman]

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?

Comment 14

[Jorg K (CEST = GMT+2)]

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

[Jorg K (CEST = GMT+2)]

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.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1340972-addref-local.patch (v2)

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

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: 1340972-addref-refptr-compose-local.patch (v2)

Sorry, I put this into the wrong patch.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: 1340972-addref-db.patch (v1)

Here's the clean-up in mailnews/db. Another patch to follow for the ugly Addref(0 business.

Comment 19

[Jorg K (CEST = GMT+2)]

OK, so the try for the first three patches is green:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fe944a1daa49b88164b8b91c18a1e0dfaabf7490

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: 1340972-addref-refptr-compose-local.patch (v2b)

Sorry, previous patch wasn't quite right. Release also nulls, so we need to do this here.

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: Part 3: 1340972-addref-refptr-compose-local.patch (v2c)

Sorry, typo.

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: Part 4: 1340972-addref-db.patch (v2)

One more ugly thing to remove.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
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 24

[Jorg K (CEST = GMT+2)]

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);

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: Part 4: 1340972-addref-db.patch (v3) [Landed: comment #49]

Sorry about the noise, found some more ugliness.

Comment 26

[Jorg K (CEST = GMT+2)]

Attachment: Part 5: 1340972-addref-refptr-db.patch (v1)

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.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: Part 6: 1340972-addref-build-extensions-mapi-mime.patch (v1) [Landed: comment #49]

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.

Comment 28

[Jorg K (CEST = GMT+2)]

Attachment: Part 3: 1340972-addref-refptr-compose-local.patch (v2d) [Landed: comment #49]

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

Comment 29

[:aceman]

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?

Comment 30

[Jorg K (CEST = GMT+2)]

Attachment: Part 7: 1340972-addref-news.patch (v1)

Comment 31

[Jorg K (CEST = GMT+2)]

Attachment: Part 7: 1340972-addref-news.patch (v1b) [Landed: comment #56]

Comment 32

[Jorg K (CEST = GMT+2)]

Attachment: Part 5: 1340972-addref-refptr-db.patch (v1b) [Landed: comment #49]

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

Comment 33

[Jorg K (CEST = GMT+2)]

Try up to part 6 looking good:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c59f926d81ac35f51307c0acdc3e1cbe379c9d36

Comment 34

[Jorg K (CEST = GMT+2)]

Attachment: Part 8: 1340972-addref-refptr-news.patch (v1) [Landed: comment #56]

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.

Comment 35

[:aceman]

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.

Comment 36

[:aceman]

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' ?

Comment 37

[Jorg K (CEST = GMT+2)]

Yes. The last reference dies, the object is destroyed.

Comment 38

[:aceman]

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.

Comment 39

[:aceman]

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

Comment 40

[Jorg K (CEST = GMT+2)]

Yes. That's the idea of refcounting. The last one closes the door.

Comment 41

[Jorg K (CEST = GMT+2)]

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

Comment 42

[:aceman]

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

Comment 43

[Jorg K (CEST = GMT+2)]

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

Comment 44

[Jorg K (CEST = GMT+2)]

Attachment: Part 9: 1340972-addref-imap.patch (v1) [Landed: comment #58]

Comment 45

[Jorg K (CEST = GMT+2)]

Attachment: Part 10: 1340972-addref-refptr-imap.patch (v1).

This is IMAP. Base, addressbook and import left.

Comment 46

[Jorg K (CEST = GMT+2)]

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.

Comment 47

[Jorg K (CEST = GMT+2)]

Attachment: Part 10: 1340972-addref-refptr-imap.patch (v2).

Comment 48

[Jorg K (CEST = GMT+2)]

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.

Comment 49

[Pulsebot]

Pushed by [e-mail]:
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

Comment 50

[Jorg K (CEST = GMT+2)]

News: Part 7+8:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=831d978007c9eda1952b5bf61368d62437e28f7e

Comment 51

[Jorg K (CEST = GMT+2)]

Attachment: Part 10: 1340972-addref-refptr-imap.patch (v3)

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

Comment 52

[Jorg K (CEST = GMT+2)]

Attachment: Part 10: 1340972-addref-refptr-imap.patch (v3b) [Landed: comment #66]

Looking at this again, kDiscoverBaseFolderInProgress didn't release the adoptedBoxSpec. Turns out that wasn't used, so it's gone now. Technical debt.

Comment 53

[:aceman]

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.

Comment 54

[Jorg K (CEST = GMT+2)]

Attachment: Part 11: 1340972-addref-addrbook.patch (v1) [Landed: comment #66]

Comment 55

[Jorg K (CEST = GMT+2)]

Try up to part 10 is green:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=391a763068509ae7a09efb754e78d7368d25d5e8

Try including part 11:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b59d90d34b288f0afcd32b1504564039032fe42e

Comment 56

[Pulsebot]

Pushed by [e-mail]:
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 57

[Jorg K (CEST = GMT+2)]

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

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b41092b15ade
Part 9: Fix use of NS_ADDREF() and NS_IF_ADDREF() in mailnews/imap. r=aceman

Comment 59

[:aceman]

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?

Comment 60

[:aceman]

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.

Comment 61

[Jorg K (CEST = GMT+2)]

(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+ ;-)

Comment 62

[Jorg K (CEST = GMT+2)]

(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+.

Comment 63

[:aceman]

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

Comment 64

[:aceman]

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.

Comment 65

[Jorg K (CEST = GMT+2)]

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

Comment 66

[Pulsebot]

Pushed by [e-mail]:
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

Comment 67

[Jorg K (CEST = GMT+2)]

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.

Comment 68

[Jorg K (CEST = GMT+2)]

Attachment: Part 12: 1340972-addref-base-search.patch (v1)

Nothing interesting here ;-)

Comment 69

[Jorg K (CEST = GMT+2)]

Attachment: Part 13: 1340972-addref-base-util.patch (v1) [Landed: comment #80]

Nothing interesting here.

Comment 70

[:aceman]

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?

Comment 71

[:aceman]

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.

Comment 72

[Jorg K (CEST = GMT+2)]

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

Comment 73

[:aceman]

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));

Comment 74

[Jorg K (CEST = GMT+2)]

Attachment: Part 12: 1340972-addref-base-search.patch (v1b)

Removed unneeded static cast. Happy?

Comment 75

[:aceman]

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

Comment 76

[Jorg K (CEST = GMT+2)]

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);

Comment 77

[Jorg K (CEST = GMT+2)]

Attachment: Part 14: 1340972-addref-base-src.patch (v1) [Landed: comment #98]

That's the straight forward stuff, the hard stuff is coming.

Comment 78

[Jorg K (CEST = GMT+2)]

Attachment: Part 12: 1340972-addref-base-search.patch (v1c) [Landed: comment #80]

(In reply to :aceman from comment #75)
> What about this:
> >    filterList->SetFolder(aFolder);
> >    filterList->m_temporaryList = true;
> >    NS_ADDREF(*aFilterList = filterList);
Yes.

Comment 79

[Jorg K (CEST = GMT+2)]

Attachment: Part 15: 1340972-addref-refptr-base.patch (v1) [Landed: comment #98]

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

Comment 80

[Pulsebot]

Pushed by [e-mail]:
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 81

[Jorg K (CEST = GMT+2)]

Comment on attachment 8907791 [details] [diff] [review]
Part 13: 1340972-addref-base-util.patch (v1) [Landed: comment #80]

I removed the unwanted space.

Comment 82

[Jorg K (CEST = GMT+2)]

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.

Comment 83

[Jorg K (CEST = GMT+2)]

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

Comment 84

[Jorg K (CEST = GMT+2)]

This is green, apart from the usual failures. I re-ran opt X, with luck it will be green as debug X.

Comment 85

[:aceman]

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?

Comment 86

[Jorg K (CEST = GMT+2)]

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 87

[:aceman]

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

Comment 88

[:aceman]

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

Comment 89

[Jorg K (CEST = GMT+2)]

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

Comment 90

[Jorg K (CEST = GMT+2)]

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.

Comment 91

[Jorg K (CEST = GMT+2)]

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.

Comment 92

[Jorg K (CEST = GMT+2)]

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 93

[:aceman]

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

Comment 94

[:aceman]

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

Comment 95

[Jorg K (CEST = GMT+2)]

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 ;-)

Comment 96

[Jorg K (CEST = GMT+2)]

Attachment: Part 16: 1340972-swap-forget.patch (v1)

find mailnews -name *.cpp -exec sed -i -e 's/\.swap(\*/\.forget(/' {} \;

Comment 97

[Jorg K (CEST = GMT+2)]

Attachment: Part 16: 1340972-swap-forget.patch (v1b)

Oops, forgot the Mac file.

Comment 98

[Pulsebot]

Pushed by [e-mail]:
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

Comment 99

[:aceman]

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

Comment 100

[Jorg K (CEST = GMT+2)]

Attachment: Part 16: 1340972-swap-forget.patch (v2) [Landed: comment #111]

Goodness gracious, what was that ugly thing?

Comment 101

[:aceman]

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 ;)

Comment 102

[Jorg K (CEST = GMT+2)]

Attachment: Part 17: 1340972-addref-import.patch (v1)

OK, this is the easy import stuff.

Comment 103

[:aceman]

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.

Comment 104

[Jorg K (CEST = GMT+2)]

Attachment: Part 17: 1340972-addref-import.patch (v1b)

OK, I fixed all the preconditions in the vicinity I touched, but there are many more.

Comment 105

[Jorg K (CEST = GMT+2)]

Attachment: Part 17: 1340972-addref-import.patch (v1c) [Landed: comment #111]

Found some more.

Comment 106

[Jorg K (CEST = GMT+2)]

Attachment: Part 18: 1340972-addref-refptr-more.patch (v1) [Landed: comment #113]

While doing the "ugly" stuff for import, I saw some bits I had missed elsewhere. So here they are.

Comment 107

[Jorg K (CEST = GMT+2)]

Attachment: Part 19: 1340972-addref-refptr-import.patch (v1)

OK, that's the end of the series, my friend ;-)

I've done some serious slashing here.

Comment 108

[:aceman]

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.

Comment 109

[Jorg K (CEST = GMT+2)]

Attachment: Part 19: 1340972-addref-refptr-import.patch (v1b)

Needed more slashing, sorry about the noise.

Comment 110

[:aceman]

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?

Comment 111

[Pulsebot]

Pushed by [e-mail]:
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

Comment 112

[Jorg K (CEST = GMT+2)]

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.

Comment 113

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2660f4337132
Part 18: Replace more error-prone Addref()/Release() with using RefPtr in mailnews/. r=aceman

Comment 114

[Jorg K (CEST = GMT+2)]

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);

Comment 115

[Jorg K (CEST = GMT+2)]

Attachment: 1340972-addref-refptr-import.patch (v2)

Don't be shocked. I ripped out a lot of stuff and made all pointers smart pointers.

Comment 116

[:aceman]

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

Comment 117

[Jorg K (CEST = GMT+2)]

Attachment: Part 19: 1340972-addref-refptr-import.patch (v2b)

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

Comment 118

[Jorg K (CEST = GMT+2)]

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.

Comment 119

[:aceman]

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

Comment 120

[Jorg K (CEST = GMT+2)]

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

Comment 121

[:aceman]

OK, you can ignore the nsISupports, but please do the others.

Comment 122

[Jorg K (CEST = GMT+2)]

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;
   }

Comment 123

[Jorg K (CEST = GMT+2)]

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

Comment 124

[Jorg K (CEST = GMT+2)]

Attachment: Part 19: 1340972-addref-refptr-import.patch (v2c) [Landed: comment #126]

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.

Comment 125

[Jorg K (CEST = GMT+2)]

Address book import works fine, so this is going to land now.

Comment 126

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0c7ce1b538aa
Part 19: Replace error-prone Addref()/Release() with using smart pointers in mailnews/import. r=aceman

Comment 127

[Jorg K (CEST = GMT+2)]

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.

Comment 128

[:aceman]

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.

Comment 129

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d5448dde5660
Part 20: Follow-up: Remove unused variable kISupportsIID. rs=bustage-fix DONTBUILD

Comment 130

[Jorg K (CEST = GMT+2)]

Jolly good, that caused a test failure:
mozilla/mach xpcshell-test  mailnews/import/test/unit/test_bug_437556.js

Comment 131

[Jorg K (CEST = GMT+2)]

Attachment: Part 21: 1340972-null.patch - follow up [landed: comment #132]

Set pointers to null were a NS_IF_RELEASE() was removed.
This makes the test pass.

Comment 132

[Pulsebot]

Pushed by [e-mail]:
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

Comment 133

[:aceman]

Interesting.

Comment 134

[Jorg K (CEST = GMT+2)]

Attachment: Part 20: 1340972-compile-errors.patch - follow-up [landed: comment #129]

Comment 135

[Jorg K (CEST = GMT+2)]

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

Comment 136

[Jorg K (CEST = GMT+2)]

Attachment: 1340972-null-more.patch - follow up [landed: comment #137]

Comment 137

[Jorg K (CEST = GMT+2)]

Pushed by [e-mail]:
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 :-(

Comment 138

[Jorg K (CEST = GMT+2)]

And two more:
https://hg.mozilla.org/comm-central/rev/a1a7c1ffb60fb5c3be376e03cffff218181f20cd (backout of part 21)
https://hg.mozilla.org/comm-central/rev/aaf36a3aee783d2455a6d6a1f11e9ce9c408b414 (Part 21 corrected)