Closed Bug 814514 Opened 8 years ago Closed 8 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED WORKSFORME
B2G C3 (12dec-1jan)
blocking-basecamp +

People

(Reporter: borjasalguero, Assigned: vicamo)

References

Details

Attachments

(2 files, 1 obsolete file)

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...
Assignee: nobody → bent.mozilla
Blocks: 808819
blocking-basecamp: --- → ?
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+
blocking-basecamp: ? → +
blocking-kilimanjaro: --- → ?
Are you working on this, Ben?
blocking-kilimanjaro: ? → ---
Flags: needinfo?(bent.mozilla)
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.
Assignee: bent.mozilla → nobody
Flags: needinfo?(bent.mozilla)
Who can do the necessary investigation?
Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.
Priority: -- → P1
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.
Target Milestone: --- → B2G C2 (20nov-10dec)
Borja, does this still happen now that bug 808220 and bug 814163 are fixed?
Assignee: nobody → vyang
Summary: [SMS API] After deleting all messages of a thread, the thread is retrieved in 'getThreadList' → B2G SMS: After deleting all messages of a thread, the thread is retrieved in 'getThreadList'
Blocks: 806592
Attached patch marionette test cases : WIP (obsolete) — Splinter Review
I can't even access any attributes from the returned list elements. Strange!
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".
Severity: critical → normal
Target Milestone: B2G C2 (20nov-10dec) → B2G C3 (12dec-1jan)
Hi Vicamo, no update for a while here. What's the status of this?
Still can't access attributes of returned array elements in attached test script. Is there any working code for getThreadList call?
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.
(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?
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.
Vicamo, what's the status? Borja, does your Gaia patch mentioned in comment #12 fix this, or are the platform changes required.
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
(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.
Attachment #688805 - Attachment is obsolete: true
Attached patch [debug]Splinter Review
Hi Bent, can you help review the original 'getThreadList' implementation again?
Flags: needinfo?(bent.mozilla)
(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.
Flags: needinfo?(bent.mozilla)
Verified again with: Gecko B2G18 - b27b0eadf48c, Gaia master - 21a7324
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.