547979 - when deleting messages very quickly, cannot delete any message anymore

Status: NEW

(Thunderbird :: General, defect)

Description

[arno renevier]

Hi,
we're having a custom extension to display list of messages, so we don't know if that bug happens in thunderbird:
we delete messages with nsIMsgFolder->DeleteMessages

when user deletes many messages in a row, sometimes, rv = mDestination->EndCopy(copySucceeded); does not succeed. 
We don't know exactly why, but 

mCopyState->m_destDB->CopyHdrFromExistingHdr(mCopyState->m_curDstKey,
          mCopyState->m_message, PR_TRUE,
          getter_AddRefs(newHdr)); fails because m_mdbAllMsgHeadersTable and m_mdbStore are null in nsMsgDatabase::CreateNewHdr

anyway, as mDestination->EndCopy fails, msgCopyService is not notified the request has ended. Therefore, all future attempts to remove any message will fail.

Comment 1

[arno renevier]

Attachment: patch v1

here is a patch to still notify msgCopyService move has ended (and failed)

Comment 2

[Ludovic Hirlimann [:Usul]]

Comment on attachment 428449 [details] [diff] [review]
patch v1

(In reply to comment #1)
> Created an attachment (id=428449) [details]
> patch v1
> 
> here is a patch to still notify msgCopyService move has ended (and failed)

Thank you very much for the patch. In order to get somewhere with it you need to ask for a specific review not just  ? see https://developer.mozilla.org/en/comm-central#Requirements. I'll set the review request for you today.

Your patch is missing some unit test, and we require that since jan 4 2010 (see https://developer.mozilla.org/en/Thunderbird/Thunderbird_Automated_Testing on how to write these tests.). If you need help with the test part please say so and we'll provide some help.

Comment 3

[David :Bienvenu]

Comment on attachment 428449 [details] [diff] [review]
patch v1

thx for the patch - I'll try it out. The braces style is not consistent with the rest of the file, but I'll clean that up when I apply it.

Comment 4

[David :Bienvenu]

Attachment: cleaned up patch

arno, can you try this patch, and see if it works? I cleaned up the whole function, style-wise, and removed the imap checks since nsImapMailFolder::EndMove() doesn't do anything anymore.

Comment 5

[arno renevier]

your patch works fine.

Comment 6

[David :Bienvenu]

ah, thx, I was not on the cc list. I'll land the patch in a bit...

Comment 7

[[e-mail]]

Comment on attachment 428976 [details] [diff] [review]
cleaned up patch

> NS_IMETHODIMP nsCopyMessageStreamListener::EndCopy(nsISupports *url, nsresult aStatus)
> {
>+  nsresult rv;
>+  nsCOMPtr<nsIURI> uri = do_QueryInterface(url, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
> 
>+  PRBool copySucceeded = (aStatus == NS_BINDING_SUCCEEDED);
>+  rv = mDestination->EndCopy(copySucceeded);
>+  // If this is a move and we finished the copy, delete the old message.
>+  if (NS_SUCCEEDED(rv))
>+  {
>+    PRBool moveMessage = PR_FALSE;
> 
>+    nsCOMPtr<nsIMsgMailNewsUrl> mailURL(do_QueryInterface(uri));
>+    if (mailURL)
>+      rv = mailURL->IsUrlType(nsIMsgMailNewsUrl::eMove, &moveMessage);
> 
>+    if (NS_FAILED(rv))
>+      moveMessage = PR_FALSE;
> 
>+    // IMAP folders ignore the EndMove call, so don't need to check that this
>+    // is an imap folder.
>+    if (moveMessage)
>+      rv = mDestination->EndMove(copySucceeded);
>+  }
>+  else // again, IMAP ignores this.
>+     rv = mDestination->EndMove(PR_FALSE);
>+
>+  // Even if the above actions failed we probably still want to return NS_OK.
>+  // There should probably be some error dialog if either the copy or 
>+  // delete failed.
>+  return NS_OK;
> }
This doesn't look quite right. Perhaps something like this:
PRBool copySucceeded = (aStatus == NS_BINDING_SUCCEEDED);
if (NS_FAILED(mDestination->EndCopy(copySucceeded)))
  copySucceeded = PR_FALSE;

PRBool moveMessage;
nsCOMPtr<nsIMsgMailNewsUrl> mailURL(do_QueryInterface(url));
if (mailURL &&
    NS_SUCCEEDED(mailURL->IsUrlType(nsIMsgMailNewsUrl::eMove, &moveMessage)) &&
    moveMessage)
      mDestination->EndMove(copySucceeded);

Comment 8

[David :Bienvenu]

Attachment: Neil's suggestion

yeah, that's nicer - this is what I'll land, then.

Comment 9

[[e-mail]]

Comment on attachment 431479 [details] [diff] [review]
Neil's suggestion

>+  nsCOMPtr<nsIURI> uri = do_QueryInterface(url, &rv);
Do you still need this? All you do with it is to QI it again.