857964 - [SMS][MMS] Timing issue in ThreadListUI.renderThreads can end up in double entries in thread list

Status: RESOLVED DUPLICATE of bug 869799

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Jan Jongboom [:janjongboom] (inactive)]

ThreadListUI.renderThreads renders every item in the thread list that it receives on the next tick via a setTimeout. This probably has some historic reasons, but the side-effect is that when the method is called twice in a certain timeframe (length of a tick * list length) double entries end up in the thread list. You can see this behavior f.e. when immediately ponging back a message when sending one.

We need to stop the old render action when a new one comes in.

Comment 1

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch

Adds an internal ID that keeps track of the render actions that are running and thus has the ability to stop old render actions. This way there won't be any double entries in the thread list.

Comment 2

[Borja Salguero [:borjasalguero]]

Could you specify some steps to reproduce? Because the tests are working. Thanks!

Comment 3

[Jan Jongboom [:janjongboom] (inactive)]

Hi Borja, execute the following code in the console when running the app in FF Nightly:

MessageManager.getThreads(ThreadListUI.renderThreads);MessageManager.getThreads(ThreadListUI.renderThreads);

All threads are in the list now twice.

Comment 4

[Rick Waldron [:rwaldron]]

Where(In reply to janjongboom from comment #3)
> Hi Borja, execute the following code in the console when running the app in
> FF Nightly:
> 
> MessageManager.getThreads(ThreadListUI.renderThreads);MessageManager.
> getThreads(ThreadListUI.renderThreads);

Where does this happen in the existing code?

> 
> All threads are in the list now twice.

Comment 5

[Jan Jongboom [:janjongboom] (inactive)]

Rick, this was just to show the effect. This can happen if two events come in right after eachother (gets worse on a big list of SMSes).

Comment 6

[Jan Jongboom [:janjongboom] (inactive)]

Updated the PR to add tests.

Comment 7

[Jan Jongboom [:janjongboom] (inactive)]

Hey, this PR has been open for quite a while now, can either of you review this?

Comment 8

[Jan Jongboom [:janjongboom] (inactive)]

Nominating as tef? as I have managed to get this effect on my phone today (1.0.1) and it looks really really weird.

Comment 9

[Alex Keybl [:akeybl]]

(In reply to janjongboom from comment #8)
> Nominating as tef? as I have managed to get this effect on my phone today
> (1.0.1) and it looks really really weird.

No data loss, and this isn't a common issue. Please nominate the patch for approval to v1.1 once resolved.

Comment 10

[Borja Salguero [:borjasalguero]]

Comment on attachment 733233 [details] [diff] [review]
Patch

I would change to a boolean var instead. Please address the comment and ask to review again when ready. Thanks!

Comment 11

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 733233 [details] [diff] [review]
Patch

Borja, please see my comment on GH

Comment 12

[Corey Frang [:gnarf]]

I might of duped this @ bug 869799