619909 - Gloda fromJSON de-persistence needs to filter out undefined returned values; deleted tags on removed messages end up as undefined, break faceting logic.

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

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

As reported on https://bugzilla.mozilla.org/show_bug.cgi?id=609941#c46, errors like this can happen:

Error: a.tag is undefined
Source File:
file:///Applications/Lanikai.app/Contents/MacOS/modules/gloda/noun_tag.js
Line: 91


This is a result of noun_tag.js's TagNoun.fromJSON method returning undefined for deleted tags.  For some reason I was under the impression I already had logic that performed this filtering, but I clearly do not.

This is easily unit testable and I am going to request permission to land on 3.1 as well.

Workarounds:

Option 1) After deleting a tag, cause gloda to reindex all of the messages that had that tag.  This is hard in most cases other than just deleting global-messages-db.sqlite.

Option 2) Re-create the tag.  Because tag keys are stable even in the face of renames, the tag must be named the same as it was originally named.

Comment 1

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

Attachment: v1 filter out undefined return values from fromJSON, use hasOwnProperty with NounTag

So, it turns out I was sorta wrong about what the problem was and this is related to bug 609941.  In my reduced patch for bug 609941 I failed to update NounTag to use hasOwnProperty.  

In that bug, when a tag with the name "watch" was added, some messages were indexed with it.  When shown in the faceted search UI things would break because of the failure to use hasOwnProperty there, now fixed.

The problem is that once that tag is deleted, if those messages do not get (gloda) reindexed, there is still an entry on the gloda message that claims the "watch" tag still exists.  NounTag was still using "in" checking and so even though there was no tag with "watch", there is still Object.prototype.watch which is what will actually get returned.  And so the comparator gets handed the native function instance which obviously does not have any of the properties we would expect an nsIMsgTag to have.

The more specific thing I thought was happening was not actually capable of breaking the faceting logic because the faceting logic was actually guarding against the "undefined" return value already.  (I was not imagining things, just not remembering them in sufficient detail).

In any event, I think this logic makes good sense and will likely save protz and Thunderbird Conversations from some headaches.  Since I am providing a unit test for both of the potential problems, I am going to call this a reasonable thing to do and land on the 3.1.x branch because of my unit tests.

It's also a reasonable 3.1.x landing because faceted search *will* break when it tries to display any of the messages that have the undefined tag; glodaFacetBindings.xml does not protect against undefined values.  It's just less likely for that to happen since it needs to show up in the top 10 and also will be somewhat less fatal.

I am directing the review to bienvenu rather than sid0 because Sid already signed off on hasOwnProperty as a viable solution and this ends up being a gloda-ish thing that interacts with tagging which is a bienvenu bailiwick thing.

Comment 2

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

Pushed to trunk:
http://hg.mozilla.org/comm-central/rev/3a67b40d56f8

Comment 3

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

Pushed to comm-1.9.2:
http://hg.mozilla.org/releases/comm-1.9.2/rev/0888a4f1d37e