Closed Bug 946942 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
blocker

Tracking

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

RESOLVED FIXED
1.3 C1/1.4 S1(20dec)
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: anshulj, Assigned: anshulj)

References

Details

(Whiteboard: [caf priority: p1][CR 584928])

Attachments

(2 files, 4 obsolete files)

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.
Patch upcoming shortly.
Attached patch bug_946942.patch (obsolete) — Splinter Review
Attached patch bug_946942.patch (obsolete) — Splinter Review
Attachment #8343361 - Attachment is obsolete: true
Attachment #8343361 - Flags: review?(gene.lian)
Status: NEW → ASSIGNED
blocking-b2g: 1.3? → 1.3+
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.
Attachment #8343362 - Flags: review+
(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.
Component: General → RIL
Comment on attachment 8343361 [details] [diff] [review]
bug_946942.patch

Already gave review+ for the second patch.
Attachment #8343361 - Flags: review?(gene.lian)
Attached patch bug_946942.patch (obsolete) — Splinter Review
Attachment #8343362 - Attachment is obsolete: true
Attached patch bug_946942.patch (obsolete) — Splinter Review
Attachment #8343864 - Attachment is obsolete: true
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.
Attachment #8343865 - Flags: review+
Keywords: checkin-needed
Attached patch bug_946942.patchSplinter Review
Removed whitespaces and rebased to the tip.
Attachment #8343865 - Attachment is obsolete: true
Attachment #8343923 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/bcdcc7f548bf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
The checked-in doesn't follow comment #4. :O
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 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?
Attachment #8344449 - Flags: review+ → review?(anshulj)
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 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.
Attachment #8344449 - Flags: review?(anshulj) → review+
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.
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!
Flags: needinfo?(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.
Flags: needinfo?(ryanvm)
Awesome! Thanks Ryan!
Flags: in-moztrap?
Target Milestone: 1.3 Sprint 6 - 12/6 → 1.3 C1/1.4 S1(20dec)
Flags: in-moztrap? → in-moztrap-
Whiteboard: [CR 584928]
Whiteboard: [CR 584928] → [caf priority: p1][CR 584928]
You need to log in before you can comment on or make changes to this bug.