Chia-hung Tai [:ctai :ctai_mozilla :cht] Reporter
	Description • 11 years ago

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.

	Chia-hung Tai [:ctai :ctai_mozilla :cht] Reporter
	Comment 1 • 11 years ago

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.

	Chia-hung Tai [:ctai :ctai_mozilla :cht] Reporter
	Comment 3 • 11 years ago

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?

	Gene Lian (I already quit Mozilla)
	Comment 4 • 11 years ago

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.

	Gene Lian (I already quit Mozilla)
	Comment 5 • 11 years ago

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

	Gene Lian (I already quit Mozilla)
	Comment 6 • 11 years ago

Comment on attachment 758385 [details] [diff] [review]
patch

r=me after using Services.io.newURI(...). Thanks!

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 7 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 8 • 11 years ago

1) add IPv6 matching.  Android libnetutils does support IPv6 host routing.
2) use |Services.io| instead.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 9 • 11 years ago

https://hg.mozilla.org/projects/birch/rev/afb71a02b5c5

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 11 years ago

https://hg.mozilla.org/releases/mozilla-b2g18/rev/7b5b8b85cc94

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 11 years ago

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/7b5b8b85cc94