Crashes in debug mode on NS_ERROR() and by releasing URL on IMAP tread

RESOLVED FIXED in Thunderbird 53.0

Status

MailNews Core
Networking: IMAP
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

Trunk
Thunderbird 53.0

Thunderbird Tracking Flags

(thunderbird52 fixed, thunderbird53 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 months ago
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.
(Assignee)

Comment 1

4 months ago
Created attachment 8820411 [details] [diff] [review]
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.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8820411 - Flags: review?(rkent)

Comment 2

4 months ago
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;
Attachment #8820411 - Flags: review?(rkent) → review-
(Assignee)

Comment 3

4 months ago
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?
Flags: needinfo?(rkent)

Comment 4

4 months ago
"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.
Flags: needinfo?(rkent)
(Assignee)

Comment 5

4 months ago
Created attachment 8820852 [details] [diff] [review]
1324893-ns-error-release-url.patch (v2).

OK, this is as you said.
Attachment #8820411 - Attachment is obsolete: true
Attachment #8820852 - Flags: review?(rkent)

Updated

4 months ago
Attachment #8820852 - Flags: review?(rkent) → review+
(Assignee)

Comment 6

4 months ago
https://hg.mozilla.org/comm-central/rev/62ac6dac1858af31af2ed4a9030637213f1800bf
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-thunderbird52: --- → affected
status-thunderbird53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
(Assignee)

Comment 7

4 months ago
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.
Attachment #8820852 - Flags: approval-comm-aurora+
(Assignee)

Comment 8

4 months ago
Aurora (TB 52):
https://hg.mozilla.org/releases/comm-aurora/rev/8db1cdbf971ab9543a22d64cfea71cde0dfdb8f6
status-thunderbird52: affected → fixed
You need to log in before you can comment on or make changes to this bug.