754865 - spin opening imap folder, new headers not downloaded

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[David :Bienvenu]

Attachment: possible fix

I've gotten into a state where my imap drafts folder won't download new headers. I tracked it down to having bogus offline operations in the db, which led to errors trying to playback offline operations, which prevented us from trying to fetch new headers. I don't know what led to the bogus offline operations, but I was able to fix the offline playback code so that the bogus operations were removed and the folder was updated. W/o this patch, the user would need to repair the folder in order to get new headers downloaded.

I'd like to avoid duplicating the check for a bogus op type, which probably means wrapping it in a helper function. I'll try to do that real quick.

Comment 1

[David :Bienvenu]

Attachment: proposed fix for review

Comment 2

[[e-mail]]

Comment on attachment 623681 [details] [diff] [review]
possible fix

>@@ -484,16 +484,21 @@ nsImapOfflineSync::ProcessAppendMsgOpera
>+  else
>+  {
>+    m_currentDB->RemoveOfflineOp(currentOp);
>+    ProcessNextOperation();
>+  }
Is this relevant to the bug?

>+          // remove operations with no type.
>+          if (!opType)
>+            m_currentDB->RemoveOfflineOp(currentOp);
>         // loop until we find the next db record that matches the current playback operation
>         while (currentOp && !(opType & mCurrentPlaybackOpType))
>         {
>           currentOp = nsnull;
>           ++m_KeyIndex;
>           if (m_KeyIndex < m_CurrentKeys.Length())
>             m_currentDB->GetOfflineOpForKey(m_CurrentKeys[m_KeyIndex],
>                                             false, getter_AddRefs(currentOp));
>           if (currentOp)
>+          {
>             currentOp->GetOperation(&opType);
>+            // remove operations with no type.
>+            if (!opType)
>+              m_currentDB->RemoveOfflineOp(currentOp);
>+          }
>         }
You can get rid of the duplication like this:
// loop until we find the next db record that matches the current playback operation
while (currentOp && !(opType & mCurrentPlaybackOpType))
{
  // remove operations with no type.
  if (!opType)
    m_currentDB->RemoveOfflineOp(currentOp);
  currentOp = nsnull;
  ++m_KeyIndex;
  if (m_KeyIndex < m_CurrentKeys.Length())
    m_currentDB->GetOfflineOpForKey(m_CurrentKeys[m_KeyIndex],
                                    false, getter_AddRefs(currentOp));
  if (currentOp)
  {
    currentOp->GetOperation(&opType);
    // remove operations with no type.
    if (!opType)
      m_currentDB->RemoveOfflineOp(currentOp);
  }
}

Comment 3

[David :Bienvenu]

(In reply to [e-mail] from comment #2)
> Comment on attachment 623681 [details] [diff] [review]
> possible fix
> 
> >@@ -484,16 +484,21 @@ nsImapOfflineSync::ProcessAppendMsgOpera
> >+  else
> >+  {
> >+    m_currentDB->RemoveOfflineOp(currentOp);
> >+    ProcessNextOperation();
> >+  }
> Is this relevant to the bug?

yes, in the sense that it also contributed to the spin opening the folder. The methods that process operations need to call ProcessNextOperation if they don't run a url, or we'll spin, and the other methods (e.g., ProcessFlagOperations) do so.

> You can get rid of the duplication like this:
> // loop until we find the next db record that matches the current playback
> operation
> while (currentOp && !(opType & mCurrentPlaybackOpType))
> {
>   // remove operations with no type.
>   if (!opType)
>     m_currentDB->RemoveOfflineOp(currentOp);
>   currentOp = nsnull;
>   ++m_KeyIndex;
>   if (m_KeyIndex < m_CurrentKeys.Length())
>     m_currentDB->GetOfflineOpForKey(m_CurrentKeys[m_KeyIndex],
>                                     false, getter_AddRefs(currentOp));
>   if (currentOp)
>   {
>     currentOp->GetOperation(&opType);
>     // remove operations with no type.
>     if (!opType)
>       m_currentDB->RemoveOfflineOp(currentOp);
>   }
> }

that duplicates the call to RemoveOfflineOp at the top and bottom of the loop, unless I'm missing something.

Comment 4

[[e-mail]]

(In reply to David Bienvenu from comment #3)
> (In reply to comment #2)
> > You can get rid of the duplication like this:
> > [snip failed attempt]
> that duplicates the call to RemoveOfflineOp at the top and bottom of the
> loop, unless I'm missing something.
Yeah, I copied and pasted instead of cut and pasted. Let me try again:
// loop until we find the next db record that matches the current playback operation
while (currentOp && !(opType & mCurrentPlaybackOpType))
{
  // may as well remove operations with no type while we're here
  if (!opType)
    m_currentDB->RemoveOfflineOp(currentOp);
  currentOp = nsnull;
  ++m_KeyIndex;
  if (m_KeyIndex < m_CurrentKeys.Length())
    m_currentDB->GetOfflineOpForKey(m_CurrentKeys[m_KeyIndex],
                                    false, getter_AddRefs(currentOp));
  if (currentOp)
    currentOp->GetOperation(&opType);
}

Comment 5

[David :Bienvenu]

Attachment: fix addressing comments

yes, that's better, and should work.

Comment 6

[David :Bienvenu]

fixed on trunk - http://hg.mozilla.org/comm-central/rev/3b21da7e9eff

don't know if anyone else ever saw this, or how to recreate it (the workaround would have been to repair the folder) but maybe this will help others...