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

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v1 stop gloda from indexing newsgroups, trying to go into virtual folders, avoid database confusion

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.

Comment 1

[David :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));
+      }

Comment 2

[David :Bienvenu]

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.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

(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.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v2 stop gloda from indexing newsgroups, trying to go into virtual folders, avoid database confusion

(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.

Comment 5

[David :Bienvenu]

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...

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

(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.

Comment 7

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/a72a70ee8bff