495304 - gloda mimemsg fails if asked to stream same set of messages twice, synchronously

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[David Ascher (:davida)]

Andrew's noticed that the JS mime streaming code sometimes fails inexplicably.  I guess I should be happy that I now have STRs.

Specifically, with the recent thread-summary view, if one deletes the message _before_ a collapsed thread by e.g. hitting the delete key (or archive will work), then the thread summary code is (correctly) called, but that code will cause the JS mime emitter to stream all of the messages in the thread pretty much simultaneously.  At least on my mac, it's very easy to get it to fail to stream at least one if not all messages in a thread.

The UX symptoms are that the snippets and tags aren't displayed in the thread summary.

The high-level code problem is that the MsgHdrToMimeMessage() call ends up being called with a "blank" MimeMessage object as argument.

NOTE: selecting a thread by clicking on the collapsed thread in the threadpane _never_ seems to cause this problem.  Only selection that happens as a result of getting rid of whatever the existing selection currently is causes the failure.

I'll let asuth comment about his suspicions as to where in the mime handling the problem is.

Comment 1

[David Ascher (:davida)]

Additional note: this happens w/ local folders as well, so it's not IMAP specific.

Comment 2

[David :Bienvenu]

Attachment: don't load collapsed message - checked in.

this makes summarizeSelection return a boolean indicating if the commandUpdater did summarize the selection - if so, nsMsgDBView::SelectionChanged knows it doesn't have to load the selected message.

Comment 3

[David :Bienvenu]

at this point, all the failures I'm seeing have to do with compaction of local folders changing the message key and causing getMessageCollectionForHeaders to exclude the headers we pass to it.

Comment 4

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

Comment on attachment 380506 [details] [diff] [review]
don't load collapsed message - checked in.

This also lets summarizeSelection otherwise co-opt the message display process.  Muah hah ha!

Comment 5

[David Ascher (:davida)]

Hmm, even w/ this, I'm not having reliable summary update in the scenario described in comment #0 on imap folders.  It's _better_, no doubt, but not yet 100%.  That said, that's testing w/ a patched up version of the folder sumary code, so it might all be my code breakage.

Comment 6

[David :Bienvenu]

Hmm, it's hard for me to tell because of the compact folder issue which messages actually have snippets.

This will fix the marking read of the first message when you land on a collapsed thread, however.

Comment 7

[David Ascher (:davida)]

The last patch (https://bugzilla.mozilla.org/attachment.cgi?id=380861) on bug 454829 should make tracking this bug down easier.

Comment 8

[David :Bienvenu]

I'm running with that patch - I'm seeing that we're streaming all the messages, but, I suspect never getting called back from gloda. So I'll add back my dump statements and see where things are going awry.

Comment 9

[David :Bienvenu]

we're getting ondataavailable calls from the mailbox message code - I suspect we're losing things somewhere between there and the js mime emitter...

Comment 10

[David :Bienvenu]

I believe the problem is that we're trying to stream the set of collapsed messages twice, in the case of deleting a message and landing on a collapsed thread. I think that confuses the jsmime emitter because it uses the url spec as an array index...

Comment 11

[David :Bienvenu]

so yeah, we're generating two selection changed notifications, which means two calls to summarizeSelection, in the case of deleting a message and landing on a collapsed thread.

Comment 12

[David :Bienvenu]

I've updated the summary - the issue is that mimemsg deletes the msg in RESULT_RENDEVOUZ and streaming the same set of messages twice doesn't work well because of it. But we don't want to be streaming the same set of messages twice anyway so I should fix that.

Comment 13

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

Does "fix that" = do not stream the message twice?  It's fairly likely the mimemsg.js/jsmimeemitter.js logic will strike again.

Actions that make sense to me, from worst to best:
- Just throw an exception.  This is likely to at least produce some error message before breaking code in ways it is unlikely to be prepared for.
- Don't use the sketchy and poorly spelled RESULT_RENDEVOUZ.  Unfortunately, I saw no good way to stash our MimeMsg on a per-URI basis when I originally wrote that.  The downside to this is it's very redundant.
- If we are trying to stream something we are already streaming, just add the extra callback to a list of callbacks rather than kicking off a new stream, so that everyone can benefit from the result.  This would be sort-of like a caching mechanism, in the event that various extensions all hook off message display change and all want the message.  (We could also potentially implement a weak-ref based caching mechanism, but that seems like overkill when we would instead want to just provide a more extension-friendly mechanism so the case never arises.)

Comment 14

[David :Bienvenu]

I don't think the summarizeSelection code should do all that work twice, even if it did work. So perhaps we should try to fix both issues. The other thing is that the other code that gets snippets (e.g., the new mail alert) sticks the snippet in the msg hdr in the .msf file, as the "preview" property so that we don't have to recalculate it all the time. I'm wondering if we should do the same thing for these snippets. If I click on a thread with a couple megabyte messages, we're going to stream the whole message in order to get the first 200 bytes using MsgHdrToMimeMessage. Or stick the preview text in the sqlite db...

Comment 15

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

Right, I agree that summarizeSelection should stop doing things twice.  But we should also deal with the deficiency in the mimemsg API/implementation.

Using the preview property sounds like the right solution for the 200 character case.  If people start getting concerned about disk space, maybe compaction could strip those properties.  One complicating factor is that the connotent implementation supports pluggable whittlers, so there is the issue of staleness.  For example, the gp-bugzilla plugin will get rid of the bugzilla e-mail cruft to show only the user's comments, and if it is installed after some things are stashed as properties, stale values will result.  I think the benefit of potentially not pegging the user's system far exceeds the wacky behavior on that front or additional implementation complexity of dealing with it, though.

Gloda already stashes the full body text (with quoting removal) of the message in the SQLite db, as it must do to use the FTS3 engine.  The issues are that not all messages are gloda indexed, we only want to access that on the async thread, and the gloda queries try and build up the full object model which results in multiple rounds of queries.

Comment 16

[David :Bienvenu]

(In reply to comment #15)
> Right, I agree that summarizeSelection should stop doing things twice.  But we
> should also deal with the deficiency in the mimemsg API/implementation.

I'm working on the latter now - my head was getting woozy from trying to figure out how to do the former. I also tried to figure out how to avoid having selectionChanged getting called twice, but there are so many edge cases with selection.
> 
> Using the preview property sounds like the right solution for the 200 character case. 
I can add that while I'm in here.

> If people start getting concerned about disk space, maybe compaction
> could strip those properties.

That's a thought. Or maybe we want some sort of disk pressure scheme similar to the memory pressure scheme that gecko uses.

Comment 17

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

(In reply to comment #16)
> (In reply to comment #15)
> > Right, I agree that summarizeSelection should stop doing things twice.  But we
> > should also deal with the deficiency in the mimemsg API/implementation.
> 
> I'm working on the latter now - my head was getting woozy from trying to figure
> out how to do the former. I also tried to figure out how to avoid having
> selectionChanged getting called twice, but there are so many edge cases with
> selection.

Yes, selection is a nightmare.  However, I have enough abstractions in bug 474701 that in theory it may just be easier to deal with the problem there.  It is trivial for it to only re-notify the summarizer when the selection actually changes.  For example:

// sorta-pseudo-code to go in the summarizeSelect() notification from the dbview:
if (gFolderDisplay.selectedIndices().toString() !=
    summarizedIndices.toString()) {
  goSummarize();
  summarizedIndices = gFolderDisplay.selectedIndices();
}

However, if someone could explain/point me at an explanation of why the double events are generated in the first place, that would make that even easier.  I think I've seen it around but do now know where it went.

Comment 18

[David :Bienvenu]

(In reply to comment #17)

> is trivial for it to only re-notify the summarizer when the selection actually
> changes.  For example:
> 
> // sorta-pseudo-code to go in the summarizeSelect() notification from the
> dbview:
> if (gFolderDisplay.selectedIndices().toString() !=
>     summarizedIndices.toString()) {
>   goSummarize();
>   summarizedIndices = gFolderDisplay.selectedIndices();
> }

If I delete a collapsed thread, we'll generate a the exact same selected indices again, as everything moves up a row to replace the deleted thread row. So it has to be a little more complicated :-)

> 
> However, if someone could explain/point me at an explanation of why the double
> events are generated in the first place, that would make that even easier.  I
> think I've seen it around but do now know where it went.

We're both setting the selected index on the treeSelection, which generates a selectionChanged notification from inside the tree code, and for good measure, we're explicitly calling selectionChanged. The latter is perhaps because we want to generate a synthetic selectionChanged event, to force a resummarize. For example, if I expand a selected & collapsed thread, we want to have selectionChanged called, even though the selected index hasn't changed (not that that's what's going on in this case, but there are a number of places where selectionChanged calls have been sprinkled like pixie dust, including one that says "don't worry [be happy!] it's cheap if we end up calling it twice" :-)

Comment 19

[David :Bienvenu]

Attachment: proposed fix

turns the callback and callbackThis into arrays, and activeListeners into an object/associative array, and handles duplicate stream requests by adding callback to existing stream listener, and send the notifications to all the listener. Apologies if I've done something silly!

Comment 20

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

Comment on attachment 381419 [details] [diff] [review]
proposed fix

>diff --git a/mailnews/db/gloda/modules/mimemsg.js b/mailnews/db/gloda/modules/mimemsg.js
>   onStopRequest: function (aRequest, aContext, aStatusCode) {
...
>+    for (let i = 0; i < this._callbacksThis.length; i++)
>+      if (this._callbacksThis[i])
>+       this._callbacks[i].call(this._callbacksThis[i], this._msgHdr, message);
>+      else
>+        this._callbacks[i].call(null, this._msgHdr, message);

nit: indentation

but also, since this._callbackThis[i] will be null, you can just reduce it to the first case and lose the if and the else clause, so it just becomes:
this._callbacks[i].call(this._callbacksThis[i], this._msgHdr, message);

> function MsgHdrToMimeMessage(aMsgHdr, aCallbackThis, aCallback) {
>   shutdownCleanupObserver.ensureInitialized();
> 
>   let msgURI = aMsgHdr.folder.getUriForMsg(aMsgHdr);
>   let msgService = gMessenger.messageServiceFromURI(msgURI);
> 
>+  // if we're already streaming this msg, just add the callback
>+  // to the listener.
>+  let listenerForURI = activeStreamListeners[msgURI];
>+  if (listenerForURI != undefined) {
>+    listenerForURI._callbacks.push(aCallback);
>+    listenerForURI._callbacksThis.push(aCallbackThis);
>+    return;
>+  }

Because of the wacky variable-argument idiom, this can go wrong.  I'm not opposed to bailing on it, but otherwise change those to:
listenerForURI._callbacks.push(aCallback ? aCallback : aCallbackThis);
listenerForURI._callbacksThis.push(aCallback ? aCallbackThis : null);

r=asuth with suggested changes made.

Comment 21

[David :Bienvenu]

fix checked in, with suggested changes made.

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 381419 [details] [diff] [review]
proposed fix

sr=Standard8, although it appears you've already checked this in ;-)

Comment 23

[David :Bienvenu]

ack, sorry...guess I was so excited to fix a gloda bug that I jumped the gun ;-)

Comment 24

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

Don't worry, there's more than enough to go around... ;)