1942876 - Improve thread safety of nsImapProtocol.cpp

Status: NEW

(MailNews Core :: Networking: IMAP, defect)

Description

[Kai Engert [:KaiE:]]

The locking logic in nsImapProtocol.cpp worries me.

Multiple protection mechanisms are used in parallel.

It isn't immediately clear which mechanism is supposed to protect which data objects.

There's (a) mLock, there are (b) multiple monitors, and in addition, it's using (c) PR_CEnterMonitor on the instance pointer.

All of this is still insufficient, or rather, not well coordinated, because we still have thread data races, as reported by our automatic TSAN checker.

While we tried to fix some of the reports from that tool, a remaining report is about a race for m_runningUrl, e.g.
between nsImapProtocol::ReleaseUrlState() which uses PR_CEnterMonitor(),
and nsImapProtocol::ReleaseUrlState() which locks mLock.

It isn't obvious what to do here, because of the different locking used. Also I see that m_runningUrl is accessed read-only in multiple places, so apparently the code hopes that reading a pointer is safe (that's not completely safe).

It should be analyzed in more detail how the above parallel write access should be protected.

The above aren't new mechanisms. Based on code history, the monitor/lock setting in the above two places were last touched by David Bienvenu in 2011.