Closed Bug 466438 Opened 12 years ago Closed 12 years ago

gloda is trying to index news messages even though it really should not (because it cannot)

Categories

(MailNews Core :: Database, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b1

People

(Reporter: asuth, Assigned: asuth)

References

Details

(Whiteboard: [has patch][can land])

Attachments

(1 file, 1 obsolete file)

Without the fix for bug 447842 that makes news streaming work for jsmimeemitter, gloda cannot successfully index news messages.  Also, we decided that gloda should not index news messages for now.  Yet gloda takes it upon itself to try and index some peoples' news messages.

This patch attempts to address this problem by consolidating our checks about newsgroups, calling them more thoroughly (some might suggest even a paranoid fashion) so nothing can slip through the cracks.

One non-obvious change to this end was to modify _indexerEnterFolder to explicitly call _indexerLeaveFolder rather than having it strive for an identical net effect.  It looked like a stale message database value from that path was causing us to sometimes end up using an iterator from a database different from the one we wanted.  This change is also desired for full and proper msf cleanup during event-driven indexing (whose core changes on bug 465122).

Requesting bienvenu review since my flag usage may be ill-conceived; also, this fix completes the fix on 465122 as noted.
Flags: blocking-thunderbird3?
Attachment #349724 - Flags: review?(bienvenu)
Whiteboard: [has patch][needs review bienvenu]
Comment on attachment 349724 [details] [diff] [review]
v1 stop gloda from indexing newsgroups, trying to go into virtual folders, avoid database confusion

this could be just

return (folderFlags & Ci.nsMsgFolderFlags.Mail && !(folderFlags & Ci.nsMsgFolderFlags.Virtual))

return (folderFlags

+    if (!(folderFlags & Ci.nsMsgFolderFlags.Mail) ||
+        // stay out of virtual folders!
+        (folderFlags & Ci.nsMsgFolderFlags.Virtual))
+      return false;
+    return true;

Should this code be skipping news and virtual folders as well?

+      rootFolder.ListDescendents(allFolders);
+      let numFolders = allFolders.Count();
+      let folderJobs = [];
+      for (let folderIndex = 0; folderIndex < numFolders; folderIndex++) {
+        let folder = allFolders.GetElementAt(folderIndex).QueryInterface(
+                                                            Ci.nsIMsgFolder);
+        folderJobs.push(
+          new IndexingJob("folder", 1, GlodaDatastore._mapFolder(folder).id));
+      }
Whiteboard: [has patch][needs review bienvenu] → [has patch][needs review comments addressed]
Also, is that folderJobs code only hooked up to the expMess UI, in which case we could say the user knows what they're doing? Though we might want to still skip virtual folders there.
(In reply to comment #2)
> Also, is that folderJobs code only hooked up to the expMess UI, in which case
> we could say the user knows what they're doing? Though we might want to still
> skip virtual folders there.

Yes, that code is currently only activated by expmess's "Go Index".  It won't actually try and index those folders, because the actual folder indexing job itself uses shouldIndex().  However, it only uses shouldIndex after entering the folder, so we get the cost hit of opening the msf (and then closing it again).  I was torn about this, but was both in a hurry and happy to have the debug output that was a side effect of having enter folder happen.

I'll attach a patch with both issues addressed.
(Implied) requested changes made.  I also moved this patch up my patch queue just above the msf closure fix so that we shouldn't have any conflict surprises related to gloda patch fever.
Attachment #349724 - Attachment is obsolete: true
Attachment #349809 - Flags: review?(bienvenu)
Attachment #349724 - Flags: review?(bienvenu)
Attachment #349809 - Flags: review?(bienvenu) → review+
Comment on attachment 349809 [details] [diff] [review]
v2 stop gloda from indexing newsgroups, trying to go into virtual folders, avoid database confusion

re the test case and the folder flag not getting set, that's very weird. AddSubfolder sets the flag...
(In reply to comment #5)
> re the test case and the folder flag not getting set, that's very weird.
> AddSubfolder sets the flag...

I think something bad is happening somewhere later on.  Immediately after executing:
 let subFolder = rootFolder.addSubfolder(ims.mboxName);
subFolder has the flag.  But by the time we get around to the updateFolderAndNotify's callback function, flags has been zeroed.

I briefly looked into it, and saw that nsLocalMailFolder::CreateSubfolder says some stuff about GetFlags/SetFlags failing due to lacking a database, but it wasn't clear what that actually meant or if that would have an impact in this different case.  After beta 1, I can try and use chronicle or manually figure out where we're going wrong.

Thanks for the review.
Keywords: checkin-needed
Whiteboard: [has patch][needs review comments addressed] → [has patch][can land]
Checked in: http://hg.mozilla.org/comm-central/rev/a72a70ee8bff
Flags: blocking-thunderbird3?
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 3.0b1
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.