Last Comment Bug 754865 - spin opening imap folder, new headers not downloaded
: spin opening imap folder, new headers not downloaded
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Networking: IMAP (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: Thunderbird 15.0
Assigned To: David :Bienvenu
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-14 08:34 PDT by David :Bienvenu
Modified: 2012-05-24 17:01 PDT (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
possible fix (2.11 KB, patch)
2012-05-14 08:34 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
proposed fix for review (4.85 KB, patch)
2012-05-14 09:49 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
fix addressing comments (874 bytes, text/plain)
2012-05-24 14:58 PDT, David :Bienvenu
neil: review+
Details

Description David :Bienvenu 2012-05-14 08:34:42 PDT
Created attachment 623681 [details] [diff] [review]
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 2012-05-14 09:49:29 PDT
Created attachment 623708 [details] [diff] [review]
proposed fix for review
Comment 2 neil@parkwaycc.co.uk 2012-05-15 05:26:34 PDT
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 2012-05-15 07:11:36 PDT
(In reply to neil@parkwaycc.co.uk 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 neil@parkwaycc.co.uk 2012-05-16 05:24:16 PDT
(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 2012-05-24 14:58:19 PDT
Created attachment 626981 [details]
fix addressing comments

yes, that's better, and should work.
Comment 6 David :Bienvenu 2012-05-24 17:01:18 PDT
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...

Note You need to log in before you can comment on or make changes to this bug.