931348 - B2G MMS: Cannot send MMS after resetting Message settings (even without any changes)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect, P1)

Description

[Gene Lian (I already quit Mozilla)]

This bug sounds really critical.

STR:

0. Use the latest master codes on 10/25 for both Gecko and Gaia.
1. Open Messaging App to send MMS (repeat this step; always successful).
2. Open Settings App -> Cellular & Data -> Message settings -> Tap on OK (don't do any change).
3. Back to Messaging App to send MMS again -> KO.

I assume this is a regression since I haven't been aware of this issue.

Comment 1

[Gene Lian (I already quit Mozilla)]

> 3. Back to Messaging App to send MMS again -> KO.

Note that the symptom is the sending circle will keep spinning for a long time.

Comment 2

[Steve Chung [:steveck]]

Using build released on 10/20 seems ok. I will try it with latest build again.

Comment 3

[Gene Lian (I already quit Mozilla)]

Note that you have to do step #2 within 30 seconds after successfully sending an MMS at step #1, because Gecko will internally keep the MMS connection for 30 seconds for the next attempt. So, if you wait for the disconnection and do the resetting, it won't cause issues.

Comment 4

[Sarah Parsons]

This issue started to occur on the Buri Master Build ID: 20131025100746

Gaia   afbf45f26a73b7cd5e0a831bea48087331975286
SourceStamp 2f2a45f04e7c
BuildID 20131025100746
Version 27.0a1

Last working Buri Master Build ID: 20131024154023

Gaia   2feebdb1a2583928f32407d76d798f8654621e2b
SourceStamp 19fd3388c372
BuildID 20131024154023
Version 27.0a1

Comment 5

[Gene Lian (I already quit Mozilla)]

Hi Ken, I'd appreciate if you could find someone look into this? I'm wondering there is something wrong with the APN settings. We haven't changed MMS logic during this time period.

Comment 6

[Ken Chang[:kenkai]]

Jessica, we need your help...:)

Comment 7

[Gene Lian (I already quit Mozilla)]

Thanks Jessica very much! Please let me know if you need to reproduce this issue.

Comment 8

[Jessica Jong [:jessica] (inactive)]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #7)
> Thanks Jessica very much! Please let me know if you need to reproduce this
> issue.

Sure! I was not able to reproduce it with my buri, either my phone is too laggy or my hands are not quick enough.
Will ask for your help when I look into this bug. Thank you.

Comment 9

[Jessica Jong [:jessica] (inactive)]

From my observations today, when this issue happens we can find the following in the logs:

10-29 16:47:15.639 I/Gecko   (  144): -@- MmsService: xhr done, but status = 0, statusText = 
10-29 16:47:15.639 I/Gecko   (  144): -@- MmsService: Fail to send. Will retry after: 300000

However, I think the root cause is: when ril.data.apnSettings key is set (no matter values changed or not), RIL will clear all data connections and reconstruct |apnSettings|. The new |apnSettings| does not contain some cached old values and callbacks, so it will fail to notify of the "network-interface-state-changed" events. In this case, MmsService though mms connection was still there so it tried to send the message without setting up mms data connection.

We need to think of a way to handle apn settings change properly when there are active connections.

Comment 10

[Gene Lian (I already quit Mozilla)]

Interesting! Sounds like not a regression, though. Thanks for finding out the root cause! It's very valuable. We need to work out a better mechanism to handle this case.

Comment 11

[Jessica Jong [:jessica] (inactive)]

A quick and easy way to fix this, since we are enhancing NetworkManager/RILNetworkInterface in the near future, is to send out explicitly a "network-interface-state-changed" event when disconnecting all active data connections due to ril.data.apnSettings change. This way, MmsService will call getDataCallStateByType("mms"), and it will know that mms connection is not in CONNECTED state.

Comment 12

[Gene Lian (I already quit Mozilla)]

Sounds good to me. However, I'm curious about why the original |deactivateDataCallByType(...)| in |updateApnSettings(...)| doesn't trigger the "network-interface-state-changed"?

In MmsService.js, we also call deactivateDataCallByType(...) to disconnect the MMS connection and can update the |this.connected| via "network-interface-state-changed".

Comment 13

[Jessica Jong [:jessica] (inactive)]

In a normal case, the flow is:

1. (RadioInterface)      deactivateDataCallByType(...) -> 
2. (RILNetworkInterface) disconnect(...) -> 
3. (RadioInterface)      deactivateDataCall(...) ->
4. (ril_worker)          deactivateDataCall(...), here it will send back to RadioInterface a "datacallstatechange" 
                                                  message with state DISCONNECTING . ->
5. (RadioInterface)      handleDataCallState(...) ->
6. (RadioInterface)      _deliverDataCallCallback(...), here it will find all the datacall callback registrants and 
                                                        trigger the callback, RILNetworkInterface(s) should be one of 
                                                        the registrants. ->
7. (RILNetworkInterface) dataCallStateChanged(...), here is the place where it is supposed to notify the 
                                                   "network-interface-state-changed" event.


In the case of updateApnSettings(...), RILNetworkInterface's datacall callback is first unregistered and then RILNetworkInterface is deleted  [1], so in Step 6 above, no callback to RILNetworkInterface(s), which are recreated by then (RILNetworkInterface(s) will register to datacall callbacks on |connect(...)| [2]).


Another thing to note is that after data deactivation is done, state is set to UNKNOWN, and no "network-interface-state-changed" event is notified for this state [3]. Currently, it does not seems to impact anything, but I think is kind of odd.

Please let me know if there is anything unclear or wrong. Thanks.


[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#1555
[2] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#3483
[3] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#3429

Comment 14

[Jessica Jong [:jessica] (inactive)]

Oops! Sorry for the alignment...

Comment 15

[Gene Lian (I already quit Mozilla)]

This one might be a regression.

Comment 16

[Jason Smith [:jsmith]]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #15)
> This one might be a regression.

comment 4 already proves this is a regression.

Comment 17

[Jessica Jong [:jessica] (inactive)]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #15)
> This one might be a regression.

Do you mean regression caused by Bug 931699?
I tried to reproduce this issue today now that Bug 931699 has been backed-out, however, I can't send mms at all with the latest build :(.
The following is found in the logs:

I/Gecko   (  441): [Child 441] WARNING: NS_ENSURE_TRUE(txToRemove) failed: file /home/jessica/hg/src/docshell/shistory/src/nsSHistory.cpp, line 1292
E/GeckoConsole(  441): [JavaScript Error: "ReferenceError: SMIL is not defined" {file: "app://sms.gaiamobile.org/js/message_manager.js" line: 478}]
I/Gecko   (  441): [Child 441] WARNING: There is no observer for "invalidformsubmit". One should be implemented!: file /home/jessica/hg/src/content/html/content/src/HTMLFormElement.cpp, line 1854
E/GeckoConsole(  141): [JavaScript Error: "Return value is not an object."]
E/GeckoConsole(  141): [JavaScript Error: "NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [inIDOMUtils.setContentState]" {file: "chrome://global/content/BrowserElementPanning.js" line: 465}]

Comment 18

[Gene Lian (I already quit Mozilla)]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #17)

> E/GeckoConsole(  441): [JavaScript Error: "ReferenceError: SMIL is not
> defined" {file: "app://sms.gaiamobile.org/js/message_manager.js" line: 478}]

This one is a Gaia issue (regression).

Btw, I was wondering this bug might not be a regression because the APN resetting issue should been there for a long time since we didn't change much logic on that. Anyway, we have to fix it.

Comment 19

[Jessica Jong [:jessica] (inactive)]

Attachment: proposed patch.

We set state to DISCONNECTING and notify observers on behalf of RILNetworkInterface when data is deactivated due to ril.data.apnSettings change.

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

Comment 20

[Kai-Chih Hu [:khu]]

Comms team, can you help to triage this one?

Comment 21

[Gene Lian (I already quit Mozilla)]

Comment on attachment 827322 [details] [diff] [review]
proposed patch.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1593,5 @@
> +
> +          // If this iface is going down, notify observers on befalf of it.
> +          // These RILNetworkInterface(s) will not be able to send out the
> +          // notifications since they are going to be recreated.
> +          if (!apnSetting.iface.connectedTypes.length) {

Why not using .inConnectedTypes(type) to notify observers for a certain type only? Just double checking.

Also, I wonder the comment you left doesn't stand. Is it sill possible fire the notification in RILNetworkInterface.disconnect(...)? Also, why the notification firing in RILNetworkInterface.dataCallStateChanged(...) doesn't work?

Btw, I think we need to fix some MMS logic as well since the MMS connection can now early disconnected. For example, we need to call disconnectTimer.cancel() whenever the this.connected is disconnected, and don't need to set up disconnectTimer if this.connected is already disconnected. I can help you with this part or we can fire another bug fixing that.

Comment 22

[Jessica Jong [:jessica] (inactive)]

Thanks for the review. Please see my response below.

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #21)
> Comment on attachment 827322 [details] [diff] [review]
> proposed patch.
> 
> Review of attachment 827322 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1593,5 @@
> > +
> > +          // If this iface is going down, notify observers on befalf of it.
> > +          // These RILNetworkInterface(s) will not be able to send out the
> > +          // notifications since they are going to be recreated.
> > +          if (!apnSetting.iface.connectedTypes.length) {
> 
> Why not using .inConnectedTypes(type) to notify observers for a certain type
> only? Just double checking.

A RILNetworkInterface can be shared by different types. If you disconnect a certain type and the RILNetworkInterface is still used by another type, a "network-interface-state-changed" will be sent in [1], so we need to send the notification only when RILNetworkInterface is not used by any type and is really going down, which is when apnSetting.iface.connectedTypes.length reaches zero.

> 
> Also, I wonder the comment you left doesn't stand. Is it sill possible fire
> the notification in RILNetworkInterface.disconnect(...)? 

Yes. I have just discussed with Ken about this. The original way was to set the GECKO_NETWORK_STATE_XXXX in ril_worker, and notification was fired in RILNetworkInterface.dataCallStateChanged(...), this is because in most of the cases ril_worker gets the correct state from modem. But for CONNECTING/DISCONNECTING state, we can set the state in RILNetworkInterface.connect(...)/disconnect(...) and fire the notifications first.
I will upload another patch for this.


> Also, why the
> notification firing in RILNetworkInterface.dataCallStateChanged(...) doesn't
> work?

RILNetworkInterfaces were recreated and does not register to data call callbacks until RILNetworkInterface.connect(...) is called. Even if it does register, it will not be able to handle the datacallstatechange due to info mismatch. Please refer to the flow in Comment 13.

> 
> Btw, I think we need to fix some MMS logic as well since the MMS connection
> can now early disconnected. For example, we need to call
> disconnectTimer.cancel() whenever the this.connected is disconnected, and
> don't need to set up disconnectTimer if this.connected is already
> disconnected. I can help you with this part or we can fire another bug
> fixing that.

Sure. I'd prefer to fire another bug for this.

Please let me know if there is anything unclear, we can discuss f2f. :)

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#3262

Comment 23

[Gene Lian (I already quit Mozilla)]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #22)
> > Also, why the
> > notification firing in RILNetworkInterface.dataCallStateChanged(...) doesn't
> > work?
> 
> RILNetworkInterfaces were recreated and does not register to data call
> callbacks until RILNetworkInterface.connect(...) is called. Even if it does
> register, it will not be able to handle the datacallstatechange due to info
> mismatch. Please refer to the flow in Comment 13.

Oh! I missed that bit. You mentioned that before. Sorry! :)

Comment 24

[Julien Wajsberg [:julienw]]

Joe, see comment 20, I think Kevin wants you to triage this.

Comment 25

[Jessica Jong [:jessica] (inactive)]

Attachment: proposed patch, v2.

The following changes are included in this patch:

1. when we receive a "ril.data.apnSettings" change, we compare the new apns with the existing apns and reset the apnSettings only if apn/user/password has changed. The compare result is not always correct due to the shared APN nature. But in the worst case, data connections are deactivated and activated again.

2. we moved the "network-interface-state-changed" notifications firing into NetworkManager. NetworkManager fires the notification after "network-interface-registered"/"network-interface-unregistered" and when all the routes (default and extra) and DNSes are all set.

3. additionally, we have added a updateNetworkInterface(...) function which just updates the routes (default and extra) and DNSes and does not re-register the network interface to avoid sending extra "network-interface-state-changed" notifications.


There seem to be lots of duplicated jobs in NetworkManager.registerNetworkInterface(...), "network-interface-state-changed" and "network-interface-registered" observer. Maybe we can merge these two events when doing NetworkManager enhancement?

Comment 26

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

(In reply to Jessica Jong [:jjong] [:jessica] from comment #25)
> Created attachment 828574 [details] [diff] [review]
> proposed patch, v2.

Per offline discussion, we're going to:

1) obsolete compareApnSettings function because it doesn't really belong to this bug.  We all know the imperfection of APN settings handler and it really deserves a complete rewrite.

2) Since we're transferring the responsibility of sending TOPIC_INTERFACE_STATE_CHANGED events to NetworkManager, I suppose there won't be TOPIC_INTERFACE_STATE_CHANGED event registration in NetworkManager any more because registering an event that is emitted by oneself makes really no sense.  We can also remove TOPIC_INTERFACE_REGISTERED/TOPIC_INTERFACE_UNREGISTERED event registration with the same reason.

3) The necessity of TOPIC_INTERFACE_REGISTERED/TOPIC_INTERFACE_UNREGISTERED can be another topic.  We can remove them if there is just no one interests in, but merging them into TOPIC_INTERFACE_STATE_CHANGED sounds like a mix of different meanings -- the existence of a certain interface and the variance of the attributes of it.  Really don't feel like it. :(

Comment 27

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 1: idl changes

Comment 28

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 2: ril changes

Comment 29

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 3: wifi changes

Comment 30

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 4: implementation

As stated in Comment 26, we are removing TOPIC_INTERFACE_STATE_CHANGED, TOPIC_INTERFACE_REGISTERED and TOPIC_INTERFACE_UNREGISTERED event registration from NetworkManager.
NetworkManager will provide a new method "updateNetworkInterface(...)" for updating routes and dnses, and will fire a TOPIC_INTERFACE_STATE_CHANGED notification when done.
However, the original issue still persists, since in updateApnSettings(...), only unregisterNetworkInterface(...) is called and not updateNetworkInterface(...).

Comment 31

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

1.3+ for regression

Comment 32

[Jessica Jong [:jessica] (inactive)]

Actually, this is not a regression, the code flow has been the same for a long time, so resetting the keyword.

Comment 33

[Jessica Jong [:jessica] (inactive)]

After rebasing the WIP patches I did find something really weird.
We have moved the notification firing into NetworkManager, so we can remove the TOPIC_INTERFACE_STATE_CHANGED observer events. However, once I removed the TOPIC_INTERFACE_STATE_CHANGED observer event ([1]~[2]), mms can not be sent on the first time, but it can always be sent out on the second time, either retrying automatically or manually. I have checked the routes and settings and they all seem to be right. Still investigating the cause of this...


[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/NetworkManager.js#188
[2] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/NetworkManager.js#257

Comment 34

[Jason Smith [:jsmith]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #32)
> Actually, this is not a regression, the code flow has been the same for a
> long time, so resetting the keyword.

Umm...why? There's a regression range associated with this bug in comment 4. So what is that regression range pointing at then?

Comment 35

[Jessica Jong [:jessica] (inactive)]

(In reply to Jason Smith [:jsmith] from comment #34)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #32)
> > Actually, this is not a regression, the code flow has been the same for a
> > long time, so resetting the keyword.
> 
> Umm...why? There's a regression range associated with this bug in comment 4.
> So what is that regression range pointing at then?

Mmm... I think maybe it's just the reproduction steps, note that step 2 must be done within 30 seconds.
I was able to reproduce it using 20131024154023.

Comment 36

[Paul Yang [:pyang] (away)]

It is still called regression, not because of no code change.  It breaks the test case(maybe slightly different) and the user story committed before.

Comment 37

[Jason Smith [:jsmith]]

Alright, let's verify this then.

Can someone check if this reproduces on 1.2 or 1.1?

Comment 38

[Gene Lian (I already quit Mozilla)]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #35)
> Mmm... I think maybe it's just the reproduction steps, note that step 2 must
> be done within 30 seconds.
> I was able to reproduce it using 20131024154023.

+1. I believe this is not a regression. This issue should have been existing from the very first beginning because we've never been thinking of fixing this corner-edge issue (users seldom change the MMS APN settings when the MMS is still downloading; needless to say only advanced users would want to change the MMS APN settings).

Comment 39

[Jason Smith [:jsmith]]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #38)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #35)
> > Mmm... I think maybe it's just the reproduction steps, note that step 2 must
> > be done within 30 seconds.
> > I was able to reproduce it using 20131024154023.
> 
> +1. I believe this is not a regression. This issue should have been existing
> from the very first beginning because we've never been thinking of fixing
> this corner-edge issue (users seldom change the MMS APN settings when the
> MMS is still downloading; needless to say only advanced users would want to
> change the MMS APN settings).

Then what exactly is the regression range pointing at?

Sarah - Can you clarify what the regression range's STR is actually looking at? The above comments seem to imply that this reproduces on the last working build cited in that regression range.

I'm pulling the blocking+ flag for now since this is leaning towards not being a regression, although I really need to understand what that regression range is pointing at.

Comment 40

[Sarah Parsons]

I just tried again using the Buri Master Build ID 20131024154023 and was not able to reproduce the issue. I used the STR in comment 0. 

Gaia   2feebdb1a2583928f32407d76d798f8654621e2b
SourceStamp 19fd3388c372
BuildID 20131024154023
Version 27.0a1

However, I was able to repro on the Buri 1.3 Build ID: 20131025100746

Gaia   afbf45f26a73b7cd5e0a831bea48087331975286
SourceStamp 2f2a45f04e7c
BuildID 20131025100746
Version 27.0a1

Comment 41

[Jason Smith [:jsmith]]

Sarah - I need to know the exact STR you are using to assess that regression window. I don't need clarification of the regression window itself. The above discussion seems to imply that the STR being used for this regression window isn't right.

Comment 42

[Sarah Parsons]

STR:

1. Launch Messaging App.
2. Create, and send an MMS.
3. Open the Settings App.
4. Select Cellular & Data, then Message Settings.
5. Tap OK without making any changes to the Message Settings.
6. Go back to the Message App and send another MMS.

Comment 43

[Jason Smith [:jsmith]]

Sounds like the above analysis actually proves that this is a regression. What might be happening here is that the patch being fixed here isn't fixing the bug in question here.

Comment 44

[Gene Lian (I already quit Mozilla)]

Hmm, I think I might misunderstand something. Thanks QA very much for all the effort on this!

Comment 45

[Jessica Jong [:jessica] (inactive)]

Thanks to :mcmanus help we have found the cause of the issue mentioned in comment 33.

Comment 46

[Jessica Jong [:jessica] (inactive)]

Attachment: Sol 1 - Part 1: idl changes

Add updateNetworkInterface() api.

Comment 47

[Jessica Jong [:jessica] (inactive)]

Attachment: Sol 1 - Part 2: ril changes

Use updateNetworkInterface() in ril.

Comment 48

[Jessica Jong [:jessica] (inactive)]

Attachment: Sol 1 - Part 3: wifi changes

Use updateNetworkInterface() in wifi.

Comment 49

[Jessica Jong [:jessica] (inactive)]

Attachment: Sol 1 - Part 4.1: implementation

Implement updateNetworkInterface() in NetworkManager.js and remove self-fired event observers.

Comment 50

[Jessica Jong [:jessica] (inactive)]

Attachment: Sol 1 - Part 4.2: additional fix

fire network-interface-state-changed events in updateRILNetworkInterface() if needed.

Comment 51

[Jessica Jong [:jessica] (inactive)]

Attachment: Alternative solution

Defer setting up APN settings until all data calls are cleared, this way the 'network-interface-state-changed' events' would be fired properly.

Comment 52

[Jessica Jong [:jessica] (inactive)]

Attachment: Sol 2 - Part 1: support reason when deactivating data call

Comment 53

[Jessica Jong [:jessica] (inactive)]

Attachment: Sol 2 - Part 2: defer apn settings setup

Comment 54

[Jessica Jong [:jessica] (inactive)]

Hi Vicamo,

There are two proposed solutions.

The first one is the one that we have discussed before, moving all the 'network-interface-state-changed' events firing to NetworkManager.js, it's just that we still need the Sol 2 - Part 4.1, calling updateNetworkInterface() when registering/unregistering network interface, to be able to fix the original issue.

The seconds one is simpler, that is, to defer the apn settings setup when there is any active data call, and wait until all data calls are cleared to setup the new apn settings.

I would prefer the second one since it will let data call deactivation finish properly. We can still move the 'network-interface-state-changed' events firing to NetworkManager.js, maybe in another bug?
Please let me know what is your opinion on this or any other suggestions are welcome! Thank you.

Comment 55

[Jean Gong :jgong]

PM triaged this bug and believes that it should be a blocker.

Comment 56

[Ken Chang[:kenkai]]

Interface changed.
Jessica, can you please discuss Vicamo directly?

Comment 57

[Anshul]

Joe, do we really care about this bug for 1.3? This issue is not blocking for us and neither for any customers on 1.3. Since we are getting so close to commercializing 1.3 any such interface changes pose risk to 1.3 quality.

Comment 58

[Anshul]

Hey Joe, I think I misread this bug. This seems like is for a single SIM and not DSDS, in which case this might need to be uplifted to 1.3.

Comment 59

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

Sorry, will have a review asap today :X

Comment 60

[Jason Smith [:jsmith]]

(In reply to Anshul from comment #57)
> Joe, do we really care about this bug for 1.3? This issue is not blocking
> for us and neither for any customers on 1.3. Since we are getting so close
> to commercializing 1.3 any such interface changes pose risk to 1.3 quality.

Absolutely disagree with this analysis. This is a bad regression that places the user in a situation where a MMS can be fail to be sent. It's also a confirmed regression, which is treated as a blocker by default unless it's minor.

Comment 61

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

Vicamo proposed Solution#3 -- we should de/activate data connection only if APN settings are actually updated. We shouldn't go deactivate connection only because we received 'ril.data.apnSettings.'

This solution benefits bug 962456 but we need to rewrite the whole updateApnSettings() and have one more cache.

Comment 62

[Jessica Jong [:jessica] (inactive)]

Thanks, Hsinyi and Vicamo.

I'd say that solution#3 does help bug 962456, but it does not solve the problem met in this bug entirely. It will be fine if user does not change the apn setting; but if user does change the apn setting, we still need to deactivate the corresponding data call, and this is where the problem we are trying to solve here comes.

Another thing is, solution#3 is really expensive for the current design, we have to compare each of the apns, then look for it in 'this.apnSettings.byType' to remove the reference, and look for it again in 'this.apnSettings.byApn' to remove the reference. That's why I put solution#3 in (or after) our 'data call enhancement', see bug 939046 item 4.

Please let me know if I am missing anything. Thanks.

Comment 63

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #59)
> Sorry, will have a review asap today :X

10 minutes.  Well, I define it's still "today" if I don't leave the office.

Comment 64

[Edgar Chen [:edgar]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #62)
> Thanks, Hsinyi and Vicamo.
> 
> I'd say that solution#3 does help bug 962456, but it does not solve the
> problem met in this bug entirely. It will be fine if user does not change
> the apn setting; but if user does change the apn setting, we still need to
> deactivate the corresponding data call, and this is where the problem we are
> trying to solve here comes.
> 
> Another thing is, solution#3 is really expensive for the current design, we
> have to compare each of the apns, then look for it in
> 'this.apnSettings.byType' to remove the reference, and look for it again in
> 'this.apnSettings.byApn' to remove the reference. That's why I put
> solution#3 in (or after) our 'data call enhancement', see bug 939046 item 4.

Hi there,

I have an idea about solution#3, how about we keep the logic of updateApnSettings() unchanged and only execute it when the new apns is NOT "exactly" the same as old one. For making the logic smarter, we can keep following up it in bug 939046.
I think above proposal still can help bug 962456, and also fix this bug for the case that user does not change the apn settings.

> 
> Please let me know if I am missing anything. Thanks.

Comment 65

[Jessica Jong [:jessica] (inactive)]

Vicamo proposed another solution#4: to use a command queue for commands that need to wait for another commands to be finished. I think it is a good solution and will benefit things like "deactivate data calls before turning radio off". However, it's not a small change, I doubt that we will able to land it for tomorrow, since there other 1.3+ bugs going on... :(

Comment 66

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

(In reply to Jessica Jong [:jjong] [:jessica] from comment #65)
> Vicamo proposed another solution#4: to use a command queue for commands that
> need to wait for another commands to be finished. I think it is a good
> solution and will benefit things like "deactivate data calls before turning
> radio off". However, it's not a small change, I doubt that we will able to
> land it for tomorrow, since there other 1.3+ bugs going on... :(

Yes, so let's take solution #2 here because it doesn't seem to affect QCRIL in anyway.

Comment 67

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: split updateApnSettings() into two parts.

Comment 68

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: defer apn settings setup.

Comment 69

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: defer setup of apn settings after data calls are cleared.

Comment 70

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

Comment on attachment 8366505 [details] [diff] [review]
Part 1: split updateApnSettings() into two parts.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2061,5 @@
>     *      corresponding APN setting.
>     *   4. Create RilNetworkInterface for each APN setting created at step 2.
>     */
> +  setupApnSettings: function(apnSettings) {
> +    if (!apnSettings) {

nit: let's keep the name 'simApnSettings' here so that we can reduce changed lines a bit.

Comment 71

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

Comment on attachment 8366509 [details] [diff] [review]
Part 2: defer setup of apn settings after data calls are cleared.

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

Thank you!

Comment 72

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: split updateApnSettings() into two parts, v2.

Address review comment 70:
- keep the parameter name 'simApnSettings'.

Comment 73

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: defer apn settings setup, v2.

rebased.

Comment 74

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: use allDataDisconnected() instead of anyDataConnected().

Sorry for finding this issue so late. To check that all data calls have been disconnected, we can not use 'anyDataConnected()', cause this function check for 'connected' data calls, but data calls may be already be in 'disconnecting' state. And since 'getDataCallByType()' is not really accurate (it won't return 'disconnecting' state'), we check for network interfaces directly in 'allDataDisconnected()'

Comment 75

[Jessica Jong [:jessica] (inactive)]

I have tested the following scenarios with part 3:
- On Buri, enable airplane mode with default and mms connected.
- On Buri, change apn settings with default and mms connected (this bug).
- On Fugu, switch default sim for data call with default and mms connected.
Not many times, but they seem to work fine.

Comment 76

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

Comment on attachment 8367111 [details] [diff] [review]
Part 3: use allDataDisconnected() instead of anyDataConnected().

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

Thanks.

Comment 77

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

(In reply to Hsin-Yi Tsai (OOO Jan. 29 ~ Feb. 5th) [:hsinyi] from comment #76)
> Comment on attachment 8367111 [details] [diff] [review]
> Part 3: use allDataDisconnected() instead of anyDataConnected().
> 
> Review of attachment 8367111 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks.

I'd say I am not a big fan of having allDataDisconnected() and anyDataConnected() both at the same time. However, we are facing a problem with the current ril network structure, i.e. RILNetworkInterface() is shared by types. The design leads to trouble in interface state changes.

And considering the timing pressure, it looks okay with this patch. And I do hope we could re-design the whole structure as soon as possible.

Anyway, big thank for Jessica's hard work :)

Comment 78

[Jessica Jong [:jessica] (inactive)]

Thanks, Vicamo and Hsinyi! :)

Try: https://tbpl.mozilla.org/?tree=Try&rev=ce6a530e61c3

Comment 79

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong (OOO Jan. 30 ~ Feb. 4) [:jjong] [:jessica] from comment #78)
> Thanks, Vicamo and Hsinyi! :)
> 
> Try: https://tbpl.mozilla.org/?tree=Try&rev=ce6a530e61c3

All green! \o/

Comment 80

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/875a264da3b7
https://hg.mozilla.org/integration/b2g-inbound/rev/e97ef5d5b644
https://hg.mozilla.org/integration/b2g-inbound/rev/e367fd549454

Comment 81

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: split updateApnSettings() into two parts (mozilla-aurora)

Comment 82

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: defer apn settings setup (mozilla-aurora)

Comment 83

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: use allDataDisconnected() instead of anyDataConnected() (mozilla-aurora)

Comment 84

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/875a264da3b7
https://hg.mozilla.org/mozilla-central/rev/e97ef5d5b644
https://hg.mozilla.org/mozilla-central/rev/e367fd549454

Comment 85

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/04b7ec6c71b7
https://hg.mozilla.org/releases/mozilla-aurora/rev/780f33e17613
https://hg.mozilla.org/releases/mozilla-aurora/rev/2df3c92b932d