808220 - [SMS API] Retrieve a 'sent' message show field 'sender' as 'undefined' STRING, not undefined

Status: RESOLVED FIXED

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

Description

[Dave Townsend [:mossop]]

This used to work but since I updated today I can no longer delete sms messages. I hit edit, then select all, then press delete and nothing happens.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Gregor, could the DB upgrade have affected this?

Comment 2

[Gregor Wagner [:gwagner]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #1)
> Gregor, could the DB upgrade have affected this?

I can't see how the DB upgrade is related to this but I will try to reproduce.
It works for me on unagi with mc and gaia tip.

Comment 3

[Gregor Wagner [:gwagner]]

I tried with an old DB that triggers the upgrade code but I can still delete the messages afterwards on current mc and gaia tip.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I just deleted a few SMSes about jury duty from my dogfooding unagi that went through the upgrade.  I probably broke the law, but the messages are gone.

Comment 5

[David Scravaglieri [:scravag]]

WORKFORME

Comment 6

[Faramarz Rashed]

just ran into this testing it with cjones :-)  sorry david... must be some race condition...

Comment 7

[Alex Keybl [:akeybl]]

Milestoning for C2 (deadline of 12/10), as this meets the criteria of "known P2 bugs found before or during C1".

Comment 8

[Borja Salguero [:borjasalguero]]

It's working for me (Today BUILD, latest in Gaia, Unagi device). Could you check it again?

Comment 9

[Gregor Wagner [:gwagner]]

(In reply to Borja Salguero [:borjasalguero] from comment #8)
> It's working for me (Today BUILD, latest in Gaia, Unagi device). Could you
> check it again?

This is an intermittent bug :( We should try to get a logcat when it happens again.

Comment 10

[mbarone]

Tested with the Unagi build
    * Gecko-67437dc
    * Gaia-92b7a95

and works fine for me. I was able to delete two different messages from differente conversations.

Comment 11

[mbarone]

In order to reproduce the bugs is useful add the TIP of the build. If this information is not provided developers and Qa can't reproduce the same environment.

Comment 12

[Dave Townsend [:mossop]]

(In reply to mbarone from comment #11)
> In order to reproduce the bugs is useful add the TIP of the build. If this
> information is not provided developers and Qa can't reproduce the same
> environment.

Can you explain what a TIP is?

Comment 13

[Borja Salguero [:borjasalguero]]

[:jsmith] This is working properly. Nothing changes in SMS from one week and a half ago. Could you recheck again? 

For me it's working. Build 11/22, Otoro, Latest Gaia.

Comment 14

[Borja Salguero [:borjasalguero]]

We have changed due to we have discovered what it's happening. The bug is in Gecko, when sending a SMS. Scenario:
- Send a SMS to phone number 123123123
- 'message.sender' is null

+ EXPECTED: SMS DB should store null/undefined

+ CURRENTLY: SMS DB is storing "undefined" as a STRING. So when SMS App is checking the following line:
var num =  message.sender || message.receiver;
SENDER is not undefined (is a STRING), so we are not rendering properly the thread.

+ SOLUTION: Store undefined properly in DB

Comment 15

[Dylan Oliver [:doliver]]

Raised priority to P1 since this is blocking a smoketest blocker (bug 815470)

Comment 16

[Gregor Wagner [:gwagner]]

I haven't looked at the special problem here but from reading comment 14 I remember having similar problems with contacts. This is an XPCOM/IDL problem. When you have a callback function defined in IDL like foo(in DOMString arg1) and you call it with foo(null), the argument in the callback function will hold 'undefined'.

I know it's ugly but the right fix is a === 'undefined' check.

Comment 17

[[:fabrice] Fabrice Desré]

Gregor is right, I checked that we store |undefined| in the DB when sending the SMS.

Borja, can you fix that on the gaia side with the === 'undefined' hack?

Comment 18

[[:fabrice] Fabrice Desré]

So, Borja fixed this one in bug 815596. I don't know if we'll have a fix for the xpconnect underlying issue in time for v1 though, so I'm tempted to close this one.

Comment 19

[Borja Salguero [:borjasalguero]]

I fixed one, but due to this bug is open we are having a lot of issues in SMS App:
https://bugzilla.mozilla.org/show_bug.cgi?id=815470

This bug should be fixed asap in order to make our SMS App (or future SMS Apps) work properly! So we cant  close this one because is not fixed.

Comment 20

[[:fabrice] Fabrice Desré]

I will try to fast track fixing the gecko bug, but you should not wait on that.

Comment 21

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Test

I've done this quick modification of the already existing SMS xpcshell tests just to confirm the xpconnect issue mentioned above.

If I am not wrong, a quick temporary solution would be passing 'null' instead of 'undefined' while creating the SMS objects.

Comment 22

[[:fabrice] Fabrice Desré]

Attachment: patch

Workaround in the sms db implementation. The root issue is in the RIL though. Sigh...

Comment 24

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b625f3f1fe5

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5b625f3f1fe5

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/21165724c1d0
https://hg.mozilla.org/releases/mozilla-beta/rev/d078592049c7