538750 - One thread involving multiples identities of me is incorrectly summarized as two distinct conversations

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Jonathan Protzenko [:protz]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2b5) Gecko/20091204 Firefox/3.6b5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.5) Gecko/20091204 Thunderbird/3.0

What appears as one thread only in the message list (threaded view) is inconsistently summarized as two different conversations in the message pane below.

The first time it was triggered by me replying with a first identity, and then a second identity to the same thread. The second time, it was roughly :

Me (address A) -> Me (address B)
Reply (address B) -> To me (address A)

I'm using the Smart Inbox which is why these messages appear as one thread. But when the selection summary is built (with the thread not collapsed, of course), it's a MultiMessageSummary that's used, not a ThreadSummary.

Reproducible: Always

Steps to Reproduce:
Just send yourself two emails with the scheme above, and view this as one thread in the Smart Inbox.



I'm not sure I filed this in the right component, so feel free to change it...

Comment 1

[Jonathan Protzenko [:protz]]

Attachment: A quick patch that checks that this actually involves multiple threads

Comment 2

[Jonathan Protzenko [:protz]]

Hi,

I've added a patch that fixes this. Basically, before summarizing messages as two different conversations, we check whether this actually involves two different threads using gDBView. If it's actually one single thread, we switch to summarizeThread, otherwise we go on with summarizeMultipleSelection.

Although this should probably be fixed at a higher level, this patch fixes this quite cleanly I think. The file involved is selectionsummaries.js (http://mxr.mozilla.org/comm-central/source/mail/base/content/selectionsummaries.js).

CC'ing davida as he's the original author of the file.

Comment 3

[David Ascher (:davida)]

Interesting bug, and awesome that you have a patch!

Wouldn't it make sense to move the logic closer to http://mxr.mozilla.org/comm-central/source/mail/base/content/messageDisplay.js#342 though?

It's too early in the morning for me to tell why that logic fails, but I suspect that's where the right fix probably lies.

cc'ing bienvenu, who'll have some opinion I suspect.

Comment 4

[David Ascher (:davida)]

moving to NEW because while I haven't tested it I believe it =).

Comment 5

[Jonathan Protzenko [:protz]]

Attachment: A new patch that operates on messageDisplay.js and tackles the issue earlier in the process.

This new patch addresses the issue earlier (in messageDisplay.js rather than in selectionSummaries.js), however, I'm still uncertain as to why this works and not the code that was there before, so I'd be glad to collect some advice.

This works both if the thread is folded or unfolded in the Smart Inbox view. There might be a deeper cause to the issue, if I have some spare time I will try to investigate more and get more information regarding this issue.

Could someone please confirm and maybe shed some light on this?

Comment 6

[David :Bienvenu]

The basic issue is that msgHdr.threadId is a per-folder thread id, whereas the view's concept of a message header's thread can be cross-folder, if the view is cross-folder. So your code is doing the right thing, basically.

Comment 7

[Jonathan Protzenko [:protz]]

Comment on attachment 427138 [details] [diff] [review]
A new patch that operates on messageDisplay.js and tackles the issue earlier in the process.

According to https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch, at this point, I should ask for review. I guess bienvenu is the right person to ask.

Comment 8

[David :Bienvenu]

Comment on attachment 427138 [details] [diff] [review]
A new patch that operates on messageDisplay.js and tackles the issue earlier in the process.

Thx for working on this! Can I get a -u diff for this?

Comment 9

[Jonathan Protzenko [:protz]]

Attachment: Same patch but using hg diff with recommended setup

Ok, same patch, but this time with hg diff --git -p -U 8 as recommended on MDC. If I got this wrong, feel free to complain, I'll send in a new patch.

Comment 10

[David :Bienvenu]

Sorry for the delay; I tried your patch, and it failed for the test case I came up with. I'm not sure why because your patch looks like it's doing the right thing. My test case was two messages in my smart drafts folder, from two different drafts folders. I selected an unrelated message, and the collapsed thread, and I saw three different entries in the summary pane, whereas I should see two, one for the thread, w/ a box around it, and one for the unrelated message. 

I think this code needs to change as well:

  summarize: function() {
    let htmlpane = document.getElementById('multimessage');
    // First, we group the messages in threads.
    // count threads
    let threads = {};
    let numThreads = 0;
    let headerParser = Components.classes["@mozilla.org/messenger/headerparser;1"].
                         getService(Components.interfaces.nsIMsgHeaderParser);
    for (let [,msgHdr] in Iterator(this._msgHdrs))
    {
      if (! threads[msgHdr.threadId]) {
        threads[msgHdr.threadId] = [msgHdr];
        numThreads += 1;
      }
      else {
        threads[msgHdr.threadId].push(msgHdr);
      }
    }

Comment 11

[David :Bienvenu]

we probably want a mozmill test for this case as well...we have a mozmill test for summarization, test-summarization.js. It would just need to be extended to test the cross-folder saved search case, probably just using a smart folder.

Comment 12

[Jonathan Protzenko [:protz]]

(In reply to comment #10)
> Sorry for the delay; I tried your patch, and it failed for the test case I came
> up with. I'm not sure why because your patch looks like it's doing the right
> thing. My test case was two messages in my smart drafts folder, from two
> different drafts folders. I selected an unrelated message, and the collapsed
> thread, and I saw three different entries in the summary pane, whereas I should
> see two, one for the thread, w/ a box around it, and one for the unrelated
> message. 
> 
> I think this code needs to change as well:
You're absolutely right. It's the same issue as before: although the code now detects correctly whether it's a MultiMessageSummary or a ThreadSummary that should be created, the grouping in MultiMessageSummary is done according to the folder thread-id instead of the current view's thread-id. I should be able to come up with a new patch in a few days.

Comment 13

[Jonathan Protzenko [:protz]]

I need some advice on this. Basically, what happens is that we need to recover the root message in the thread (according to the *view*) for each msgHdr that's passed to MultiMessageSummary.

Before going further, please note that gFolderDisplay.selectedMessages contains messages that are hidden by collapsed threads, while selectedIndices does not.

The problem lies in getting the root message for a msgHdr that's under a collapsed thread. The only way I can see of doing that involves calling findIndexOfMsgHdr(msgHdr, true) which will actually expand the thread, and then calling getThreadContainingIndex(), getting the first message, and finally collapsing back the thread. Not very efficient, is it? We can't use selectedIndices because there's no correspondence between that and selectedMessages. They often don't have the same length.

Any ideas?

Comment 14

[Jonathan Protzenko [:protz]]

Bienvenu, last time we discussed this on IRC, the conclusion was you had to implement some threadId property on nsIMsgDbHdrs so that we can recover the threadId from any msgHdr, even those that are hidden by collapsed threads.

Can I mark this as assigned to you?

Comment 15

[David :Bienvenu]

I think the idea was to make nsMsgXFViewThread have a thread id, and add a method to the view that allows you to get the view thread object for a given msg hdr. You can assign to me, but I don't know when I'm going to get to it.

Comment 16

[Jonathan Protzenko [:protz]]

David, is there any chance we can check in at least the current patch? It is very short and is likely to improve the experience for all the users which use threaded view and smart folders (sorry, unified folders ;-)). I think the benefit from this would be high. If I provide mozmill tests for that case, do you think it could make it for rc1?

Comment 17

[David :Bienvenu]

Jonathan, if you come up with a mozmill test, we (the drivers) will consider the patch. Our code freeze is in 10-14 days, and we're really trying to ramp down changes and risk, so I can't promise anything. But the patch is pretty straightforward, I agree.

Comment 18

[Jonathan Protzenko [:protz]]

Attachment: Same patch as before, but with mozmill tests

David, could you possibly take a look at this? I added a Mozmill test. It was kind of hard to reproduce the issue without being able to do the write/reply sequence myself. I'm injecting pre-built messages with the right In-Reply-To headers, and I need to move them between folders to demonstrate the issue.

I haven't found a better way to test for correct summarization than this:
+  assert_true(mmDoc.getElementById("heading").textContent.indexOf("2 conversations") < 0,
+    "This thread was wrongly summarized as multiple conversations.");

I didn't find any function to test for thread summarization instead of multiple threads summarization. Even when in multiple-threads-summary, if I call assert_messages_summarized with all the messages, it returns true even though it only displays one message for each thread (I guess that's intended).

I can quickly fix any remarks regarding the Mozmill test in order to get this ready for rc1, if you feel so :).

Comment 19

[Jonathan Protzenko [:protz]]

Attachment: Same patch as before, but with mozmill tests and a cleaner test that does not rely on a string

Here's a cleaner test:

  assert_true(mmDoc.getElementsByClassName("count").length === 0,
    "This thread was wrongly summarized as multiple conversations.");

The summarization code adds, when in multiple-thread-summary mode, a (possibly empty) <div class="count"> for each thread it summarizes. When in single-thread-summary mode, these <div>s are not added, so that allows us to quickly test whether we are in the right mode.

Comment 20

[Jonathan Protzenko [:protz]]

Following discussion with dmose on IRC, I'm nominating for blocking-rc1 because while I believe this to be a minor issue, I still think this can be very confusing for users.
- I frequently happened to switch email addresses in the middle of a conversation (e.g. I get a message on my personal address and I want to use my professional one instead), and since I use "Unified Inbox" all the time, it's quite strange to see one thread in the folder display and two threads in the multi message pane.
- Moreover, I think we expect more and more users to use unified folders, and since this will be the "big migration release", we might as well try to remove those small glitches to ensure a smoother experience.

On the technical side, this is quite a simple patch that I've been testing on my computer for months. I haven't experienced any issues. Plus, I managed to come up with a test that clearly demonstrates the issue, and it fills a gap in the mozmill tests (I remember asuth writing in one of the comments that we should clearly test for multi-thread summarization vs. single-thread summarization). So that'd be high benefit.

The second part of the bug still needs some change on the C++ side which I'm unable to perform right now. We can leave that for later, though.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #20)
> Following discussion with dmose on IRC, I'm nominating for blocking-rc1 because
> while I believe this to be a minor issue, I still think this can be very
> confusing for users.

Thanks for the nomination.  After looking more closely, I think this probably doesn't actually block.  That said, after it gets review, feel free to nominate for approval.  If it doesn't make 3.1, we could certainly consider it for 3.1.x.

> - Moreover, I think we expect more and more users to use unified folders, and
> since this will be the "big migration release", we might as well try to remove
> those small glitches to ensure a smoother experience.

One of the main reasons this doesn't block is because, unlike in 3.0, Unified folders are not the default in 3.1.

Comment 22

[David :Bienvenu]

Comment on attachment 443589 [details] [diff] [review]
Same patch as before, but with mozmill tests and a cleaner test that does not rely on a string

thx, this test works. It's way too late for 3.1, but we can get this into the trunk so that it'll be in 3.2. Please pester me to add the backend support we need to fix this completely...

Comment 23

[Ludovic Hirlimann [:Usul]]

(In reply to comment #22)
> (From update of attachment 443589 [details] [diff] [review])
> thx, this test works. It's way too late for 3.1, but we can get this into the
> trunk so that it'll be in 3.2. Please pester me to add the backend support we
> need to fix this completely...

What's the best way to do that david ?

Comment 24

[David :Bienvenu]

File a bug along the lines of #c15 and assign it to me.

Comment 25

[Jonathan Protzenko [:protz]]

Please cc me on the followup bug if you can.

Comment 26

[Ludovic Hirlimann [:Usul]]

Bug 572094

Comment 27

[Mark Banner (:standard8)]

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

Comment 28

[Jonathan Protzenko [:protz]]

Comment on attachment 443589 [details] [diff] [review]
Same patch as before, but with mozmill tests and a cleaner test that does not rely on a string

While we're at it, we might as well fix this on 3.1.1 :).

Comment 29

[Mark Banner (:standard8)]

We had to back out the test part of this due to unit test failures as a result of interactions with other tests:

http://hg.mozilla.org/comm-central/rev/9ca8aa343127

Jonathan is going to fix this up next week. I'm happy to leave the code in trunk as we can let it bake a bit. Reopening, so that we remember there's stuff to finish off on this bug.

Comment 30

[Mark Banner (:standard8)]

Unfortunately, I had to back out the code changes as well:

http://hg.mozilla.org/comm-central/rev/301d8d13af5c

It appears that the change was causing random crashes when test-summarization.js was being run during the mozmill tests.

I saw the crash on Mac and Windows, so its not just one platform. Typical log:

http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1276625001.1276627536.23625.gz#err2

I'll attach the full record of the crash in a moment.

Comment 31

[Mark Banner (:standard8)]

Attachment: Crash Log (mac)

Interestingly this has morkRowObject::CloseRowObject at the top of its location, which I think we've had as crashes before... hopefully David can remember.

Comment 32

[Mark Banner (:standard8)]

Attachment: Crash Log (windows)

I'm attaching the windows log as well, as I think it gives slightly more information - in particular that we're in nsMsgThread when we're in between the cycle collection code and the mork cleanup code.

Comment 33

[David :Bienvenu]

The crash is because we have a thread object whose lifetime is exceeding the life of the db - the temporary thread objects created here:

+    let firstThreadId = dbView.getThreadContainingIndex(selectedIndices[0])

and here:

+      let threadId = dbView.getThreadContainingIndex(selectedIndices[i])

So this is basically the js garbage collection problem. Up until now, js code hasn't tried to use the thread objects much. I don't have an easy answer for fixing this - the db could keep track of thread objects it has handed out and clear them when the db goes away, as we do for msg hdrs. Or the thread object could be a db listener. Or the thread object could stop caching the mork row, though that would have a bit of a perf impact. The first one is the right thing to do, I believe.

Or we could expose a method on the view that allows the summarization code to ask the question it wants w/o going through the thread object. What would that question be, exactly, protz? boolean AreMsgHdrsInSameThread(in nsIMsgDBHdr aHdr1, in nsIMsgDBHdr aHdr2)?

Comment 34

[Jonathan Protzenko [:protz]]

AreMsgHdrsInSameThread is ok to determine whether we're dealing with a single thread or multiple threads, but remember the second part of the patch (the one I couldn't fix): we need to group messages by view thread, and using the function you suggest would probably result in inefficient JS with possibly bad complexity.

Do you think you could expose a

Array Array msgHdr GroupMsgHdrsByViewThread (in nsIMsgDBHdr Array)

?

That would solve the issue in a clean way and would allow for an efficient implementation on the C++ side, because you'll know the view thread ids.

Comment 35

[David :Bienvenu]

Attachment: fix thread gc issues - checked in.

protz, if you want to try this patch, it should fix the crash. I've reworked the db thread handling to keep track of extant db thread objects, which might actually have some memory footprint/perf wins.

Comment 36

[David :Bienvenu]

(In reply to comment #34)

> That would solve the issue in a clean way and would allow for an efficient
> implementation on the C++ side, because you'll know the view thread ids.

The view thread objects don't have the crashing issue, and I believe I have a reasonable fix for the crash issue in any case. So I'm inclined to fix the core code.

Comment 37

[Mark Banner (:standard8)]

Comment on attachment 443589 [details] [diff] [review]
Same patch as before, but with mozmill tests and a cleaner test that does not rely on a string

Cancelling approval request whilst we sort out the issues on this bug (which I think need protz to respond to David).

Comment 38

[Jonathan Protzenko [:protz]]

David,

With bug 572094 now fixed, and your patch that addresses the JS collection issue, I think I have everything needed to come up with a new patch. So let's forget what I said about GroupMsgHdrsByViewThread, I think it's fine right now :). Thanks for fixing the core code.

Patch should be available in the next few days.

Comment 39

[David :Bienvenu]

Comment on attachment 451768 [details] [diff] [review]
fix thread gc issues - checked in.

this should stop the test case from its occasional Tinderbox failure. It could also help out with various issues that may arise if we handed out multiple nsMsgThread objects for the same thread.

Comment 40

[Jonathan Protzenko [:protz]]

Attachment: New updated patch (test + fixes)

David, here's a brand new patch.

I tested this on 1.9.2. I backported your patch from 572094 on 1.9.2 (add getViewThreadForMsgHdr), applied the one in attachment 451768 [details] [diff] [review] (avoid mork corruption issues), and applied the one I'm attaching right now, and it all seems to work fine.

It was a bit of a struggle with the Makefile system, but I'm pretty confident it's alright now =).

This patch is twofold:
- it makes sure that the code in messageDisplay.js uses view threads
- it makes sure that the code in selectionSummaries.js groups threads according to view thread keys when creating a multi message summary

I entirely rewrote the test to make it cleaner and more robust. The test is commented, and now tests for the two points above.

The patch from bug 572094 has already been applied on trunk, but the one for the database corruption (attachment 451768 [details] [diff] [review]) issues was not designed for trunk, and so the patch failed to apply (that's why I used comm-1.9.2 initially). However, I think I managed to fix it properly, so I'll be attaching another patch for review to save you the hassle of doing it yourself, since I've done it already.

Comment 41

[Jonathan Protzenko [:protz]]

Attachment: fix thread GC issues (for c-c)

Here's the updated patch for c-c.

Concerning that thread gc issue, I haven't been able to reproduce it on my mac at momo, so I guess we'll just have to hope that my new test will also reproduce the same testing conditions as the previous patch.

Comment 42

[Mark Banner (:standard8)]

Comment on attachment 451768 [details] [diff] [review]
fix thread gc issues - checked in.

r/sr=Standard8, although I actually tested protz's non-bitrotted version of the patch.

Comment 43

[David :Bienvenu]

Comment on attachment 463393 [details] [diff] [review]
New updated patch (test + fixes)

thx for the patch, some nits about the comments:

For comments, we tend to either use
/**
 *
 */

or 
//
//


makes sure messageDisplay.js understand there's

should be "understands"

Comment 44

[Jonathan Protzenko [:protz]]

Attachment: Comment nits taken into account, ready for checkin

Carrying :bienvenu's r+ on this patch (only comments were changed).

Comment 45

[Jonathan Protzenko [:protz]]

Attachment: Comment nits taken into account, ready for checkin

Same thing but with the proper Mqueue formatting so that the author and commit message are ready.

Comment 46

[Mark Banner (:standard8)]

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