Closed Bug 1386220 Opened 4 years ago Closed 4 years ago

Misc Addref/Release fixes

Categories

(MailNews Core :: Backend, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 56.0

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

Details

Attachments

(2 files, 1 obsolete file)

I've written these patches for bug 1342745, but since that bug never progressed, they never got landed.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8892427 - Flags: review?(acelists)
Comment on attachment 8892428 [details] [diff] [review]
1386220-addref-release-nsImapMailFolder.patch

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

I'm no expert in this but let's try it.
Attachment #8892428 - Flags: review?(acelists) → review+
Attachment #8892427 - Flags: review?(acelists) → review+
Well, the patch in attachment 8892427 [details] [diff] [review] is 100% clear:

  nsMsgSearchScopeTerm* scope = new nsMsgSearchScopeTerm(nullptr, nsMsgSearchScope::offlineMail, folder);
  scope->AddRef();
  if (!scope) return NS_ERROR_OUT_OF_MEMORY;

is 100% nonsense. First deference, then check for null :-(
Besides, 'new' never fails in Mozilla, if it doesn't work, it will terminate and not return.
The RefPtr does exactly what the AddRef(), Release() does, only that you don't have to write it and you don't need to track all the various code path that might forget the Release().

As for the other patch in attachment 8892428 [details] [diff] [review], that's more difficult, let's analyse it.

class nsImapMailFolder has a member variable:
  nsImapMoveCoalescer *m_moveCoalescer;

It does this with this member variable:
m_moveCoalescer = nullptr; in nsImapMailFolder::nsImapMailFolder():
  No longer required when switched to RefPtr since those initialise themselves.
NS_IF_RELEASE(m_moveCoalescer) in nsImapMailFolder::~nsImapMailFolder():
  That's necessary since in nsImapMailFolder::GetMoveCoalescer() 
  there was an m_moveCoalescer->AddRef().
  So when the owning object dies, it needs to count down the reference count with a Release().
  Switching to a RefPtr the Refptr will be destroyed in nsImapMailFolder::~nsImapMailFolder()
  and count down the reference count in the referenced object automatically.
NS_IF_RELEASE(m_moveCoalescer) in nsImapMailFolder::Shutdown():
  Somehow the shutdown logic wants to get rid of m_moveCoalescer.
  Just setting it to nullptr won't decrease the reference count.
  Since I switch to Refptr, setting this to nullptr will decrease the reference count
  in the referenced object. So the Release() changes to nulling it.
Now a little out of order:
m_moveCoalescer->AddRef() in nsImapMailFolder::GetMoveCoalescer():
  That's where the thing gets allocated with 'new', since we now assign to a
  Refptr, we can lose the AddRef() since this is what the RefPtr does for us.
That concludes the analysis for m_moveCoalescer.

Next we look at nsImapFolderCopyState.
This object gets created in nsImapMailFolder::CopyFolder() via
    nsImapFolderCopyState *folderCopier = new nsImapFolderCopyState(this, srcFolder, isMoveFolder, msgWindow, listener);
    NS_ADDREF(folderCopier); // it owns itself.
So the object gets created and needs to live until all the interested parties are done with it. So the creation does and AddRef() and the interested parties in nsImapFolderCopyState::AdvanceToNextFolder(), nsImapFolderCopyState::OnStopRunningUrl() and nsImapFolderCopyState::OnStopCopy() do a Release() which will decrease the reference count compensating for the initial AddRef(). Well all interested parties are done with the object and release their reference, the object will die.

In the new scheme we have
+    RefPtr<nsImapFolderCopyState> folderCopier =
+      new nsImapFolderCopyState(this, srcFolder, isMoveFolder, msgWindow, listener);
     return folderCopier->StartNextCopy();
So what happens? A new object is created with a reference count of one since it's assigned to a RefPtr.
When nsImapMailFolder::CopyFolder() returns, 'folderCopier' is destroyed and the reference it added gets counted down. So the referenced parties no longer have to do a Release() to get rid of that initial reference so the object can die in peace.

Now, why does copying still work when returning from nsImapMailFolder::CopyFolder() decreases the refcount and potentially destroys the object. The answer is that all the interested parties establish their own references. Debugging shows that after the call to folderCopier->StartNextCopy() the mRefCnt is 2, one reference from 'folderCopier' which is about to die and one from an interested party. The object will die when that interested party goes away.

I hope I convinced you ;-)
Removed needed check after 'new'.
Attachment #8892428 - Attachment is obsolete: true
Attachment #8892664 - Flags: review+
Boy, it's too late: This was meant to be: Removed *needless* check after 'new'.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/317ad7aa38be
AddRef/Release fix in nsMsgFilterList.cpp. r=aceman
https://hg.mozilla.org/comm-central/rev/76541ad94755
AddRef/Release fix in nsImapMailFolder.cpp. r=aceman
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
BTW, we did the same thing here:
https://hg.mozilla.org/comm-central/rev/a10e7c19d7f373693e2a193204f17b4de3f7f741#l1.12

There I had erroneously added a 'delete' here:
https://hg.mozilla.org/comm-central/rev/ef7420896e68b4a9ffd30c7ad50ac0cb9c25b990#l2.12

But you can't delete the object since the preceding call downloadState->DownloadArticles() grabbed a reference. So if you delete the object, you get a crash. So RefPtr is our friend, just like in this bug here.
Target Milestone: --- → Thunderbird 56.0
Version: unspecified → Trunk
There is however, a shadow of a doubt remaining. If what I said in comment #4 in the second last paragraph "Now, why does copying still work ..." were true, why didn't the original code do this:

    nsImapFolderCopyState *folderCopier = new nsImapFolderCopyState(this, srcFolder, isMoveFolder, msgWindow, listener);
    NS_ADDREF(folderCopier); // it owns itself.
    rv = folderCopier->StartNextCopy();
    folderCopier->Release();
    return rv;

since this is what the RefPtr does now. Instead, it did the release three times in nsImapFolderCopyState::AdvanceToNextFolder(), nsImapFolderCopyState::OnStopRunningUrl() and nsImapFolderCopyState::OnStopCopy(). So someone obviously thought that the object needed to survive until there (which is true) by holding the initial reference as long as possible, which doesn't appear to be necessary since copying folders (between servers) still works without it since some other interested party holds a reference.
Summary: Misc Addref/Release fixes → Misc Addref/Release leak fixes
No, that didn't fix any leaks, it just removed code clumsiness with the risk to introduce more leaks.
Summary: Misc Addref/Release leak fixes → Misc Addref/Release fixes
You need to log in before you can comment on or make changes to this bug.