User:Andrew Sutherland/MailNews/GlobalDatabase/ReviewBelievedResolved

Believed Addressed

asuth believes these points are addressed. Please sign-off on them or move them to agreed addressed or invent a new notation or something. Next action is the person whose heading it is under.

bienvenu

indexer magic constant

(bienvenu c15 q8) might add a comment referring to the NS_ERROR define you're catching here:

     catch ( e if e.result == 0xc1f30001) {
       // this means that we need to pend on the update.
       this._log.debug("Pending on folder load...");

(asuth 9/18) This was addressed with changeset 048686317d14, see the file diff. Note that since use of updateFolder is going away soon, this will shortly be mooted.

(asuth 9/22)So, changeset e997476f295b leaves this using Cr.blah instead, although the change to Components.results was made earlier.

indexer obsolete comment

(bienvenu c15 q6) Is this comment obsolete? You're not using setTimeout, or this._messenger, in indexer.js

   // we need this for setTimeout... what to do about this?
   this._messenger = Cc["@mozilla.org/messenger;1"].
                       createInstance(Ci.nsIMessenger);
   this._timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);

(asuth c17 a6) Yes, it's obsolete and went away sometime yesterday in a clean-up/update pass. _messenger (which oddly inherited the comment) also went away... I think that was detritus from the mime-body-fetching before it entirely migrated to mimemsg.js.

(asuth 9/18) This happened in changeset a5e649db05eb, only that file was affected.

expmess installation/usage

(bienvenu c15 q2) Should I be installing the expmess xpi to see the status of indexing? Is there a newer version of it than 0.0-m1?

(asuth c17 a2) Yes, expmess is the only way to see what is happening. The XPI is very old and probably will break things. I would install the extension directly from hg. (So, check out https://hg.mozilla.org/users/bugmail_asutherland.org/expmess/, then create a file "expmess@mozillamessaging.com" in your profile's extensions directory that provides the path to the checked-out directory.)

expmess LDAP frowny faces

(bienvenu c28 attached patch) I was never seeing anything from expmess when I clicked on a message, other than the frowny face :-( This fixes it for me. I suspect we were getting an ldap directory, which throws an exception when this is called. Andrew, let me know if you want me to just land this in your repo, or if you want to fix it another way...

(asuth c29 approving patch) LDAP, eh? Feel free to land it in the gloda repo.

(Standard8 c30) Yes, not all address book types can support this function at the moment.

(bienvenu c31 noting commit 3ac64fe0567b) finally coerced hg into letting me push

nsMsgSearchDBView::GetType fall-through

(bienvenu c33) patch provided) make seach view return a view type we were generating an error before because the base class returns an error. Gloda & friends wants this not to error out.

(asuth c35 r+ patch Patch looks good; applies and makes the problem go away. Danke.

(Neil c36 sr+) Basically this function is the inverse of the switch in CreateBareDBView, right?

(bienvenu c37 commit marking) yes, it's the same kind of thing...

code formatting: equal spaces

(bienvenu c15 q8) we generally put spaces around the '=', at least in c++

collection.js

   let iWrite=0;
   for (let iRead=0; iRead < items.length; iRead++) {

datamodel.js:

           let values = this[attrDef.boundName];
           for (let iValue=0; iValue < values.length; iValue++)

(asuth) Fixed in changeset 0f5e5f6ba559. 1561f5ddffc9 also has a related change to make "for(" become "for (".

utils variable naming

(bienvenu c26 q1)

 md5HashString: function gloda_utils_md5hash(aString) {
   let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
                   createInstance(Ci.nsIScriptableUnicodeConverter);
   let trash = {};
   converter.charset = "UTF-8";
   let emailArr = converter.convertToByteArray(aString, trash);

   let hasher = Cc['@mozilla.org/security/hash;1'].

"emailArr" seems an odd var name here, if this is a generic utility function

-)

(asuth) changed to the more generic 'data'.

indexing preference exposure

(bienvenu c15 q7)do we expect this pref to live permanently? If so, it's worth putting in mailnews.js so that it shows up in about:config and has a default.

   let branch = prefService.getBranch("mailnews.database.global.indexer");

(asuth c17 a7) I'm not sure. I think Thunderbird will always want the event-driven indexing active, but it sounds like SeaMonkey may not want it on by default? I created the pref as a better alternative to having chrome reach in to turn that on, but without any deep rationale.

(asuth current status (9/18)) Note that this pref fails to stop some listeners from being registered, which is bad. I need to expose the pref in mailnews.js...

(asuth) The preference was exposed via mailnews.js and even has UI now.

MIME Parsing Assertion

(bienvenu c15 q4) I see this assertion when picking the "index everything" menu item:

mime.dll!MimeObject_parse_eof(MimeObject * obj=0x064878a8, int abort_p=0x00000000)  Line 302 + 0x25 bytes    C++
mime.dll!MimeContainer_parse_eof(MimeObject * object=0x064878a8, int abort_p=0x00000000)  Line 129 + 0xe bytes    C++

(asuth c17 a4)I'm not familiar with that one. What's the actual assertion text? What type of message was it trying to process?

(bienvenu c20 p6) I'll save it next time I see it - "already parse eof/eom" was one of them, iirc.

(bienvenu c24) Here's the assertion I see - "obj already parsed" is the string, I think.

mime.dll!MimeObject_parse_eof Line 302 + 0x25 bytes
mime.dll!MimeContainer_parse_eof Line 129 + 0xe bytes
mime.dll!MimeContainer_finalize Line 98 + 0x10 bytes
mime.dll!MimeUntypedText_finalize Line 117 + 0xa bytes
mime.dll!mime_free Line 323 + 0xe bytes
mime.dll!MimeContainer_finalize Line 109 + 0x9 bytes
mime.dll!MimeMessage_finalize Line 126 + 0xa bytes
mime.dll!mime_free Line 323 + 0xe bytes
mime.dll!mime_display_stream_complete Line 989 + 0x9 bytes
mime.dll!nsStreamConverter::OnStopRequest Line 1049 + 0xf bytes
msgbsutl.dll!nsMsgProtocol::OnStopRequest Line 389 + 0x5a bytes

bug 475496 logged.

SMIME Assertions

(bienvenu c15 q5) as well as this one, quite frequently:

mime.dll!nsSMimeVerificationListener::Release()  Line 275 + 0x6c bytes   
pipnss.dll!nsCOMPtr<nsISMimeVerificationListener>::~nsCOMPtr<nsISMimeVerificationListener>()
pipnss.dll!nsSMimeVerificationJob::`scalar deleting destructor'()  + 0xf
pipnss.dll!nsCertVerificationThread::Run()  Line 150 + 0x20 bytes    C++
pipnss.dll!nsPSMBackgroundThread::nsThreadRunner(void * arg=0x02aa0c80) Line 45

(asuth c17 a5) I don't think I've actually seen this assertion either, but I had noticed that the SMime stuff was getting involved in an undesirable fashion not too long ago. It apparently feeds events to the lock icon on the message pane, and may be involved in the deadlocks I had noticed a-ways back.

(asuth c18) I believe this is the assertion described by bug 379661. So, while the problem is not confined to gloda, I think I still need to investigate/deal with the SMIME interaction.

(bienvenu c19) yes, I probably just saw it because gloda streams all my messages through libmime, the weakest link :-)

(asuth status (9/18)) I need to look into what's going on with the SMIME stuff.

(asuth) bug 475499 logged to track the need to not have other MIME handlers fire on our (indexing) watch.

expmess tree row assertions

(bienvenu c15 q3) I'm seeing lots of assertions about the tree row count getting out of sync - those are a sign of not sending the right notifications to the tree control.

(asuth c17 a3) Yes. I think the hg version of expmess improves that, but I'm not sure it's entirely fixed.

(bienvenu c20 p5) I haven't seen the row count assertions since updating expmess.

(asuth current status (9/18)) This is not fixed. I don't know what exactly is up, but it's an exclusively expmess sort of issue. (I haven't actually looked at it for some time, could be a mistaken off-by-one.) I will fix this because it should be easy.

(asuth) expmess is dead. long be dead expmess.

gloda suffixtree dump statements

(bienvenu c26 q3) There are a lot of dump statements, e.g.,

dump("considering " + this._str.slice(stringStart, stringEnd) + " with pos " + 
    stringStart + ":" + stringEnd + " with state " +
    aState.start + ":" + aState.end +
    "(patFirst:" + patternFirst + " patLast: " + patternLast +
    " delta: " + aDelta + ")\n");
 if (patternFirst >= stringStart) {
   if (!(stringStart in aPresence)) {
dump("  adding! (patternFirst: " + patternFirst + ")\n");
     aPresence[stringStart] = true;
     aResults.push(this._offsetsToItems[mid*3+2]);
   }
 }
 else {
dump("  disregarding because pattern is not contained. (patternLast:" +
 patternLast + " aPatLength: " + aPatLength + " stringstart: " +
 stringStart + "\n"); 
 }

these should either be turned into logging, or removed, or turned into calls to a function which calls dump based on a global bool, defaulted to false.

(asuth) The only dump statements that exist now are in the dump() function which exists for helpful debugging support and an example routine.

conversation unread messages enhancement

(bienvenu c15 q9) Conversations might want have an attribute for the number of unread messages, though it's easily calculated, obviously.

(asuth c17 a8) What specific use-case are you imagining here? We can easily create a getter that performs this tally (by forcing the messages to be loaded and then tallying). If the goal is just to be able to query for conversations with unread messages, the message attribute schema lets us find conversations with at least one unread message (although the query mechanism does not yet expose that). But if we want to query conversations based on their number of un-read, or ordering them, then it definitely makes sense to add the attribute explicitly and take the complexity hit to maintain it. (We're already part-way there with the popularity for contacts and our desire to track first message date/last message date for the conversation, though.)

(bienvenu c20 p7) Nothing specific - as a user, I find the unread count in a thread to be a very interesting bit of information, often, but I'm not sure if that translates into the need to be able to query based on that. And it's a royal pain when that count gets wrong :-)

(asuth) bug 468286 tracks such a desire.

idle service use

(bienvenu c15 q1) I don't see the gloda code using the idle service to do background indexing on idle - am I missing that? Is that planned, or does it not seem like the right thing to do?

(asuth c17 a1) Correct, the idle service is not currently used. The initial implementation just used a timer interval to schedule things, and it appears to work reasonably well. Which is, no one who has tried gloda thus far has complained about a lack of responsiveness. In practice, the introduction of the body fetching has actually slowed things down even more.

My original plan was to use the idle service to control the parameters of the timer-based indexing. Those variables being: the number of milliseconds between re-scheduling, and the number of messages processed before we stop processing pending another timer interrupt (captured using a token bucket-style implementation). The idea was that when the machine was idle, we could throttle up our indexing.

I think this general idea is the right idea, because we *do* want to index when the user is around, otherwise we lose the ability to keep up with new messages / changed messages. At least, for new/changed messages. When we are just trying to get gloda up to speed, that part is not as important, but we still want to get up to speed as quickly as possible.

There's also some forthcoming interaction with Emre's automatic IMAP offline-fetching stuff that needs to happen. I haven't entirely thought it through, but it seems like, to a large extent, we could just play follow-the-leader on the offline fetching... but there are a large number of cases that need to be dealt with.

Your thoughts/input are greatly appreciated on this one.

(bienvenu c20 p2) Re using the idle service to know when to throttle up the indexing, I think that's a good plan. My brief experience with gloda and expmess is that indexing while I'm doing stuff is pretty painful, so much so that I would really throttle down the amount of stuff that's done when not idle. When new messages arrive, I would put them on a queue of messages to be indexed, and not index them right away. Similarly, if the user selects 100 messages and deletes them, we'd really like to avoid doing all the gloda housekeeping right away (it looks like you have infrastructure in place to do all this, so it's probably just a matter of being less aggressive while not idle). Also (I apologize for not fully groking the code :-) ), can/do you do the body indexing separate from the basic adding of the header to gloda? As you say, streaming the message through libmime is pretty cpu intensive.

I realize there are tradeoffs between having the gloda data up to date, and keeping the UI performant, and there's always the shutdown case. It might be simplest just to remember which folders have pending operations, and sync those on the next run, instead of building full-blown persistance of your job queues.

I'm on Windows, on a rather old box, but I really did feel the pain when new mail came in and they started getting indexed while I was reading and deleting them...

(dmose c22) Seems like chunking up the work into a finer granularity might be another way to address this.

(asuth current status (9/18)) Indexing is being re-done.

(asuth) Indexing was re-done and the idle service does come into the picture. However, there are some issues about not being adaptive, and bug 470329 tracks that.

make integration

(bienvenu c27 attached patch) I think you also need this patch to get gloda to build if you build from the top - I had to build from mailnews/db otherwise, to get gloda to build. I haven't finished checking that this will fix it, but I suspect it will.

(asuth current status (9/18)) This patch is good and should be folded into my tree patch.

(asuth) The patch was effectively folded in; gloda is in the tree now and knows how to build, etc.

updateFolder usage

(bienvenu c32) One other issue I've run into - gloda seems to update folders on startup, and I run into an assertion because we're in the middle of classifying new mail as junk or not junk, because the ui is getting new mail at the same time. I've hit this for my pop3 inbox, and the assertion is in nsMsgLocalMailFolder::SpamFilterClassifyMessages. This is a potentially bad assertion because if we get out of sync with our classification requests, it can break things like new mail notification and junk mail processing.

There's no particular reason for the indexer to update local folders, since unlike IMAP, it doesn't really do anything interesting as far as the indexer is concerned. I guess you're doing it to make sure the .msf is up to date...which is a bit of an edge case, but I can see why you'd want to do that. But consider the imap case, where update folder will issue a select on the imap server and download new messages; does the indexer want to do that? As you've pointed out before, you'll need to sync with Emre's auto update stuff as well...

You could actually just call GetDatabaseWithReparse for local mail folders. Or I could add a new method to nsIMsgFolder that does what you want for both kinds of folders. UpdateFolder is really meant more for opening folders in the UI, since it kinda assumes that's what's going on (hence the running of the junk mail controls). Or I could treat UpdateFolder with a null msg window as meaning it's not coming from the UI...

(asuth c34) Right, I initially did this for local folders because sometimes in my testing (because of me, generally), the .msf files did not yet exist. I wanted to use GetDatabaseWithReparse, but wanted to use nsIMsgFolder and avoid a downcast. It seemed like a nice freebie that the IMAP state should be up-to-date to boot. (After all, what's the point of a global index if you have to click on things to get the results to be accurate...)

Since Emre's code will be a given, I think I can rely on it to make sure the IMAP is sufficiently up-to-date. That just leaves avoiding dying when the msf doesn't exist yet. If that means an instanceof check and a call to GetDatabaseWithReparse, that works for me.

(asuth) I did what what I said I would. We use GetDatabaseWithReparse.

misc

• Will gloda land as an extension or in core? For gloda to not be an extension, the components need to be added to the packages - see http://mxr.mozilla.org/comm-central/search?string=nsLDAPPrefsService.js for an example

(We landed in core)

• I see a bunch of assertions in AsyncExecute::~AsyncDelete while indexing - "still pending events"

 	xpcom_core.dll!NS_DebugBreak_P(unsigned int aSeverity=0x00000001, const char * aStr=0x038e2e94, const char * aExpr=0x038e2eac, const char * aFile=0x038e2ec8, int aLine=0x000001ce)  Line 359 + 0xc bytes	C++
 	strgcmps.dll!AsyncExecute::~AsyncExecute()  Line 462 + 0x2b bytes	C++
 	strgcmps.dll!AsyncExecute::`scalar deleting destructor'()  + 0xf bytes	C++
 	strgcmps.dll!AsyncExecute::Release()  Line 523 + 0x90 bytes	C++
 	xpc3250.dll!XPCJSRuntime::GCCallback(JSContext * cx=0x049f2c60, JSGCStatus status=JSGC_END)  Line 818 + 0x14 bytes	C++
 	jsd3250.dll!jsds_GCCallbackProc(JSContext * cx=0x049f2c60, JSGCStatus status=JSGC_END)  Line 531 + 0xe bytes	C++
 	gklayout.dll!DOMGCCallback(JSContext * cx=0x049f2c60, JSGCStatus status=JSGC_END)  Line 3582 + 0x17 bytes	C++
 	xpc3250.dll!XPCCycleCollectGCCallback(JSContext * cx=0x049f2c60, JSGCStatus status=JSGC_END)  Line 459 + 0x17 bytes	C++
 	js3250.dll!js_GC(JSContext * cx=0x049f2c60, JSGCInvocationKind gckind=GC_NORMAL)  Line 3734 + 0x9 bytes	C++
 	js3250.dll!JS_GC(JSContext * cx=0x049f2c60)  Line 2477 + 0xb bytes	C++
 	xpc3250.dll!nsXPConnect::Collect()  Line 530 + 0xa bytes	C++
 	xpcom_core.dll!nsCycleCollector::Collect(unsigned int aTryCollections=0x00000001)  Line 2256 + 0x19 bytes	C++
 	xpcom_core.dll!nsCycleCollector_collect()  Line 2904 + 0x16 bytes	C++
 	gklayout.dll!nsJSContext::CC()  Line 3411 + 0x6 bytes	C++
 	gklayout.dll!nsUserActivityObserver::Observe(nsISupports * aSubject=0x00000000, const char * aTopic=0x023d527c, const unsigned short * aData=0x00000000)  Line 265	C++
 	xpcom_core.dll!nsObserverList::NotifyObservers(nsISupports * aSubject=0x00000000, const char * aTopic=0x023d527c, const unsigned short * someData=0x00000000)  Line 129	C++
 	xpcom_core.dll!nsObserverService::NotifyObservers(nsISupports * aSubject=0x00000000, const char * aTopic=0x023d527c, const unsigned short * someData=0x00000000)  Line 184	C++
 	gklayout.dll!nsUITimerCallback::Notify(nsITimer * aTimer=0x04aebf90)  Line 220	C++
 	xpcom_core.dll!nsTimerImpl::Fire()  Line 424	C++
 	xpcom_core.dll!nsTimerEvent::Run()  Line 514	C++
 	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=0x00000001, int * result=0x0012f864)  Line 511	C++
 	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00d022e8, int mayWait=0x00000001)  Line 227 + 0x16 bytes	C++
 	gkwidget.dll!nsBaseAppShell::Run()  Line 170 + 0xc bytes	C++

This was a storage problem that has been fixed.

• I managed to get the UI locked up one time, again while indexing was going on.

This was probably the garbage collection issue and not a proper deadlock death.

Standard8

test directory server debug output

(Standard8 c13)

-          } else {
-            dump("Wants directory: "+prop+"\n");

Andrew I don't see why you feel the need to delete this. If you're getting it a lot, then maybe something is requesting a directory more than it should be (or we need to fix it). However I see it as a useful tool because if obtaining the directory is buried in code somewhere, it can be very hard to realise that the directory isn't being obtained because we haven't provided a test for it at xpcshell level.

(asuth c14So, I thought I was the one who added it in a fit of debugging. I realized this was not the case when it showed up in the patch, but I left it in because it seemed like it might be more confusing to someone who didn't understand how nsIDirectoryServiceProvider and family work.

For example, I was experiencing a problem where popstate.dat could not be found. I saw that output, saw the source of the dump, saw that it was throwing an exception, and assumed that I needed to be providing another directory path in the handler. Of course, it turns out that the exception is actually a normal thing to do and allows the built-in logic to just build off the single directory path we do provide. So the debug there led me down the wrong path.

My concern can easily be mitigated by adding a comment to the code with the reinstatement of the debug. Does that sound good?

(Standard8 c16) Yes, maybe a clearer debug statement as well - I think it'd just be good to highlight if we are getting things wrong.

(asuth status 9/18) I will integrate this into the next test patch update or at commit preparations if that doesn't happen.

(asuth)I apparently was good to my word. There's now a crazy detailed explanation that gets dumped in that case.

dmose

startup

• At startup, I'm seeing some errors, even though the two files

 mentioned are in my source tree:

2008-10-02 17:12:48     gloda.everybody ERROR   !!! error loading resource://gloda/modules/noun_freetag.js
2008-10-02 17:12:48     gloda.everybody ERROR   (undefined:61) [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: file:///Users/dmose/s/gloda/obj-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/modules/everybody.js :: loadModule :: line 61"  data: no]
2008-10-02 17:12:48     gloda.everybody INFO    ... loading resource://gloda/modules/index_ab.js
2008-10-02 17:12:48     gloda.indexer   INFO    Event-Driven Indexing is now true
2008-10-02 17:12:48     gloda.everybody ERROR   !!! error loading resource://gloda/modules/index_ab.js
2008-10-02 17:12:48     gloda.everybody ERROR   (undefined:61)
[Exception... "Component returned failure code: 0x80520012
(NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]"  nsresult:
"0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame ::
file:///Users/dmose/s/gloda/obj-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/modules/everybody.js
:: loadModule :: line 61"  data: no]

This was likely a transient failure, and I believe the problem has been addressed. The problem of error messages like this being somewhat non-helpful is still there, but that's more of a fundamental problem with poor error reporting from Cu.import than anything else.

gloda.js :: initMyIdentities

(dmose) It would nice if this method were split up into shorter functions for readability/maintainability. If you don't have time to do this now, just add a TODO comment.

(asuth) I think it got more readable now? maybe? it uses various helper methods and seems to have a reasonable flow. I'm going to declare victory, at lesat.

(dmose) myEmailAddresses could be declared inside the for loop

(asuth) It has been re-written in such a way that this is no longer relevant.

(dmose) please track the "deal with account addition/modification/removal" TODO somewhere

(asuth) bug 475502 logged.

(dmose) "Me" wants to come from a .properties file, I bet.

   myContact = GlodaDatastore.createContact(null, null, fullName || "Me",
                                            0, 0);

(asuth) Ack, good point.

(asuth) bug 466212

indexing errors

• recently when indexing, I saw about 8 of these:
• 2008-10-02 18:30:19 gloda.datastore ERROR Synchronous step gave up after 32000 tries.

This is no longer a possible problem since everything happen on the asynchronous thread.

shutdown sqlite/storage assertions

(dmose c38) I shut Thunderbird down while it was in the middle of downloading my giant IMAP Trash folder the first time. Saw the following debug output

2008-09-17 19:11:14     gloda.indexer   INFO    Shutting Down
2008-09-17 19:11:14     gloda.indexer   INFO    Event-Driven Indexing is now
false
2008-09-17 19:11:14     gloda.datastore INFO    Closing pending transaction out
for shutdown.
2008-09-17 19:11:14     gloda.datastore INFO    Closing async connection
WARNING: sqlite3_close failed. There are probably outstanding statements!: file
/Users/dmose/s/gloda/src/mozilla/storage/src/mozStorageConnection.cpp, line 236
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x8052000e
(NS_ERROR_FILE_IS_LOCKED) [mozIStorageConnection.close]"  nsresult: "0x8052000e
(NS_ERROR_FILE_IS_LOCKED)"  location: "JS frame ::
file:///Users/dmose/s/gloda/obj-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/modules/datastore.js
:: gloda_ds_shutdown :: line 493"  data: no]
************************************************************
2008-09-17 19:11:14     gloda.datastore INFO    Closing async connection
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0xc1f30001
(NS_ERROR_NOT_INITIALIZED) [mozIStorageConnection.close]"  nsresult:
"0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame ::
file:///Users/dmose/s/gloda/obj-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/modules/datastore.js
:: gloda_ds_shutdown :: line 493"  data: no]
************************************************************

Doing it a second time, I ended up with a few of these as well:

###!!! ASSERTION: Still pending events!: 'mPendingEvents.Count() == 0', file
/Users/dmose/s/gloda/src/mozilla/storage/src/mozStorageEvents.cpp, line 462

Both of these were running without the IMAP changes from Emre, FWIW.

(asuth status 9/18) With the indexer changes, I will have all asynchronous statements have result trackers combined with a shutdown listener to ensure that they flush before we allow shutdown to complete. Of course, the fundamental problem you encountered is that indexing currently doesn't care about shutdown and so keeps going up until the rug gets pulled out from under it... which will be addressed.

(asuth 10/7) I did what I said I would did, and it actually works as of this changeset.

Noun IDs

(dmose) does having _nextNounID not be persisted means that ids saved in the database could change across Thunderbird invocations if for some reason the ordering of calls to defineNoun change? If so, that seems fragile...

(asuth) Correct, noun id's are neither stable nor consistent (except for the internal, hard-coded ones). However, noun id's never touch the database at this time, so it's more a question of being confusing/useless for debugging. (Our full knowledge of attributes currently relies on them being registered at the start of every session; we don't persist their noun info. Given our current feature-set, this does not preclude the ability to garbage collect unused attributes.)

The existence of noun id's at this point is effectively an optimization, since their canonical identifier is either their name or the NOUN_ID constant on their data model class's prototype. They only would need to be persisted if/when we start putting them in the database for generic references that do not rely on parameterized attributes. Which is to say, we could create a "generic-reference" attribute whose parameter is the name of the noun that it is a generic-reference too. That neatly avoids having the noun id touch the database and allows easy introspection. I'm not sure that's the right solution to whatever problem would drive such a need, though.

Would you prefer that move to using some centralized noun ID registry or begin persisting the noun ID's to head off our likely future need?

(dmose) I don't think that needs to be done before landing. Please just track this issue somewhere (a bug, wiki page, wherever), and let's deal with it when a concrete need arises.

(asuth) bug 458992 logged.

Noun Definition

(dmose) instead of inventing the term "first class" that newbies to the code will have to learn, how about calling the nounMeta property "allowsAttrs" or "allowsArbitraryAttrs"? This seems like it'll be more obvious.

(asuth) Yes, this sounds like a good idea, I will do it.

(asuth) I did it!

general

• (do now or track for later): At some point you might want to pull in a current copy of log4moz.js; the enumerateProperties stuff that landed not too long ago looks _really_ useful

pulled current copy

• having indexing happening while we're on battery power is seems like it could really eat battery life. sdwilsh was talking about adding an API to Gecko to check that. In the meantime, being "offline" seems like it could be a good proxy for that. Can you file a bug on looking into what sort of strategy we want for the 3.0 timeframe?

This is implemented.

• (this was observed):

###!!! ASSERTION: Still pending events!: 'mPendingEvents.Count() == 0', file
/Users/dmose/s/gloda/src/mozilla/storage/src/mozStorageEvents.cpp,
line 462

This was a mozStorage bug that is now fixed.

• In the cases where you don't actually care about the index in a an Iterator:
  • foreach (let [iItem, item] in Iterator(aItems))
• it's clearer to explicitly ignore the index like so:
  • foreach let [,item] in Iterator(aItems)
• Alternately, you could use items.forEach(), possibly in combination with an expression closure:
  • items.forEach(function foo() expression);

I have done the former. I was avoiding the latter because it seemed likely to be less efficient, but I didn't really have any proof of that, and with tracing now enabled, it is likely to be moot. I think I'll leave things as-is because I do prefer the former construct slightly.

collection.js

• It looks like the GlodaCollection constructor duplicates a bunch of code from _onItemsAdded. Could it just call that method directly?

Good point, I have done this.

• Why use apply here rather than calling this.items.push() directly?

  _onItemsAdded: function gloda_coll_onItemsAdded(aItems) {
    this.items.push.apply(this.items, aItems);

I use apply as an equivalent to the python func(*args) mechanism to spread a list of arguments for a function that expects multiple arguments rather than a list of arguments. I have done this in the hope that it is faster than repeatedly calling push, and more efficient than using concat (which, to my understanding, would create a new list and have us abandon the existing list to GC.)

datamodel.js

• having raw attributes defined as a tuple of [attribute id, parameter, value] means that consumers end up accessing by integer index, which feels error prone (and makes calling code unnecessarily hard to read). How about making them be objects instead, so that the accessors are property names? e.g. { id: 1, parameter: "foo", value: "bar" }

The refactoring has eliminated the APV representation and its variants for nearly all purposes. The representation still exists for adding/removing the attribute database rows, but no code apart from the datastore's add/remove code uses them, and the representation is appropriate in that case because that's the database row format.

• while all the getter magic in the MixIn is elegant, it feels to me

 like it also makes the code less approachable.  This would seem like
 another reason to move towards raw storage fields.

Post-refactoring, the MixIn's functionality to that end no longer exists. I have added some helpers that assist in dealing with the attributes generically, however. For example, if you want to walk all of the attributes on an object along with the helpers, there is a function that aids in doing this. But attribute access no longer requires any helpers, as you can simply directly access the object like a dictionary.

/**
 * @TODO Return the collection of messages belonging to this conversation.
 * (And weakly store a reference to the collection.  Once the user is rid of
 *  it, we really don't care.)
 */

• What's still TODO in the above comment? Also, the last sentence isn't very clear, please be more explicit about what's intended.

This has been mooted by a proper change to the conversation object to have it construct/return a fully asynchronous collection of the messages belonging to the conversation.

• Should GlodaMessage.toString() use something from a properties file? Or will this never be displayed to end users?

toString is never intended to be user-visible. It is only for debugging assistance and to ensure that use of the objects as keys in object dictionaries works reasonably without false collisions.

• GlodaMessage.{_nuke,_ghost} don't appear to be private methods, so presumably they want to have doxygen comments and to not begin with an underscore.

They are not public. They are more private but friend accessible. Someone with access to a GlodaMessage instance should not be under the impression those methods are for them to use; they are only there for the indexer.

• In the GlodaMessage folderMessage getter, shouldn't both of those LOGs be at a higher level than just "info"?

• does the special property want to be a boolean instead of an integer?

no, this is now an enumerated value.

• explain doesn't appear to be called anywhere, so it can presumably go away

It was used by expmess, but I gutted it because it was causing string issues and turned out to be less useful than I originally planned. now fully excised

• seems like it would make sense to provide a boundName setter rather than allowing Gloda.js to reach inside this GlodaAttributeDef

I've moved it to just be an attribute; if we're not going to be read-only and not have any logic, no need for getters and setters. changeset

• getValueFromInstance calls getAttributeInstances, which only exists on GlodaMessage objects, so presumably it will fail when called on any other type of object. Is this intentional? If so, please document...

As of my contact generalization changes getAttributeInstances is now part of a mix-in that GlodaMessage and GlodaContact both get, plus anyone else who wants in.

explattr.js

• it'd be nice to have a brief (1 sentence) comment at the top describing the file and pointing to the more detailed description of explciit addrs

done

• the comment above the class object definition appears to be wrong

previously done

everybody.js

• instead of adding "" to an exception to force it to be a string, either call the .toString method (best) or use the String() conversion operator (ok too)

rendered moot by more extensive formatting

gloda.js

• Why bother having a local let variable in getMessageForHeader?

It probably existed for debugging, but is now gone, thanks.

• use @return in the documentation for getMessageForHeader, please.

done

• Please figure out how to track the attribute orphaning issue somewhere (spin-off bug, wiki page...)

Created bug 458989.

• defineAttribute
  • please choose something more meaningful than HATHATHAT to increase readability

This method was refactored and no longer contains redundant code-paths that require manual upkeep.

utils.js

• A brief comment explaining why dateFormat exists and why it lives where it does would be helpful (unless you want to get rid of it now).

I have nuked it and the gloda message view that required it. See changeset.

• I'm a little confused about why md5HashString is converting from UTF-8.

 My understanding is that the way Gecko is currently compiled, Spidermonkey
 strings are always UCS2/UTF16.

Per the doxygen, the implementation is actually from the nsICryptoHash documentation, so I disclaim responsibility for wackiness. However, I will note that nsICryptoHash::update takes an octet array, increasing the complexity.

• The parseMailAddresses comment is ironically, a bit difficult to parse :-). Maybe clarify the sentence a bit or show an example?

doc improved

• extractBodySnippet doesn't currently do anything or have any callers. if you plan to use it, feel free to leave, but if it's cruft left over from a previous iteration....

gone

memory issues

(dmose) with the new indexer, I've seen RSIZE get extremely large (> 1G) once. Perhaps re-indexing my store would trigger that behavior again.

(asuth) by new indexer, do you mean the sweep-based, event-augmented indexer completed in changeset 13592e09f549 and pushed on the morning of Sunday, Sep 21? If not, then I think we're okay. If so, then it's cause for concern, although I think it would mainly be the garbage collector's fault for being lazy and maybe we can kick it to make it collect more frequently.

(dmose) I did mean the indexer completed in the changeset you referenced, so I suspect we still have some sort of issue.

(asuth status 10/7) I still need to look into this, especially on OS X.

(asuth)Gloda had a problem where it would not close the databases when it was done with them. This would lead to memory issues. That gloda issue has been addressed.

more general

• be sure to kill the various UI stuff that we don't need before landing (properties, dtd, preferences stuff, etc.)

I believe philor killed this stuff. (It's gone, in any event.)

• if there isn't one already, please file a bug on adding or changing the nsIMsgFolderNotificationService to have a notifyMsgsAdded method so we don't get so many method calls the first time a folder is opened

bug 475506

• having indexing happening while we're on battery power is seems like it could really eat battery life. sdwilsh was talking about adding an API to Gecko to check that. In the meantime, being "offline" seems like it could be a good proxy for that. Can you file a bug on looking into what sort of strategy we want for the 3.0 timeframe?

bug 475505

gloda.js / everybody.js file structuring

(dmose) your gloda_int.js theory in the js comments sounds right; if it's easy to do (it should be, right?) is now the time? If now's not the time, please file a spinoff bug or figure out some other way to ensure that this work gets tracked...

(asuth) I implemented public.js to this end. I feel like this is probably sufficient for the time being, especially because it's somewhat more obvious/explicit.

datamodel.js

• should the argument to getValueFromInstance actually be called aNoun?

method no longer exists.

• some interesting debug spew that makes me think gloda might be streaming

something on thread where it's not expecting to be streamed:

2008-09-21 19:39:41     gloda.indexer   DEBUG   0 candidate messages
2008-09-21 19:39:41     gloda.indexer   DEBUG   ...creating new message.
  body length: 1564
2008-09-21 19:39:41     gloda.datastore DEBUG   CreateMessage: 8, 123015, 520,
 1207609855000000, 47FAA9FF.6020206@redhat.com
WARNING: NS_ENSURE_TRUE(mHeaderSink) failed: file /Users/dmose/s/gloda
/src/mailnews/mime/src/mimecms.cpp, line 294
###!!! ASSERTION: nsSMimeVerificationListener not thread-safe:
 '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /Users/dmose
/s/gloda/src/mailnews/mime/src/mimecms.cpp, line 275
###!!! ASSERTION: PipUIContext not thread-safe: '_mOwningThread.GetThread() == 
PR_GetCurrentThread()', file /Users/dmose/s/gloda/src/mozilla/security/manager
/ssl/src/nsNSSComponent.cpp, line 2739
2008-09-21 19:39:41     gloda.indexer   DEBUG   *** Indexing message: 186254 : 
FYI
2008-09-21 19:39:41     gloda.indexer   DEBUG   1 candidate messages
2008-09-21 19:39:41     gloda.indexer   DEBUG   candidate folderID: null 
messageKey: null

I feel that bug 475499 (gloda should avoid triggering non-libmime handlers) from another point probably covers it. Namely, the existing JS implementation handles multi-threaded shenanigans quite well. Our JS implementation for the mime processing does nothing to non-threadsafe dudes. The only problem is these jerk listeners we don't want listening in anyways.

datamodel.js

• s/trigger his caching/trigger it's caching/

Comment no longer exists due to re-write.

• getValueFromInstance: in the case where this._singular is set but instances.length > 1, should we assert or throw? If not, how about documenting exactly what's going to be returned?

Also no longer exists.

gloda.js

• Seems like "nounDef" would be clearer and more readable than "nounMeta". It would also parallel attrDef nicely.

This change was made.

• defineAttribute: please choose something more meaningful than HATHATHAT to increase readability

I refactored the code and lost HATHATHAT.

general

• At startup, I'm seeing some errors, even though the two files mentioned are in my source tree:

2008-10-02 17:12:48     gloda.everybody ERROR   !!! error loading resource://gloda/modules/noun_freetag.js
2008-10-02 17:12:48     gloda.everybody ERROR   (undefined:61) [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: file:///Users/dmose/s/gloda/obj-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/modules/everybody.js :: loadModule :: line 61"  data: no]
2008-10-02 17:12:48     gloda.everybody INFO    ... loading resource://gloda/modules/index_ab.js
2008-10-02 17:12:48     gloda.indexer   INFO    Event-Driven Indexing is now true
2008-10-02 17:12:48     gloda.everybody ERROR   !!! error loading resource://gloda/modules/index_ab.js
2008-10-02 17:12:48     gloda.everybody ERROR   (undefined:61)
[Exception... "Component returned failure code: 0x80520012
(NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]"  nsresult:
"0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame ::
file:///Users/dmose/s/gloda/obj-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/modules/everybody.js
:: loadModule :: line 61"  data: no]

This is just crappy error reporting on the part of the platform. It claims it can't find files that don't load. Something must have been broken.

• recently when indexing, I saw about 8 of these:

2008-10-02 18:30:19     gloda.datastore ERROR   Synchronous step gave up after 32000 tries.

The massive database overhaul did away with the synchronous reads from the main thread. Problem solved!

datamodel.js =

• having raw attributes defined as a tuple of [attribute id, parameter, value] means that consumers end up accessing by integer index, which feels error prone (and makes calling code unnecessarily hard to read). How about making them be objects instead, so that the accessors are property names? e.g. { id: 1, parameter: "foo", value: "bar" }

There is still a form somewhat like this for the internal representation when serializing to the database, but this is no longer an issue because we are doing raw storage.

• while all the getter magic in the MixIn is elegant, it feels to me like it also makes the code less approachable. This would seem like another reason to move towards raw storage fields.

We did it! Woo! Raw storage!

/**
 * @TODO Return the collection of messages belonging to this conversation.
 * (And weakly store a reference to the collection.  Once the user is rid of
 *  it, we really don't care.)
 */

• What's still TODO in the above comment? Also, the last sentence isn't very clear, please be more explicit about what's intended.

The comment is gone.

• Should GlodaMessage.toString() use something from a properties file? Or will this never be displayed to end users?

Never be displayed to end users. Just for debugging.

• GlodaMessage.{_nuke,_ghost} don't appear to be private methods, so presumably they want to have doxygen comments and to not begin with an underscore.

They are implementation private methods. Nothing outside of gloda should use them. I agree that they should be documented; other documentation requests sufficiently capture that, though.

• In the GlodaMessage folderMessage getter, shouldn't both of those LOGs be at a higher level than just "info"?

They aren't entirely unexpected things. Just things we want to be aware of when they happen, when developing. Don't need to clog up the error console with them.

• Presumably the GlodaContact.pictureURL attribute doesn't want to have its format and source hardcoded. OK by me to track this somehow and address later, though.

bug 475512

• Given that GlodaHasAttributeMixin.get_rawAttributes uses getMessageAttributes under the hood, why/how does it actually work when mixed into an (eg) GlodaContact?

Mooted by raw storage changes.

general

• Saw a few of these while indexing:

###!!! ASSERTION: Still pending events!: 'mPendingEvents.Count() == 0', file
/Users/dmose/s/gloda/src/mozilla/storage/src/mozStorageEvents.cpp,
line 462

This was a storage bug, since fixed.

• Saw this in my error console today:

2008-10-09 14:40:46	gloda.indexer	ERROR	Inconsistency in
 conversations invariant on undefined.  It has conv id 3039 but expected 3042

I had broken the conversation logic for a bit. It was a sad thing.

• When running the indexer on my entire mailstore with lightning installed, I see a bunch of these fly by, presumably for invites that I've got lying around. Not sure whether this is a lightning or a calendar issue:

Error: convertToHTML: Cannot create itipItem: [Exception... "Component
 returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER)
 [nsIMsgMailNewsUrl.msgWindow]"  nsresult: "0x80004003
 (NS_ERROR_INVALID_POINTER)"  location: "JS frame ::
 file:///Users/dmose/s/gloda/obj-tb/mozilla/dist/ShredderDebug.app/
 Contents/MacOS/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/
 components/lightningTextCalendarConverter.js :: lmcCTH :: line 226"  data: no]
Source File:
 file:///Users/dmose/s/gloda/obj-tb/mozilla/dist/ShredderDebug.app/
 Contents/MacOS/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/
 components/lightningTextCalendarConverter.js
 Line: 236

bug 475499 desires to not let other things trigger on our streaming.

• GlodaFolder stuff in gloda.js and datamodel.js needs doxygen comment love

What's with you and documentation, man? ;) (General documentation needs well understood, dropping this specific point.)

collection.js

  // should we also maintain a unique value mapping...
  if (this._nounMeta.usesUniqueValue)
    this._uniqueValueMap = {};

  this.items = aItems || [];
  this._idMap = {};
  if (this._uniqueValueMap) {

• If you bump those two initializers up above the first if, you should

be able to collapse the two ifs into one.

I somehow addressed this so that it is moot.

  _onItemsAdded: function gloda_coll_onItemsAdded(aItems) {
    this.items.push.apply(this.items, aItems);

• Why use apply here rather than calling this.items.push() directly?

One call does it all, versus needing a loop? I superstitiously believe it to be more efficient, although I could see how with tracing it might actually end up slower.

• It looks like the GlodaCollection constructor duplicates a bunch of code from _onItemsAdded. Could it just call that method directly?

I do that now; thanks for the suggestion.

• does _onItemsRemoved want to be _onItemsDeleted for symmetry?

I say no, because 'deleted' has certain connotations that removed does not. Namely, something can be removed because it no longer matches a filter constraint (ex: posessing a tag), as opposed to having been deleted from the message store.

• _onItemsRemoved seems pretty complex for something so basic. It's not clear to me why making it simpler would be O(n^2). Can you elaborate?

Removing the contents of one list from another list is going to be O(n^2) unless you explode one of the lists to a hashset representation. Namely, list removal is O(n), we have up to n items, etc. I'm not sure there's really a more concise way to do the hashset exploding with the tools at hand, and we do have a bit of bookkeeping.

• the last half of GlodaLRUCacheCollection.delete appears to be

duplicated in GlodaLRUCacheCollection.add. Factor that out into a separate method that can be shared? An _unlinkItem could be similarly factored out of a couple of methods for improved readability. Neither of these things are huge wins, though, so if you want to do them later in another bug, that's fine.

I don't think the duplication level is too high, and the unlink cases are generally specialized to the case. Going to leave things as is for now.

deletion

• Just saw the following output:

2008-10-10 12:05:05     gloda.indexer   DEBUG   folderDeleted notification
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
 [Exception... "'[JavaScript Error: "this._datastore is undefined" {file:
 "file:///Users/dmose/s/gloda/obj-tb/mozilla/dist/ShredderDebug.app/
 Contents/MacOS/modules/indexer.js" line: 1591}]' when calling method:
 [nsIMsgFolderListener::folderDeleted]"  nsresult: "0x80570021
 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"
  data: yes]
************************************************************

Yes, that code had a bad case of the this-is-not-what-we-thought-it-was and perhaps some typos. Those have been resolved, although deletion is somewhat broken still, but that has a bug on it.

general

* In the cases where you don't actually care about the index in a an Iterator:

  foreach (let [iItem, item] in Iterator(aItems))

it's clearer to explicitly ignore the index like so:

  foreach let [,item] in Iterator(aItems)

Alternately, you could use items.forEach(), possibly in combination
with an expression closure:

  items.forEach(function foo() expression);

I do the former now, thanks for the suggestion.

gloda.js

• in _bindAttribute, it would be clearer if nounMeta were explicitly called objNounMeta (or objNounDef, as suggested elsewhere), to contrast it with subjectNounMeta. Also, the comment referring to "on-object" bindings is confusing because it's referring to a JS object representing the Gloda "subject" in the middle of code that touches Gloda "objects".

I do this now, thanks.

build lossage

• something with lots of:

2008-10-12 17:55:01     gloda.datastore ERROR   Synchronous step gave up after
 32000 tries.

This was the synchronous stuff previously mentioned and noted as mooted.

• "Expression tree is too large" lossage

bug 463736

query.js

• (Split based on action) Looks like neither WILDCARD [is not] used by any callers. Unless you know of a specific use case for them, I'm somewhat inclined to suggest axing them in the interest of simplicity. This would also allow the newQuery documentation comment in gloda.js to be made more understandable as well. On the other hand, this isn't a huge win.

WILDCARD is used for full-text search; it's a sentinel object.

• I think test() would be easier to grok if you split the then and else clauses of |if attribDef.singular| out into separate functions. It might be worth collapsing them into one since they're almost identical, but it's possible that the split is better for readability.

Test has been refactored and is now a lot cleaner. We just promote the singular cases into the list case.

• The whole structure of the APV as "sometimes [attr, param, value]" and "sometimes that stuff + other things in a contextually determined ordering" seems both hard to read and error-prone. This wants to be a hash, I suspect.

The crazy APV representation has been normalized to [constraint type, attribute def, values...]. A dict might have been a good idea too; I'm coming from python-land where tuples are much much more efficient than objects/dictionaries where possible. Of course, in spider-monkey land, since dictionaries are objects, and objects can be fairly efficient with shape stuff, it's less of an issue and I should probably try and break myself of that habit.

glautocomp.js

• in ACGR_addRows, why use apply? It appears to be setting |this| to what it would have been set to anyway.

Previously remarked-upon; it avoids the perceived waste of concat and yet is only a single call to push. May be crazy.