983215 - [RIL][DSDS] Sending SMS/MMS doesn't respect given serviceId

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Doug Sherk (:drs) (inactive)]

I haven't dug further than this, but from the SMS app:

this._mozMobileMessage.send(..., {serviceId: 1})

Doesn't override the SIM to send the message from, instead always sending it from the first.

It does respect the ril.sms.defaultServiceId setting, though.

Comment 1

[Julien Wajsberg [:julienw]]

For information, here is the code used in the SMS app: [1].

We might have a string instead of a long, I don't know how WebIDL behaves in that case. I'll try things locally.

[1] https://github.com/julienw/gaia/blob/63865a3ed07817176539450b82386e2bdd106755/apps/sms/js/message_manager.js#L491-L496

Comment 2

[Julien Wajsberg [:julienw]]

I tried forcing either 0, 1, 2, "1" or anything, it always sent using my configured default SIM "0". So either I'm not using the API correctly, or there is a bug.

Comment 3

[Joe Cheng [:jcheng] (please needinfo)]

1.4+ blocks a user story

Comment 4

[Gene Lian (I already quit Mozilla)]

It should be working. Please see [1].

[1] http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/MobileMessageManager.cpp#198

Comment 5

[Edgar Chen [:edgar]]

We suspect there is an logic error in MobileMessageManager [1].
Patch is on the way.

[1] https://github.com/EdgarChen/mozilla-central/blob/119f4a0bdbdf3e4a8145b04d28d5aec578052a5e/dom/mobilemessage/src/MobileMessageManager.cpp#L198-L207

Comment 6

[Edgar Chen [:edgar]]

Attachment: Patch, v1

Comment 7

[Edgar Chen [:edgar]]

Comment on attachment 8391069 [details] [diff] [review]
Patch, v1

Hi Gene, could you help to review this? Thank you.

Comment 8

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8391069 [details] [diff] [review]
Patch, v1

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

My bad. Shame on me. :(

Comment 9

[Edgar Chen [:edgar]]

Try result: https://tbpl.mozilla.org/?tree=Try&rev=9e40cc03bc10

Comment 11

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

I don't see any difficulty here that we have to file an extra bug for test cases.

Comment 12

[Edgar Chen [:edgar]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #11)
> I don't see any difficulty here that we have to file an extra bug for test
> cases.

Hi Vicamo,

My original idea is to land the fix patch ASAP because it blocks gaia development.
So can we land the fix part first? And I will keep working on test case part.

Thank you.

Comment 13

[Jason Smith [:jsmith]]

Already has a patch, so I'm removing testcase-wanted.

Comment 14

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

Attachment: part 2/2: test cases : WIP

Comment 15

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

Comment on attachment 8391421 [details] [diff] [review]
part 2/2: test cases : WIP

Obsoleting this because now it does raise a problem in emulator -- SMS sent to 2nd modem doesn't loop back.

Comment 16

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

(In reply to Jason Smith [:jsmith] from comment #13)
> Already has a patch, so I'm removing testcase-wanted.

I thought v1.4 is about stability.  Yet another prove that it is not.

Comment 17

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/rev/f965cc60b604

Comment 18

[Julien Wajsberg [:julienw]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #16)
> (In reply to Jason Smith [:jsmith] from comment #13)
> > Already has a patch, so I'm removing testcase-wanted.
> 
> I thought v1.4 is about stability.  Yet another prove that it is not.

I don't think you're talking about the same "testcase" meaning.
The keyword "testcase-wanted" means that we want a minimal web page to show the bug.
What you cant, Vicamo, is unit tests, and I fully agree with you. But I think there is a misunderstanding here.

FWIW it doesn't block Gaia development, we could do the development just fine using mocks. It blocks the final testing though.

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f965cc60b604

Comment 20

[Anthony Ricaud (:rik)]

Although it's really nice to have a fix so quickly, we shouldn't have landed it without tests. The Gaia team can compile Gecko with this patch applied if it is really blocking development. Also, writing the tests often helps you check that the fix is the correct one. And finally, given the amount of different configurations that we now ship or plan to ship, there is no way for us developers to manually test on every device before landing so we need to write new tests to get our automation to do that for us.