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

Status: VERIFIED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Enpei]

Attachment: time stamp: 17:52, 12/03.

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%

Comment 1

[Julien Wajsberg [:julienw]]

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?

Comment 2

[Gene Lian (I already quit Mozilla)]

(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.

Comment 3

[Julien Wajsberg [:julienw]]

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?

Comment 4

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

DSDS defect to be fixed in 1.3.

Comment 5

[Gene Lian (I already quit Mozilla)]

Attachment: Patch

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

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? :)

Comment 7

[Gene Lian (I already quit Mozilla)]

Hi Enpei, I wondered Bug 948854 can cover this bug. Could you please test this issue again? Thanks!

Comment 8

[Enpei]

Hi Gene,

Latest fugu still has the problem.
Fugu
Gaia      1318d1612299ba5d86820bbb6a65ae090f3b2fd6
Gecko     af909ff1dce5d69871146f4b44d13b2c877f9bca
BuildID   20140127061647
Version   28.0a2

Comment 9

[Enpei]

Attachment: time stamp: 17:23, 1/27

Comment 10

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

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.

Comment 11

[Vicamo Yang [:vicamo][: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!

Comment 12

[Gene Lian (I already quit Mozilla)]

(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.

Comment 13

[Gene Lian (I already quit Mozilla)]

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

Comment 14

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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

Comment 15

[Jessica Jong [:jessica] (inactive)]

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

Comment 16

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0596865af40e

Comment 19

[Enpei]

Verified on Fugu.
Fugu
Gaia      77fdf058ccb77f40497fb696912a1ce12192eea7
Gecko     9c7f92ffaa19db4a004103104d2da78b8cbd2056
BuildID   20140129130749
Version   28.0a2