814514 - B2G SMS: After deleting all messages of a thread, the thread is retrieved in 'getThreadList'

Status: RESOLVED WORKSFORME

(Core :: DOM: Device Interfaces, defect, P1)

Description

[Borja Salguero [:borjasalguero]]

Steps to reproduce:
- Send some messages to 'phone number A'
- 'getThreadList' return an item for 'phone number A'
- Delete ALL messages from/sent to 'phone number A'
- Call again 'getThreadList'

Expected:
- NO Thread for 'phone number A'

Currently:
- There is an entry for the thread 'phone number A' in the object store...

Comment 1

[Maria Angeles Oteo (:oteo)]

It's necessary to include if there are not messages exchanged in a thread, the thread must be removed.
It must be BB+ as it blocks Bug 808819, which is already BB+

Comment 2

[Andrew Overholt [:overholt]]

Are you working on this, Ben?

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This should work already, we have code to delete the entry in the thread database when all messages are deleted. It's possible that bug 808220 is to blame here too?

In any case I'm not actively working on this, we need some more investigation to see what's actually going wrong.

Comment 4

[Andrew Overholt [:overholt]]

Who can do the necessary investigation?

Comment 5

[Andrew Overholt [:overholt]]

Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.

Comment 6

[Borja Salguero [:borjasalguero]]

We have the Gaia patch ready, but we need resolve this one before making the PR. I've updated the milestone because It should be align with the bugs that this one is blocking.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Borja, does this still happen now that bug 808220 and bug 814163 are fixed?

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

Attachment: marionette test cases : WIP

I can't even access any attributes from the returned list elements. Strange!

Comment 9

[Andrew Overholt [:overholt]]

Jonas said this will be an easy fix and can get done in C3.  I'm lowering the severity since this seems like bad UX but not "critical".

Comment 10

[Dietrich Ayala (:dietrich)]

Hi Vicamo, no update for a while here. What's the status of this?

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

Still can't access attributes of returned array elements in attached test script. Is there any working code for getThreadList call?

Comment 12

[Borja Salguero [:borjasalguero]]

In Gaia Im working on it. We have just land Building Blocks, son this week this patch will be ready. Once I have the branch ready I will send you the code and we could work with this new branch & latest Gecko, checking if this bug persists.

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Borja Salguero [:borjasalguero] from comment #12)
> In Gaia Im working on it. We have just land Building Blocks, son this week
> this patch will be ready. Once I have the branch ready I will send you the
> code and we could work with this new branch & latest Gecko, checking if this
> bug persists.

Does this mean that we may not need a Gecko fix for this issue?

Comment 14

[Borja Salguero [:borjasalguero]]

It means that for checking this bug, we need a patch in Gaia aligned with Gecko code. Currently we are not using 'getThreadList' in Gaia due to this bug was there.

Comment 15

[Dietrich Ayala (:dietrich)]

Vicamo, what's the status? Borja, does your Gaia patch mentioned in comment #12 fix this, or are the platform changes required.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Guys, this bug is super confusing.

Can someone confirm that this is actually still actually happening. There is already code in gecko to make sure that a thread is removed from getThreadList when the last message in the thread is deleted.

Borja: Can you confirm that that isn't actually happening, and let us know which version of gecko you are using to test.


Vicamo: It *should* be possible to test this code even though there is no-one calling getThreadList. The code for removing messages is still called, and that is the code which is responsible for removing the entry from the objectStore backing getThreadList. So it should be possible to verify that things get removed as they should. The removal happens here:

http://mxr.mozilla.org/mozilla-central/source/dom/sms/src/ril/SmsDatabaseService.js#712

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Jonas Sicking (:sicking) from comment #16)
> Vicamo: It *should* be possible to test this code even though there is
> no-one calling getThreadList.

I know. My problem here is this code doesn't seem to work at all. I just found the jsval is array-typed in SmsRequest::NotifyThreadList(), but becomes an empty object in Marionette test script. Seems to be something wrong in xpconnect.

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Attachment: marionette test cases : WIP

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [debug]

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

Hi Bent, can you help review the original 'getThreadList' implementation again?

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #20)
> Hi Bent, can you help review the original 'getThreadList' implementation
> again?

Great! It's a bug that exists only in non-OOP and I've created bug 824467 for that. Thanks for Thinker :)

Besides, I've tested Borja's steps and no any thread entry remains after deleting all messages from conversation view with nightly Gecko & Gaia. I'll mark this bug as WORKSFORME if I get the same result with Gecko-b2g18 later.

Comment 22

[Vicamo Yang [:vicamo][:vyang]]

Verified again with: Gecko B2G18 - b27b0eadf48c, Gaia master - 21a7324