Closed Bug 879225 Opened 8 years ago Closed 8 years ago

B2G MMS: When mms APN is not shared with default data connection, mmsc/mmsproxy host routes aren't set.

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24
blocking-b2g leo+
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: ctai, Assigned: vicamo)

References

Details

(Whiteboard: [fixed-in-birch])

Attachments

(1 file, 1 obsolete file)

When mms APN is not
shared with default data connection, mmsc/mmsproxy host routes aren't
set because of possible mismatching format like 10.1.1.1, or
http://mmsc:8080. The first case is ignored and the second triggers an
exception. So when mms and default data connection are the same one,
traffic to mmsproxy will be directed to wrong network interface.
This is a follow-up fix for leo+ bug 875390. SO I nominate it for leo+. It will cause sending/receiving MMS fail in some sim card with data connection on.
No longer blocks: 875390
blocking-b2g: --- → leo?
Depends on: 875390
Attached patch patch (obsolete) — Splinter Review
Attachment #758385 - Flags: review?(gene.lian)
Attachment #758385 - Flags: feedback?(ctai)
Comment on attachment 758385 [details] [diff] [review]
patch

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

::: dom/system/gonk/NetworkManager.js
@@ +228,5 @@
>  
>    ppmm.addMessageListener('NetworkInterfaceList:ListInterface', this);
> +
> +  // Used in resolveHostname().
> +  defineLazyRegExp(this, "REGEXP_IPV4", "^\\d{1,3}(?:\\.\\d{1,3}){3}$");

Should we also support IPV6?
Attachment #758385 - Flags: feedback?(ctai) → feedback+
Comment on attachment 758385 [details] [diff] [review]
patch

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

::: dom/system/gonk/NetworkManager.js
@@ +32,5 @@
>                                     "nsIDNSService");
>  
> +XPCOMUtils.defineLazyServiceGetter(this, "gIoService",
> +                                   "@mozilla.org/network/io-service;1",
> +                                   "nsIIOService");

You don't really need this. Please see below.

@@ +136,5 @@
>  
> +function defineLazyRegExp(obj, name, pattern) {
> +  obj.__defineGetter__(name, function() {
> +    delete obj[name];
> +    return obj[name] = new RegExp(pattern);

I don't really get it. Why do you need to delete the property and create a new RegExp object for it whenever looking up the property?

Shouldn't it be:

if (obj[name] === undefined) {
  obj[name] = new RegExp(pattern);
}
return obj[name];

@@ +626,5 @@
>      let retval = [];
>  
> +    for (let hostname of hosts) {
> +      try {
> +        let uri = gIoService.newURI(hostname, null, null);

Nice clean-up! But I think you can directly use:

Services.io.newURI(hostname, null, null);

since the |Services.jsm| has already imported in the NetworkManager.js.

@@ +636,5 @@
>          continue;
>        }
>  
> +      try {
> +        let hostnameIps = gDNSService.resolve(hostname, 0);

It's fine to wrap this in a try block. Just curious about why need to do so.
(In reply to Gene Lian [:gene] from comment #4)
> @@ +136,5 @@
> >  
> > +function defineLazyRegExp(obj, name, pattern) {
> > +  obj.__defineGetter__(name, function() {
> > +    delete obj[name];
> > +    return obj[name] = new RegExp(pattern);
> 
> I don't really get it. Why do you need to delete the property and create a
> new RegExp object for it whenever looking up the property?
> 
> Shouldn't it be:
> 
> if (obj[name] === undefined) {
>   obj[name] = new RegExp(pattern);
> }
> return obj[name];

I'm wrong. This is really an advanced coding skill. Good to learn. :)
Comment on attachment 758385 [details] [diff] [review]
patch

r=me after using Services.io.newURI(...). Thanks!
Attachment #758385 - Flags: review?(gene.lian) → review+
(In reply to Gene Lian [:gene] from comment #4)
> Review of attachment 758385 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/NetworkManager.js
> @@ +636,5 @@
> > +      try {
> > +        let hostnameIps = gDNSService.resolve(hostname, 0);
> 
> It's fine to wrap this in a try block. Just curious about why need to do so.

Feeding anything |gDNSService.resolve| don't recognize results in an exception thrown.  In order not to affect following operations, wrapping with try-catch here seems nice to me.
Attached patch patch v2Splinter Review
1) add IPv6 matching.  Android libnetutils does support IPv6 host routing.
2) use |Services.io| instead.
Attachment #758385 - Attachment is obsolete: true
Attachment #758453 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/afb71a02b5c5
Assignee: nobody → vyang
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
blocking-b2g: leo? → leo+
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.