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...

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.

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.

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

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?

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 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 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?

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.

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); } }

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.

(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.

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?

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?

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.

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?

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.

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 :).

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.

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.

(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 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...

(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 ?

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

Please cc me on the followup bug if you can.

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

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 :).

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.

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.

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

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.

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)?

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.

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.

(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 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).

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 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.

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.

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 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 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"

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

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

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