Closed Bug 983215 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

defect
Not set
normal

Tracking

(blocking-b2g:1.4+, firefox28 wontfix, firefox29 wontfix, firefox30 fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.4 S4 (28mar)
blocking-b2g 1.4+
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: drs, Assigned: edgar)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 947140
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
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.
Summary: [DSDS] SMS doesn't respect given serviceId → [RIL][DSDS] SMS doesn't respect given serviceId
blocking-b2g: --- → 1.4?
Flags: needinfo?(btseng)
1.4+ blocks a user story
blocking-b2g: 1.4? → 1.4+
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
Assignee: nobody → echen
Flags: needinfo?(btseng)
Summary: [RIL][DSDS] SMS doesn't respect given serviceId → [RIL][DSDS] Sending SMS/MMS doesn't respect given serviceId
Attached patch Patch, v1Splinter Review
Comment on attachment 8391069 [details] [diff] [review]
Patch, v1

Hi Gene, could you help to review this? Thank you.
Attachment #8391069 - Flags: review?(gene.lian)
Comment on attachment 8391069 [details] [diff] [review]
Patch, v1

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

My bad. Shame on me. :(
Attachment #8391069 - Flags: review?(gene.lian) → review+
Blocks: 983673
Depends on: 854326
Duplicate of this bug: 983673
I don't see any difficulty here that we have to file an extra bug for test cases.
(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.
Already has a patch, so I'm removing testcase-wanted.
Keywords: testcase-wanted
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.
Attachment #8391421 - Attachment is obsolete: true
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/f965cc60b604
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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.
Target Milestone: --- → 1.4 S4 (28mar)
You need to log in before you can comment on or make changes to this bug.