Closed Bug 945647 Opened 6 years ago Closed 6 years ago

[DSDS][MMS] When change primary outgoing data SIM during MMS attachment downloading, it should stop downloading process.

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 verified, b2g-v1.4 fixed)

VERIFIED FIXED
1.3 C3/1.4 S3(31jan)
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- verified
b2g-v1.4 --- fixed

People

(Reporter: echu, Assigned: airpingu)

References

Details

(Whiteboard: [dsds_US_test] [FT:RIL])

Attachments

(3 files)

When press download button of a MMS and before attachment is loaded, switch primary outgoing data should also stop the process.

* Build Number                
Fugu
Gaia:     124f6b7105b86fc3f7a11aaf0a354abc93b36047
Gecko:    d3d3413a417302268344e1be2be03cfcaefd2ff0
BuildID   20131203063021
Version   28.0a1

* Reproduce Steps
1. Send a MMS to SIM 1 which is primary outgoing data.
2. Open the message and press download button, message shows downloading.
3. Go to SIM manager to change primary outgoing data to SIM 2. 
4. Back to the message in step 2. 

* Expected Result
Downloading is stopped.

* Actual Result
It still shows downloading.

* Occurrence rate
100%
Whiteboard: [dsds_US_test]
Hey Gene,

What is Gecko doing in this case?
* Is it doing the expected result? In that case, it should send an error on this request.
* Is it still downloading? In that case, maybe the behavior expected by QA is wrong?
Component: Gaia::SMS → RIL
Flags: needinfo?(gene.lian)
(In reply to Julien Wajsberg [:julienw] from comment #1)
> Hey Gene,
> 
> What is Gecko doing in this case?
> * Is it doing the expected result? In that case, it should send an error on
> this request.
> * Is it still downloading? In that case, maybe the behavior expected by QA
> is wrong?

Gecko doesn't properly handle the case of switching the primary SIM when the MMS is still downloading. In this case, we should stop the download and immediately return the request.error.
Assignee: nobody → gene.lian
Blocks: b2g-mms
Flags: needinfo?(gene.lian)
Hardware: x86_64 → ARM
blocking-b2g: --- → 1.3?
Gene, I don't understand something. I think the error is broader than only this case.

When we switch the SIM, the old network interface should be disabled, right? Therefore, we should forcibly close all associated active connections, and therefore the error would be automatically sent.

What do you think?
DSDS defect to be fixed in 1.3.
blocking-b2g: 1.3? → 1.3+
Whiteboard: [dsds_US_test] → [dsds_US_test] [FT:RIL]
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
Attached patch PatchSplinter Review
Attachment #8365760 - Flags: review?(vyang)
Attachment #8365760 - Flags: review?(btseng)
Comment on attachment 8365760 [details] [diff] [review]
Patch

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

The patch looks okay to me to fix the problem of changing SIM while there is some MMS transaction ongoing.

However, shall we also cover the question mentioned by julien in comment#3 or file another bug to cover it?

It will be beter that MmsService provides at least a default error cause if the mms connection was disconnected unexpectedly
by invoking flushPendingCallbacks() as followed with a newly-defined reason instead of doing nothing:
MmsConnection::observe():
        .....
        this.connected = connected;
        if (!this.connected) {
          flushPendingCallbacks(_HTTP_STATUS_CONNECTION_LOST);   <-- Here
          return;
        }
        ....
And mapping this error cause to INTERNAL_ERROR or something else.

What is your opinion? :)
Attachment #8365760 - Flags: review?(btseng) → review+
Hi Enpei, I wondered Bug 948854 can cover this bug. Could you please test this issue again? Thanks!
Depends on: 948854
Flags: needinfo?(echu)
Keywords: qawanted
Hi Gene,

Latest fugu still has the problem.
Fugu
Gaia      1318d1612299ba5d86820bbb6a65ae090f3b2fd6
Gecko     af909ff1dce5d69871146f4b44d13b2c877f9bca
BuildID   20140127061647
Version   28.0a2
Flags: needinfo?(echu)
Keywords: qawanted
Comment on attachment 8365760 [details] [diff] [review]
Patch

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

I think Bevis has his point.  The data connections should have been disconnected and the transaction should have been cancelled.  We can have this as a backup solution since the clock is ticking, but still want to know the root cause.
Attachment #8365760 - Flags: review?(vyang)
Comment on attachment 8365760 [details] [diff] [review]
Patch

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

Gene has find out the root cause.  Basically MMS connection is never turned off when "ril.data.defaultServiceId" changes in Gaia because it's actually controlled by "ril.mms.defaultServiceId" instead.  Under DSDA configuration, that's fine; but when it comes to DSDS, it's not something we would have expected.  Under DSDS, MMS connections should be turned off upon that settingschanged event when we have both 'mms' and 'data' connected no matter they're shared or not.

This makes mms-not-disconnected a separated bug, and leaving the patch here becoming really appropriate for the case.  Because in my opinion, MmsService should be the only one that knows what "dom.mms.defaultServiceId" really means, while NetworkManager knows all available network interfaces, and ConnectionManager knows the telephony related policies such as DSDA or DSDS that affect the availability of all RILNetworkInterface instances.

Thank you, Gene!
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #11)
> Gene has find out the root cause.  Basically MMS connection is never turned
> off when "ril.data.defaultServiceId" changes in Gaia because it's actually
> controlled by "ril.mms.defaultServiceId" instead.  Under DSDA configuration,
> that's fine; but when it comes to DSDS, it's not something we would have
> expected.  Under DSDS, MMS connections should be turned off upon that
> settingschanged event when we have both 'mms' and 'data' connected no matter
> they're shared or not.
> 
> This makes mms-not-disconnected a separated bug, and leaving the patch here
> becoming really appropriate for the case.  Because in my opinion, MmsService
> should be the only one that knows what "dom.mms.defaultServiceId" really
> means, while NetworkManager knows all available network interfaces, and
> ConnectionManager knows the telephony related policies such as DSDA or DSDS
> that affect the availability of all RILNetworkInterface instances.

Fire Bug 964708 as a follow-up.
(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #12)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #11)
> > Gene has find out the root cause.  Basically MMS connection is never turned
> > off when "ril.data.defaultServiceId" changes in Gaia because it's actually
> > controlled by "ril.mms.defaultServiceId" instead.  Under DSDA configuration,
> > that's fine; but when it comes to DSDS, it's not something we would have
> > expected.  Under DSDS, MMS connections should be turned off upon that
> > settingschanged event when we have both 'mms' and 'data' connected no matter
> > they're shared or not.
> > 
> > This makes mms-not-disconnected a separated bug, and leaving the patch here
> > becoming really appropriate for the case.  Because in my opinion, MmsService
> > should be the only one that knows what "dom.mms.defaultServiceId" really
> > means, while NetworkManager knows all available network interfaces, and
> > ConnectionManager knows the telephony related policies such as DSDA or DSDS
> > that affect the availability of all RILNetworkInterface instances.
> 
> Fire Bug 964708 as a follow-up.

Hmmm... RadioInterfaceLayer did deactivateDataCalls even for mms type, but not sure why mms connection hasn't been successfully cut in this situation. Seems we didn't really get the root cause :( More investigation is required.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js?from=RadioInterfaceLayer.js#941
Hmmm.. I found a flaw in our current design. This happens when data is disabled, it will enter the condition in [1] and return after setting the appropriate data registration for each sim. However, in my tests, after data registration is unregistered, we will receive a unsolicited data call state change indicating that the previous data call was disconnected, so the mms connection should still be disconnected.
Anyway, it is a bug that we should fix. Thank you!

[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js?from=RadioInterfaceLayer.js#925
(In reply to Jessica Jong [:jjong] [:jessica] from comment #15)
> Hmmm.. I found a flaw in our current design. This happens when data is
> disabled, it will enter the condition in [1] and return after setting the
> appropriate data registration for each sim. However, in my tests, after data
> registration is unregistered, we will receive a unsolicited data call state
> change indicating that the previous data call was disconnected, so the mms
> connection should still be disconnected.
> Anyway, it is a bug that we should fix. Thank you!
> 
> [1]
> http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/
> RadioInterfaceLayer.js?from=RadioInterfaceLayer.js#925

Oh, got it. Thanks for investigation, Jessica!
https://hg.mozilla.org/mozilla-central/rev/a0489a2299c5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Verified on Fugu.
Fugu
Gaia      77fdf058ccb77f40497fb696912a1ce12192eea7
Gecko     9c7f92ffaa19db4a004103104d2da78b8cbd2056
BuildID   20140129130749
Version   28.0a2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.