1324893 - Crashes in debug mode on NS_ERROR() and by releasing URL on IMAP thread

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Jorg K (CEST = GMT+2)]

In longer debug sessions I'm unlucky and crash like this:

[1816] ###!!! ASSERTION: missing url or sink: 'Error', file c:/mozilla-source/comm-central/mailnews/imap/src/nsImapProtocol.cpp, line 1823
Hit MOZ_CRASH(nsWeakReference not thread-safe) at c:/mozilla-source/comm-central/mozilla/xpcom/glue/nsWeakReference.cpp:139
#01: nsImapUrl::~nsImapUrl (c:\mozilla-source\comm-central\mailnews\imap\src\nsimapurl.cpp:79)
#02: nsImapUrl::`scalar deleting destructor'[C:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\bin\xul.dll +0x6f1754]
#03: nsMsgMailNewsUrl::Release (c:\mozilla-source\comm-central\mailnews\base\util\nsmsgmailnewsurl.cpp:56)
#04: nsImapUrl::Release (c:\mozilla-source\comm-central\mailnews\imap\src\nsimapurl.cpp:83)
#05: nsImapProtocol::ReleaseUrlState (c:\mozilla-source\comm-central\mailnews\imap\src\nsimapprotocol.cpp:1032)
#06: nsImapProtocol::ProcessCurrentURL (c:\mozilla-source\comm-central\mailnews\imap\src\nsimapprotocol.cpp:1844)
#07: nsImapProtocol::ImapThreadMainLoop (c:\mozilla-source\comm-central\mailnews\imap\src\nsimapprotocol.cpp:1393)
#08: nsImapProtocol::Run (c:\mozilla-source\comm-central\mailnews\imap\src\nsimapprotocol.cpp:1065)

Kent's comment in a private message was:

I looks to me like this code is odd:
https://dxr.mozilla.org/comm-central/rev/764af75d8f8cb66b4a998979a47dc19056eadf3e/mailnews/imap/src/nsImapProtocol.cpp#1024
  // we want to make sure the imap protocol's last reference to the url gets released
  // back on the UI thread. This ensures that the objects the imap url hangs on to
  // properly get released back on the UI thread.
  if (saveFolderSink)
  {
    NS_ReleaseOnMainThread(mailnewsurl.forget());
    saveFolderSink = nullptr;
  }
}

That is, I don't see what saveFolderSink has to do with NS_ReleaseOnMainThread(mailnewsurl.forget());  I'm guessing that the crash occurred because saveFolderSink was null.

and further ...

So, change the NS_ERROR
https://dxr.mozilla.org/comm-central/rev/764af75d8f8cb66b4a998979a47dc19056eadf3e/mailnews/imap/src/nsImapProtocol.cpp#1823
to NS_WARNING with some sort of recovery, and
make sure you also do not trigger an assertion by making sure that
NS_ReleaseOnMainThread(mailnewsurl.forget()); gets called.

Of course it would be good to figure out the original source of the
problem instead rather than just preventing assertion crashes here.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1324893-ns-error-release-url.patch (v1).

This shouldn't do any damage:
1) The NS_ERROR() only triggers in a debug compile, so
   now the behaviour is the same as a release build.
2) Always releasing the URL on the main thread is the way to go.

Sadly there is no way to reproduce this reliably, so I can't look at the reasons for the 'assert'. However, there is no damage done is bringing the debug build closer to the release build.

Comment 2

[Kent James (:rkent)]

Comment on attachment 8820411 [details] [diff] [review]
1324893-ns-error-release-url.patch (v1).

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

I'm afraid that I don't understand the threading issues well enough here to convince myself that saveFolderSink is unnecessary and can be removed. (See the original bug 391259). What I would prefer is that you simply change the release to:

  if (mailnewsurl)
  {
    NS_ReleaseOnMainThread(mailnewsurl.forget());
  }
  saveFolderSink = nullptr;

Comment 3

[Jorg K (CEST = GMT+2)]

Umm, let's take a look at your proposal:

  nsCOMPtr<nsIImapMailFolderSink> saveFolderSink;

  {
    MutexAutoLock mon(mLock);
    if (m_runningUrl)
    {
      mailnewsurl = do_QueryInterface(m_runningUrl);
      saveFolderSink = m_imapMailFolderSink;

      m_runningUrl = nullptr; // force us to release our last reference on the url
      m_urlInProgress = false;
    }
  }
  // Need to null this out whether we have an m_runningUrl or not
  m_imapMailFolderSink = nullptr;

  // we want to make sure the imap protocol's last reference to the url gets released
  // back on the UI thread. This ensures that the objects the imap url hangs on to
  // properly get released back on the UI thread.
  if (mailnewsurl)
  {
    NS_ReleaseOnMainThread(mailnewsurl.forget());
  }
  saveFolderSink = nullptr;
}

Looks to me that 'saveFolderSink' is never read. So why have it? I was only used in the |if (saveFolderSink)| which we will be removing. What am I missing? Is there some magic going on that you're still holding a reference to something via this variable?

Comment 4

[Kent James (:rkent)]

"What am I missing? Is there some magic going on that you're still holding a reference to something via this variable?"

I don't know if there is some magic or not, which is why I think it is risky to throw it away. I don't really understand why it was added in the first place, but it was clearly simply to hold a reference, so being read or not is not relevant.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1324893-ns-error-release-url.patch (v2).

OK, this is as you said.

Comment 6

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/62ac6dac1858af31af2ed4a9030637213f1800bf

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 8820852 [details] [diff] [review]
1324893-ns-error-release-url.patch (v2).

Let's take this to Aurora like bug 1299920 so we get all the "let's release this on the main thread" goodness in the one version.

Comment 8

[Jorg K (CEST = GMT+2)]

Aurora (TB 52):
https://hg.mozilla.org/releases/comm-aurora/rev/8db1cdbf971ab9543a22d64cfea71cde0dfdb8f6