478172 - gloda indexer should guard against empty references

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

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

Attachment: v1 ensure references prove true

I'm not entirely sure why this is happening, as I thought I straightened out the references parsing logic at the root of things, but the indexer should guard against betrayal and ensure that the reference is non-false (so not null or an empty string).

I'm losing the list comprehension because I think it would look funny or include a redundant XPCOM call to accomplish the required logic.

Comment 1

[David :Bienvenu]

Comment on attachment 361948 [details] [diff] [review]
v1 ensure references prove true

I don't know why this would happen - is numReferences wrong, or is one of the middle references null?

Comment 2

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

Either numReferences is wrong or it counts the case where the last value is empty or null.  (The list of references gloda was building when converted to a string showed ",," for the reference in question, which would correspond to the 'highest' reference.  (The reason there is a trailing comma is that gloda also pushes on the message-id of the message it is indexing...)

What is confusing is that this only seemed to happen on a very few messages for me... and all of the examples I looked at were actually bugzilla messages...

Comment 3

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

Looks like in bug 466796 I got confused about the caching of m_numReferences (or the call sequence that might allow it to be made a moot issue).  It does get cached, and if your call sequence is GetNumReferences before any calls to GetStringReferences, you are going to get the cached value.  As such, msf files dating from before bug 466796 was fixed could be subject to this problem.

Things are made somewhat more confusing by GetStringReference returning NS_OK even when the reference number requested is not legit.

I feel like the best solution might just be to always parse to initialize the num references variable.  Such a patch could also abstain from creating the numRefs column (or setting it).

Comment 4

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

Attachment: v1 always parse references

dang, I didn't realize that you approved my kludge patch.  In any event, this patch should fix things for gloda and traditional threading.

This does what I proposed in the previous comment, namely removing the num references column entirely, and always parsing the header when the number of references is required.  Even though the correctness issue should only happen for "old" msf files, I think this is probably a win overall.  If you are accessing the number of references, you presumably are going to want to see them, so you would end up parsing anyways.  If you have no references, we won't have a references column entry, so we'll quickly be able to zero out m_numReferences.  And we avoid storing a column, so there's disk and memory savings.

I did make one semantic change, which is to return an error if you ask for a reference with an illegal index rather than silently return empty.

I should be up front that I honestly have no idea why I was experiencing this problem on some bugmail mails in the first place, but this solution is equivalent to the gloda work-around above, which definitely did resolve the problem.  (I could investigate it, but it doesn't seen worth it, especially if the actual problem was just that the references parsing code was newfoundly buggy when I received those messages but was subsequently fixed...)

Comment 5

[David :Bienvenu]

moving to b3, but I'll give this a shot now.

Comment 6

[David :Bienvenu]

Comment on attachment 363597 [details] [diff] [review]
v1 always parse references

this seems fine going forward. The one unintended consequence is that if you switch back and forth between builds w/ this patch, e.g., TB 3.0 b3, and builds w/o it (e.g., TB 2.0), you will get somewhat less accurate threading, but since those builds mostly do subject threading, it won't be a complete disaster.

Comment 7

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

pushed: http://hg.mozilla.org/comm-central/rev/ddaacd907768