648584 - assertion removing the last message from a threaded unread only view

Status: NEW

(MailNews Core :: Backend, defect)

Description

[David :Bienvenu]

If I view unread only threaded, and delete the last message in the view, I get an assertion about writing to an invalid nsTArray index, in nsMsgThreadedDBView::RemoveByIndex. It's easy enough to fix the assertion, but the code isn't quite right in general so I should fix that as well.

In Thunderbird, it's easy enough to get in this situation by clicking the unread widget in the quick filter bar, while in threaded mode.

Comment 1

[David :Bienvenu]

Attachment: proposed fix

This fixes the issue by looking a the headers of the expanded thread in unread only view, and promoting the first child, if any, to level 0, and if there are two or more children, marking the first child as a thread with children. This fixes the assertion, and fixes some edge cases where not all the messages in the thread are unread.

Comment 2

[[e-mail]]

Sorry, I'm not sure what the STR are, when you say the last message in the view, does this mean there is only one message in the view, and if so is it a collapsed thread, or does it mean it is the bottommost message in the view, and if so is it a collapsed thread or is it the bottommost message of an expanded thread?

Comment 3

[David :Bienvenu]

bottommost message in the view, in an expanded thread. In TB, you can use the quick filter bar to see this:

mark both messages in a folder & thread as unread.
In threaded mode, quick on the "unread" button in the quick filter bar.

Delete the top most message, then the next message.

I think you can do the same thing in SM with view | messages, unread instead of clicking the quick filter bar.

Comment 4

[[e-mail]]

(In reply to comment #3)
> bottommost message in the view, in an expanded thread. In TB, you can use the
> quick filter bar to see this:
> 
> mark both messages in a folder & thread as unread.
> In threaded mode, quick on the "unread" button in the quick filter bar.
> 
> Delete the top most message, then the next message.
> 
> I think you can do the same thing in SM with view | messages, unread instead of
> clicking the quick filter bar.
Sorry, I was unable to reproduce the assertion in SeaMonkey either with view | messages | unread or view | threads | unread. I had one unread thread in the folder, and I tried deleting the messages starting from the bottom, and I tried deleting the messages starting from the top.

Comment 5

[David :Bienvenu]

(In reply to comment #4)

> Sorry, I was unable to reproduce the assertion in SeaMonkey either with view |
> messages | unread or view | threads | unread. I had one unread thread in the
> folder, and I tried deleting the messages starting from the bottom, and I tried
> deleting the messages starting from the top.

I couldn't get SM to trigger the assertion either, using the various view options. I'll see if I can find any fancier way to get SM to execute this code.

Comment 6

[Wayne Mery (:wsmwk)]

Do you see this assertion?

Comment 7

[Thomas D. (:thomas8)]

Assuming that these are the STR:

1) in random (local) folder, have two messages belonging to same thread
2) mark both unread (M), and defocus to ensure that they don't get auto-marked-read while you're testing (or set auto-mark-read-delay higher)
3) switch to threaded view (from column header thread column header)
4) open error console, clear it
5) Show quick filter, and then (quickly):
- click on unread filter button on qfb (to show only unread)
- select first msg, press [del]
- press [del] again to delete 2nd msg (so that msg pane is now empty)
6) recheck error console

wfm with those steps for me on WinXP/ TB17/ TB Trunk:

Actual Result = Expected Result
- msgs correctly deleted (and recoverable by undo)
- no assertion found in error console (stays blank, with view on ALL errors)

Comment 8

[:aceman]

Not sure. While trying to reproduce I get this in the console:
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520015: file /var/SSD/TB-hg/mailnews/local/src/nsMsgBrkMBoxStore.cpp, line 902
JavaScript error: chrome://messenger/content/mailWindowOverlay.js, line 3037: NS_ERROR_FILE_ACCESS_DENIED: Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIMsgFolder.markMessagesRead]
This happens when the last message changes from unread to read. Not sure if that is related. But I couldn't see any "assertion".

Comment 9

[Wayne Mery (:wsmwk)]

rsx, can you also verify the assertion no longer happens in SM?

I wonder if the patch is regardless still needed.

Comment 10

[Wayne Mery (:wsmwk)]

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

removing review request pending further investigation

Comment 11

[rsx11m]

I'm not sure if I applied the STR correctly, but I don't see this issue or any assertions in SeaMonkey (neither Windows nor Linux) either. It has to be kept in mind though that the qfb implementation of SM is different than the TB one, thus may not trigger the problem.

I'd read "the code isn't quite right in general so I should fix that as well" that it might be worth revisiting the patch anyway, despite the lack of an assertion, given that "something" is apparently wrong with the way how the code currently is written in that function (and the patch would also simplify the existing code).

The patch in attachment 524707 [details] [diff] [review] has slightly bitrotted but still applies after accounting for bug 579517 (s/PRUint32/uint32_t) and bug 776630 (s/nsnull/nullptr).