946942 - [MMS] MO/MT MMS fail if the MMS APN is shared with default APN

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Anshul]

On AT&T networks, the default and MMS calls are shared. So when the MmsService starts the call is already connected. Since MmsService.js only seems to read the APN settings using nsIRilNetworkInterface when the "network-interface-state-changed" event is received, which in this case will not happen as the data call was already up, the MmsService doesn't get the correct MMS APN settings and therefor MO/MT MMS fail.

Comment 1

[Anshul]

Patch upcoming shortly.

Comment 2

[Anshul]

Attachment: bug_946942.patch

Comment 3

[Anshul]

Attachment: bug_946942.patch

Comment 4

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8343362 [details] [diff] [review]
bug_946942.patch

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

Thanks Anshul for finding this. :) Indeed, the initialization is wrong under the case of shared APN. If you don't mind, I'd suggest writing the codes to be:

// If the MMS network is connected during the initialization, it means the
// MMS network must share the same APN with the mobile network by default.
// Under this case, |networkManager.active| should keep the mobile network,
// which is supposed be an instance of |nsIRilNetworkInterface| for sure.
if (this.connected) {
  let networkManager = Cc["@mozilla.org/network/manager;1"]
                         .getService(Ci.nsINetworkManager);

  let activeNetwork = networkManager.active;
  if (!(activeNetwork instanceof Ci.nsIRilNetworkInterface)) {
    return;
  }

  let rilNetwork = activeNetwork.QueryInterface(Ci.nsIRilNetworkInterface);
  if (rilNetwork.serviceId != this.serviceId) {
    return;
  }

  // Set up the MMS APN setting based on the connected MMS network,
  // which is going to be used for the HTTP requests later.
  this.setApnSetting(rilNetwork);
}

to make it more clear that we can only access |serviceId| and call |setApnSetting(...)| when the network is an instance of nsIRilNetworkInterface.

If it looks good to you, please apply my revision and directly go ahead to land with r=gene (not r=<gene>). Thanks!

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +248,5 @@
>  
>      this.connected = this.radioInterface.getDataCallStateByType("mms") ==
>        Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED;
> +    if (this.connected) {
> +      let networkManager = 

nit: trailing white space.

Comment 5

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

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #4)
>   let networkManager = Cc["@mozilla.org/network/manager;1"]
>                          .getService(Ci.nsINetworkManager);
>   let activeNetwork = networkManager.active;
>   if (!(activeNetwork instanceof Ci.nsIRilNetworkInterface)) {
>     return;
>   }
>   let rilNetwork = activeNetwork.QueryInterface(Ci.nsIRilNetworkInterface);
>   if (rilNetwork.serviceId != this.serviceId) {
>     return;
>   }

These have to go some day we're going to support DSDA.  The best solution seems to be bug 928861, but we'll only land it in 1.4.  So please also have some comments addressing the assumption that in DSDS active network must be the same interface with the mms one.

Comment 6

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8343361 [details] [diff] [review]
bug_946942.patch

Already gave review+ for the second patch.

Comment 7

[Anshul]

Attachment: bug_946942.patch

Comment 8

[Anshul]

Attachment: bug_946942.patch

Comment 9

[Anshul]

Comment on attachment 8343865 [details] [diff] [review]
bug_946942.patch

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

Taking forward previous approval from gene as per her request.

Comment 10

[Anshul]

Attachment: bug_946942.patch

Removed whitespaces and rebased to the tip.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 13

[Gene Lian (I already quit Mozilla)]

The checked-in doesn't follow comment #4. :O

Comment 14

[Gene Lian (I already quit Mozilla)]

OK. After some thoughts, letting |QueryInterface(Ci.nsIRilNetworkInterface)| throws exception might be better if the active network is supposed to be a RIL network interface.

Comment 15

[Gene Lian (I already quit Mozilla)]

Attachment: A follow-up to align to comment #4

Comment 16

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8344449 [details] [diff] [review]
A follow-up to align to comment #4

I'd like to apply this follow-up. The service ID is only valid after we're sure the network is a nsIRilNetworkInterface one. Does that work for you?

Comment 17

[Gene Lian (I already quit Mozilla)]

Just worrying about the codes are mistakenly using |.serviceId| to check the availability of RIL network interface. For example, the |.serviceId| of wifi network interface will be undefined. If so, we'd better use |activeNetwork instanceof Ci.nsIRilNetworkInterface| as suggested at comment #4.

Comment 18

[Anshul]

Comment on attachment 8344449 [details] [diff] [review]
A follow-up to align to comment #4

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

Sorry about that Gene.

Comment 19

[Gene Lian (I already quit Mozilla)]

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

This is a follow-up patch to polish the previous one. I'm not sure if V1.3 is already branched or not. If yes, we need to uplift this follow-up patch as well.

Comment 20

[Gene Lian (I already quit Mozilla)]

As mentioned at comment #19, to make sure I didn't do anything wrong during the recent V1.3 branching, I'd like to needinfo Ryan to keep track of this. Thanks!

Comment 21

[Ryan VanderMeulen [:RyanVM]]

1.3 is already branched. Comment 19 will need to be uplifted to Aurora. Setting status-firefox28 to affected so I'll find it.

Comment 22

[Gene Lian (I already quit Mozilla)]

Awesome! Thanks Ryan!

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ee493ea80c8f

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9624db84c496