New messages sometimes not scrolled into view

RESOLVED FIXED in Instantbird 53

Status

defect
--
major
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: florian, Assigned: florian)

Tracking

trunk
Instantbird 53

Thunderbird Tracking Flags

(thunderbird_esr4551+ fixed, thunderbird51 fixed, thunderbird52 fixed, thunderbird53 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
This seems to be a regression from bug 1315479, but it actually just uncovered an existing bug. If we interrupt the displayPendingMessages loop before getNextPendingMessage returned null, the _pendingMessages array isn't cleaned up and getPendingMessagesCount returns a bogus value at the next call.
(Assignee)

Updated

2 years ago
Blocks: 1315479
(Assignee)

Comment 1

2 years ago
Posted patch FixSplinter Review
Attachment #8812626 - Flags: review?(aleth)

Comment 2

2 years ago
Comment on attachment 8812626 [details] [diff] [review]
Fix

Review of attachment 8812626 [details] [diff] [review]:
-----------------------------------------------------------------

So that means this bug happens whenever we run out of messages to display just as we run out of time for the current cycle.

I suppose the original idea behind this was to be more generous about letting the array keep growing before scrolling to the end.

The real problem here seems to be that there's two counters, this._nextPendingMessageIndex and this._pendingMessagesDisplayed, one being internal to the stuff that can be overridden.
Attachment #8812626 - Flags: review?(aleth) → review+

Comment 3

2 years ago
https://hg.mozilla.org/comm-central/rev/d4e762ef561de865b6e95befb61beafaf914866d
Bug 1318974 - Cleanup the _pendingMessages array even if getNextPendingMessage never returns null, r=aleth. a=aleth CLOSED TREE

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 53

Comment 4

2 years ago
We should probably uplift this!
(Assignee)

Comment 5

2 years ago
(In reply to aleth [:aleth] from comment #4)
> We should probably uplift this!

We can for the sake of correctness, but we backed bug 1315479 out of 52, so the bug is likely invisible there. I think it hardly ever manifests itself when loading old messages, and we are very unlikely to have more than 40 new incoming messages to display at once.

Comment 6

2 years ago
Comment on attachment 8812626 [details] [diff] [review]
Fix

[Approval Request Comment]
User impact if declined: Fixes a relatively rare but annoying edge case
Testing completed (on c-c, etc.): yes
Attachment #8812626 - Flags: approval-comm-esr45?
Attachment #8812626 - Flags: approval-comm-beta?
Attachment #8812626 - Flags: approval-comm-aurora?

Updated

2 years ago
Attachment #8812626 - Flags: approval-comm-beta?
Attachment #8812626 - Flags: approval-comm-beta+
Attachment #8812626 - Flags: approval-comm-aurora?
Attachment #8812626 - Flags: approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.