Joshua Cranmer [:jcranmer] Reporter
	Description • 13 years ago

Since <http://hg.mozilla.org/mozilla-central/rev/4beec31b9ea9>, mozilla-central has removed nsAutoLock.h, which removes nsAutoLock, nsAutoUnlock, nsAutoMonitor, and nsAutoCMonitor.

grep indicates that IMAP (via monitors) and address book (via locks) code are the only consumers of this in comm-central.

	Mark Banner (:standard8)
	Comment 1 • 13 years ago

I believe bienvenu may be working on this already, if people want to help, please co-ordinate with him.

	David :Bienvenu Assignee
	Comment 2 • 13 years ago

yes, I'm working on this. I'm starting with the imap code, so if anyone wants to look at the address book issues, you won't be stepping on my toes, for now.

	Ian Neal
	Comment 3 • 13 years ago

What about ldap/xpcom? Does that count as address book?

	David :Bienvenu Assignee
	Comment 4 • 13 years ago

(In reply to comment #3)
> What about ldap/xpcom? Does that count as address book?

yes, or at least, it counts as "not imap". If you want to look at the ldap code, that would be great.

	Ian Neal
	Comment 5 • 13 years ago

This patch follows the pattern of removal from part 4 of bug 645263
I've tested it compiles, but without the rest of the code being patched, cannot test if it works.

	Ian Neal
	Comment 6 • 13 years ago

Again, patch compiles but cannot complete testing until ldap code is patched.

	Ian Neal
	Comment 7 • 13 years ago

For the ldap code, have to look at the changes
from nsAutoCMon to nsAutoMon (bug 58904 and bug 489135)
from nsAutoMon to MonitorAutoEnter (bug 645263)

	Joshua Cranmer [:jcranmer] Reporter
	Comment 8 • 13 years ago

(In reply to comment #5)
> Created attachment 523472 [details] [diff] [review]
> Remove autolock from ldap code patch v1.0
> 
> This patch follows the pattern of removal from part 4 of bug 645263
> I've tested it compiles, but without the rest of the code being patched, cannot
> test if it works.

That's what bug 647003 was for, but I guess I didn't mention it well enough ?

	neil@parkwaycc.co.uk
	Comment 9 • 13 years ago

Comment on attachment 523487 [details] [diff] [review]
Remove autolock from addrbook code patch v1.0

>nsWabAddressBook.cpp(109) : error C2065: 'MutexAutoLock' : undeclared identifier
>nsWabAddressBook.cpp(109) : error C2146: syntax error : missing ';' before identifier 'guard'
>nsWabAddressBook.cpp(109) : error C3861: 'guard': identifier not found
>nsWabAddressBook.cpp(117) : error C2146: syntax error : missing ';' before identifier 'guard'
>nsWabAddressBook.cpp(117) : error C3861: 'guard': identifier not found

>nsAbWinHelper.cpp(277) : error C2438: 'mMutex' : cannot initialize static class data via constructor

	neil@parkwaycc.co.uk
	Comment 10 • 13 years ago

(In reply to comment #9)
>(From update of attachment 523487 [details] [diff] [review])
>>nsWabAddressBook.cpp(109) : error C2065: 'MutexAutoLock' : undeclared identifier
>>nsWabAddressBook.cpp(109) : error C2146: syntax error : missing ';' before identifier 'guard'
>>nsWabAddressBook.cpp(109) : error C3861: 'guard': identifier not found
>>nsWabAddressBook.cpp(117) : error C2146: syntax error : missing ';' before identifier 'guard'
>>nsWabAddressBook.cpp(117) : error C3861: 'guard': identifier not found
This is just a missing using namespace mozilla;

>>nsAbWinHelper.cpp(277) : error C2438: 'mMutex' : cannot initialize static class data via constructor
This was a class member, and it doesn't seem to have been converted correctly.

	David :Bienvenu Assignee
	Comment 11 • 13 years ago

remember that mailnews code needs to compile against both mozilla 2.0 and mozilla trunk, so you're going to need #ifdef MOZILLA_2_0_BRANCH.

	David :Bienvenu Assignee
	Comment 12 • 13 years ago

e.g., MutexAutoLock doesn't exist in Mozilla 2.0

We may want to use an #idfeffed MOZILLA_2_0_BRANCH for MutexAutoLock as nsAutoLock or something similar to reduce the amount of #ifdefs. I'll know more about what works when I've tackled the imap code.

	David :Bienvenu Assignee
	Comment 13 • 13 years ago

this gets nsAbWinHelper.cpp compiling with mozilla 2.0 (and I believe mozilla trunk)

	Joshua Cranmer [:jcranmer] Reporter
	Comment 14 • 13 years ago

(In reply to comment #12)
> e.g., MutexAutoLock doesn't exist in Mozilla 2.0

<http://mxr.mozilla.org/mozilla2.0/source/xpcom/glue/Mutex.h?mark=166#166> disagrees with you.

	David :Bienvenu Assignee
	Comment 16 • 13 years ago

Comment on attachment 523631 [details] [diff] [review]
cumulative patch that compiles on windows
[Checked in: Comment 34]

this does compile with 2.0 so we could switch to MutexLock though it makes me nervous to switch our locking mechanisms close to a release.

	David :Bienvenu Assignee
	Comment 17 • 13 years ago

With all three patches, I can build and run the trunk. I haven't tried the imap patch with mozilla 2.0 yet.

	neil@parkwaycc.co.uk
	Comment 19 • 13 years ago

Why are you removing some but adding other new calls to PR_CEnterMonitor?

	David :Bienvenu Assignee
	Comment 20 • 13 years ago

(In reply to comment #19)
> Why are you removing some but adding other new calls to PR_CEnterMonitor?

In general, I'd prefer to use the stack type locks, which is why I removed some of them. I was trying to use nsAutoMon before it went away, and it doesn't allow the same thread to re-enter the same monitor, which is why I switched some code to use PR_CEnterMonitor.

	Ian Neal
	Comment 21 • 13 years ago

Comment on attachment 523669 [details] [diff] [review]
patch that builds w/ 2.0 as well

>+++ b/mailnews/imap/src/nsImapIncomingServer.cpp
>@@ -104,7 +105,7 @@ NS_INTERFACE_MAP_BEGIN(nsImapIncomingSer
>   NS_INTERFACE_MAP_ENTRY(nsIUrlListener)
> NS_INTERFACE_MAP_END_INHERITING(nsMsgIncomingServer)
> 
>-nsImapIncomingServer::nsImapIncomingServer()
>+nsImapIncomingServer::nsImapIncomingServer() : mLock("imapServer")
To match the form elsewhere in the patch shouldn't this be mLock("nsImapIncomingServer")? The firefox patches seem to be of the form mLock("nsImapIncomingServer.mLock"), should we be trying to match that?

>@@ -3181,15 +3182,33 @@ NS_IMETHODIMP nsImapIncomingServer::SetT
>   nsresult rv = GetTrashFolderName(oldTrashName);
>   if (NS_SUCCEEDED(rv))
>   {
>-    nsCAutoString oldTrashNameUtf7;
>-    rv = CopyUTF16toMUTF7(oldTrashName, oldTrashNameUtf7);
>-    if (NS_SUCCEEDED(rv))
>+    nsCOMPtr<nsIMsgFolder> oldFolder;
>+    // oldTrashName might be a partial URI
>+    nsCOMPtr<nsIMsgFolder> rootFolder;
>+    rv = GetRootFolder(getter_AddRefs(rootFolder));
>+    if (NS_SUCCEEDED(rv) && rootFolder)
>     {
>-      nsCOMPtr<nsIMsgFolder> oldFolder;
>-      rv = GetFolder(oldTrashNameUtf7, getter_AddRefs(oldFolder));
>-      if (NS_SUCCEEDED(rv) && oldFolder)
>-        oldFolder->ClearFlag(nsMsgFolderFlags::Trash);
>+      nsCString folderUri;
>+      CopyUTF16toUTF8(oldTrashName, folderUri);
>+      nsCString uri;
>+      rv = rootFolder->GetURI(uri);
>+      uri.Append('/');
>+      uri.Append(folderUri);
>+      rv = rootFolder->GetChildWithURI(uri, PR_TRUE, PR_FALSE, getter_AddRefs(oldFolder));
>     }
>+    // oldTrashName might be the path name
>+    if (!oldFolder)
>+    {
>+      nsCAutoString oldTrashNameUtf7;
>+      rv = CopyUTF16toMUTF7(oldTrashName, oldTrashNameUtf7);
>+      if (NS_SUCCEEDED(rv))
>+      {
>+        nsCOMPtr<nsIMsgFolder> oldFolder;
>+        rv = GetFolder(oldTrashNameUtf7, getter_AddRefs(oldFolder));
>+      }
>+    }
>+    if (NS_SUCCEEDED(rv) && oldFolder)
>+      oldFolder->ClearFlag(nsMsgFolderFlags::Trash);
>   }
>   return SetUnicharValue(PREF_TRASH_FOLDER_NAME, chvalue);
> }
>+++ b/mailnews/imap/src/nsImapMailFolder.cpp
>@@ -425,26 +425,27 @@ nsresult nsImapMailFolder::AddSubfolderW
>   folder->SetParent(this);
>   flags |= nsMsgFolderFlags::Mail;
> 
>-  PRInt32 pFlags;
>-  GetFlags ((PRUint32 *) &pFlags);
>-  PRBool isParentInbox = pFlags & nsMsgFolderFlags::Inbox;
>-
>   nsCOMPtr<nsIImapIncomingServer> imapServer;
>   rv = GetImapIncomingServer(getter_AddRefs(imapServer));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  //Only set these if these are top level children or parent is inbox
>   if (isInbox)
>     flags |= nsMsgFolderFlags::Inbox;
>-  else if (isServer || isParentInbox)
>+  else
>   {
>     nsMsgImapDeleteModel deleteModel;
>     imapServer->GetDeleteModel(&deleteModel);
>     if (deleteModel == nsMsgImapDeleteModels::MoveToTrash)
>     {
>+      nsCString trashUri;
>+      nsCOMPtr<nsIMsgFolder> rootFolder;
>+      GetRootFolder(getter_AddRefs(rootFolder));
>+      rootFolder->GetURI(trashUri);
>+      trashUri.Append('/');
>       nsAutoString trashName;
>       GetTrashFolderName(trashName);
>-      if (name.Equals(trashName))
>+      AppendUTF16toUTF8(trashName, trashUri);
>+      if (uri.Equals(trashUri, nsCaseInsensitiveCStringComparator()))
>         flags |= nsMsgFolderFlags::Trash;
>     }
>   }
Are the changes above to do with this bug or something from another unrelated patch?

>+++ b/mailnews/imap/src/nsImapProtocol.cpp
>@@ -1001,9 +971,9 @@ void nsImapProtocol::ReleaseUrlState(PRB
>       m_mockChannel->Close();
> 
>     {
>-      // grab a monitor so m_mockChannel doesn't get cleared out
>+      // grab a lock so m_mockChannel doesn't get cleared out
>       // from under us.
>-      nsAutoCMonitor mon(this);
>+       MutexAutoLock mon(mLock);
Nit: indentation

>@@ -4199,30 +4171,30 @@ void nsImapProtocol::WaitForPotentialLis
> {
>   PRIntervalTime sleepTime = kImapSleepTime;
> 
>-    PR_EnterMonitor(m_fetchMsgListMonitor);
>+    m_fetchMsgListMonitor.Enter();
>     while(!m_fetchMsgListIsNew && !DeathSignalReceived())
>-        PR_Wait(m_fetchMsgListMonitor, sleepTime);
>+        m_fetchMsgListMonitor.Wait(sleepTime);
Nit: could fix this indentation whilst you are here (though I find some of the indentation confusing on this file, so it might be correct).

> void nsImapProtocol::WaitForPotentialListOfBodysToFetch(PRUint32 **msgIdList, PRUint32 &msgCount)
> {
>   PRIntervalTime sleepTime = kImapSleepTime;
> 
>-    PR_EnterMonitor(m_fetchBodyListMonitor);
>+    m_fetchBodyListMonitor.Enter();
>     while(!m_fetchBodyListIsNew && !DeathSignalReceived())
>-        PR_Wait(m_fetchBodyListMonitor, sleepTime);
>+        m_fetchBodyListMonitor.Wait(sleepTime);
Nit: again, as above comment.

>+++ b/mailnews/imap/src/nsImapProtocol.h
>@@ -88,6 +88,7 @@
> #include "nsAutoPtr.h"
> #include "nsIMsgFolder.h"
> #include "nsIMsgAsyncPrompter.h"
>+#include "mozilla/Monitor.h"
Is Mutex.h included elsewhere for mLock (below)?

>+  mozilla::Monitor m_dataAvailableMonitor;   // used to notify the arrival of data from the server
>+  mozilla::Monitor m_urlReadyToRunMonitor;   // used to notify the arrival of a new url to be processed
>+  mozilla::Monitor m_pseudoInterruptMonitor;
>+  mozilla::Monitor m_dataMemberMonitor;
>+  mozilla::Monitor m_threadDeathMonitor;
>+  mozilla::Monitor m_waitForBodyIdsMonitor;
>+  mozilla::Monitor m_fetchMsgListMonitor;
>+  mozilla::Monitor m_fetchBodyListMonitor;
>+  mozilla::Monitor m_passwordReadyMonitor;
>+  mozilla::Mutex mLock;

>+++ b/mailnews/imap/src/nsImapUrl.cpp
> #include "nsAlgorithm.h"
> #include "nsServiceManagerUtils.h"
> 
>+using namespace mozilla;
>+
> static NS_DEFINE_CID(kCImapHostSessionListCID, NS_IIMAPHOSTSESSIONLIST_CID);
> 
>-nsImapUrl::nsImapUrl()
>+nsImapUrl::nsImapUrl() : mLock("imapUrl")
Shouldn't this be mLock("nsImapUrl")? See comment at top for more.

	David :Bienvenu Assignee
	Comment 22 • 13 years ago

yeah, that diff is part of an other patch. Re the lock names, they're extremely cosmetic (only used in debug mode), but I can change them.

Re Neil's comments, this patch is definitely not done - in particular, TellThreadToDie (the UI-thread version) uses MutexAutoLock, but, unfortunately, so do some of the callers, which generates a PR_Assert. So I either need to switch back to PR_CEnter/ExitMonitor, or make sure all callers to TellThreadToDie grab the mutex lock, and remove it from TellThreadToDie.

	Bengt-Erik Soderstrom
	Comment 23 • 13 years ago

I wonder if, perhaps, all this has to do with a compile error I get since a couple of days ago in ldap/xpcom/src/nsLDAPService.cpp:50: nsAutoLock.h not found.

	Ian Neal
	Comment 24 • 13 years ago

Comment on attachment 523472 [details] [diff] [review]
Remove autolock from ldap code patch v1.0

Moving patch/review to bug 647003

	Ian Neal
	Comment 25 • 13 years ago

(In reply to comment #23)
> I wonder if, perhaps, all this has to do with a compile error I get since a
> couple of days ago in ldap/xpcom/src/nsLDAPService.cpp:50: nsAutoLock.h not
> found.

That is bug 647003

	David :Bienvenu Assignee
	Comment 26 • 13 years ago

Haven't run into any assertions or hangs with this version...

	neil@parkwaycc.co.uk
	Comment 27 • 13 years ago

Comment on attachment 523919 [details] [diff] [review]
fix to quiet assertions

>diff --git a/mailnews/imap/src/nsImapMailFolder.cpp b/mailnews/imap/src/nsImapMailFolder.cpp
Bits of other patches?

>-      PR_EnterMonitor(m_urlReadyToRunMonitor);
>+      m_urlReadyToRunMonitor.Enter();
>       m_lastActiveTime = PR_Now();
>       m_nextUrlReadyToRun = PR_TRUE;
>-      PR_Notify(m_urlReadyToRunMonitor);
>-      PR_ExitMonitor(m_urlReadyToRunMonitor);
>+      m_urlReadyToRunMonitor.Notify();
>+      m_urlReadyToRunMonitor.Exit();
IIRC this can be written as
MonitorAutoEnter mon(m_urlReadyToRunMonitor);
m_lastActiveTime = PR_Now();
m_nextUrlReadyToRun = PR_TRUE;
mon.Notify();

>-  PR_EnterMonitor(m_threadDeathMonitor);
>+  m_threadDeathMonitor.Enter();
>   m_safeToCloseConnection = aIsSafeToClose;
>   m_threadShouldDie = PR_TRUE;
>-  PR_ExitMonitor(m_threadDeathMonitor);
Care with scoping, I guess:
{
  MonitorAutoEnter mon(m_threadDeathMonitor);
  m_safeToCloseConnection = aIsSafeToClose;
  m_threadShouldDie = PR_TRUE;
}

	David :Bienvenu Assignee
	Comment 29 • 13 years ago

I used MonitorAutoEnter where it seemed appropriate, and addressed the other comments.

	neil@parkwaycc.co.uk
	Comment 30 • 13 years ago

Comment on attachment 524241 [details] [diff] [review]
3.1 fix addressing comments.

While I wait for my compile to complete...

>-nsImapIncomingServer::nsImapIncomingServer()
>+nsImapIncomingServer::nsImapIncomingServer() : mLock("nsImapIncomingServer.mLock")
As this is > 80 chars, could have this on two lines i.e.
nsImapIncomingServer::nsImapIncomingServer()
  : mLock("nsImapIncomingServer.mLock")

> nsImapProtocol::nsImapProtocol() : nsMsgProtocol(nsnull),
>-    m_parser(*this)
>+    mLock("nsImapProtocol.mLock"), m_parser(*this),
>+    m_dataAvailableMonitor("imapDataAvailable"),
>+    m_urlReadyToRunMonitor("imapUrlReadyToRun"),
>+    m_pseudoInterruptMonitor("imapPseudoInterrupt"),
>+    m_dataMemberMonitor("imapDataMember"),
>+    m_threadDeathMonitor("imapThreadDeath"),
>+    m_waitForBodyIdsMonitor("imapWaitForBodyIds"),
>+    m_fetchMsgListMonitor("imapFetchMsgList"),
>+    m_fetchBodyListMonitor("imapFetchBodyList"),
>+    m_passwordReadyMonitor("imapPasswordReady")
m_parser is later on in the .h file so needs to be listed last.

>-      // grab a monitor so m_mockChannel doesn't get cleared out
>+      // grab a lock so m_mockChannel doesn't get cleared out
>       // from under us.
>-      nsAutoCMonitor mon(this);
>+       MutexAutoLock mon(mLock);
Nit: extra space

>-  {
>-    nsAutoCMonitor mon(this);
>-
>-    m_urlInProgress = PR_TRUE;  // let's say it's busy so no one tries to use
>+  PR_CEnterMonitor(this);
>+
>+  m_urlInProgress = PR_TRUE;  // let's say it's busy so no one tries to use
Is it possible that someone will add RAII for PR_CEnterMonitor? It might be worth leaving the block scope in just in case ;-)

>+  {
>+    MonitorAutoEnter mon(m_threadDeathMonitor);
>+    m_threadShouldDie = PR_TRUE;
>+  }
>+  m_dataAvailableMonitor.Enter();
>+  m_dataAvailableMonitor.Notify();
>+  m_dataAvailableMonitor.Exit();
Nit: forgot to MonitorAutoEnter this one.

>+
>+  MonitorAutoEnter urlReadyMon(m_urlReadyToRunMonitor);
>+  urlReadyMon.NotifyAll();
In fact, MonitorAutoEnter(m_urlReadyToRunMonitor).NotifyAll() compiles. Not looked at the output to see whether it does the right thing though.

>+  MonitorAutoEnter pseudoInterruptMon(m_pseudoInterruptMonitor);
>+  return m_pseudoInterrupted;
I like this change!

> void nsImapProtocol::SetMailboxDiscoveryStatus(EMailboxDiscoverStatus status)
> {
>-    PR_EnterMonitor(GetDataMemberMonitor());
>+  m_dataMemberMonitor.Enter();
>   m_discoveryStatus = status;
>-    PR_ExitMonitor(GetDataMemberMonitor());
>+  m_dataMemberMonitor.Exit();
> }
> 
> EMailboxDiscoverStatus nsImapProtocol::GetMailboxDiscoveryStatus( )
> {
>   EMailboxDiscoverStatus returnStatus;
>-    PR_EnterMonitor(GetDataMemberMonitor());
>+  m_dataMemberMonitor.Enter();
>   returnStatus = m_discoveryStatus;
>-    PR_ExitMonitor(GetDataMemberMonitor());
>+  m_dataMemberMonitor.Exit();
> 
>     return returnStatus;
> }
But you overlooked the opportunity here.

>-      PR_EnterMonitor(m_passwordReadyMonitor);
>+      m_passwordReadyMonitor.Enter();
>       while (m_password.IsEmpty() && !NS_FAILED(m_passwordStatus) &&
>              m_passwordStatus != NS_MSG_PASSWORD_PROMPT_CANCELLED &&
>              !DeathSignalReceived())
>-        PR_Wait(m_passwordReadyMonitor, sleepTime);
>+        m_passwordReadyMonitor.Wait(sleepTime);
>       rv = m_passwordStatus;
>-      PR_ExitMonitor(m_passwordReadyMonitor);
>+      m_passwordReadyMonitor.Exit();
>       password = m_password;
Missing MonitorAutoEnter love here.

>+  m_passwordReadyMonitor.Enter();
>+  m_passwordReadyMonitor.Notify();
>+  m_passwordReadyMonitor.Exit();
And here.

>+  m_passwordReadyMonitor.Enter();
>+  m_passwordReadyMonitor.Notify();
>+  m_passwordReadyMonitor.Exit();
Ditto.

	neil@parkwaycc.co.uk
	Comment 32 • 13 years ago

Comment on attachment 524273 [details] [diff] [review]
fix addressing comments - checked in, w/ comments addressed
[Checked in: Comment 35]

>-    m_parser(*this)
...
>+    m_passwordReadyMonitor("imapPasswordReady"),
>+     m_parser(*this)
You got rid of one extraneous space but another crept in ;-)
(Particularly noticeable as this line should effectively remain unchanged.)

>+  MonitorAutoEnter passwordMon(m_passwordReadyMonitor);
>+  passwordMon.Notify();
...
>+  MonitorAutoEnter mon(m_passwordReadyMonitor);
>+  mon.Notify();
[I guess you got tired of typing passwordMon ;-) ]

	David :Bienvenu Assignee
	Comment 33 • 13 years ago

Comment on attachment 523631 [details] [diff] [review]
cumulative patch that compiles on windows
[Checked in: Comment 34]

I think Neil has basically reviewed this, but not officially...

	David :Bienvenu Assignee
	Comment 34 • 13 years ago

Ian's patch landed - http://hg.mozilla.org/comm-central/rev/283cf841daac - marking fixed.

	Serge Gautherie (:sgautherie)
	Comment 35 • 13 years ago

Comment on attachment 524273 [details] [diff] [review]
fix addressing comments - checked in, w/ comments addressed
[Checked in: Comment 35]

http://hg.mozilla.org/comm-central/rev/85be50c0b00f

	neil@parkwaycc.co.uk
	Comment 36 • 13 years ago

Comment on attachment 523631 [details] [diff] [review]
cumulative patch that compiles on windows
[Checked in: Comment 34]

>-    static PRLock *mMutex ;
>+    static mozilla::Mutex mMutex ;
Apparently we're not supposed to have a static Mutex ...