1032097 - [B2G][MMS] Resolve/Add/Remove extra Host by MmsService per MMS connection.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

In current design, only the hosts listed in the APN settings will be resolved and added into the routing table by NetworkManager after the corresponding connection is established.

However, for MMS retrieval, it is possible to access a host specified in the URL of the mms-notification which is not related to the mmsc/mms proxy addresses added previously when MMS connection is established.

Fire this bug to address this problem.

Comment 1

[:gerard-majax]

Can't we just make use of NetworkService addHostRouteWithResolve in the MmsService ?

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

Update title per comment 1 and comment 41 in bug 1031656.

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

After further discussion off-lined, it will be a better design to:
1. Remove APIs of setExtraHostRoute()/removeExtraHostRoute() from NetworkManager.
   That means routing of MMS connection will no longer be handled by NetworkManager.
2. Provide new APIs in NetworkService to return Promise when setting/removing HostRoute with
   selected networkInterface.
   Note: these APIs will also help to resolve hostname if needed.
3. For MMS transactions, all the routing has to be decided by MmsService with the following rule:
   a. If MMS Proxy is available, setExtraHostRoute() for MMS Proxy.
   b. Otherwise, 
      - for outgoing MMS, setExtraHostRoute() for MMSC.
      - for retrieving MMS, setExtraHostRoute() for the host in the downloaded url.
   c. Remove routing before disconnecting MMS connection.
      (This is to prevent unexpected routing when shared APN is used.)

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

reset target milestone due to unexpected personal affairs last week.

Comment 5

[:gerard-majax]

Bevis, could you please make something for this ? We really need some patch that can be applied against v1.3 for partner maintenance release, otherwise a non neglictible part of the target audience will not be able to use MMS.

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Hi Lissy,

As triaged in Bug 1031656, this is not a 1.3+ bug.
However, I am still going to work on this from next week and 
trying to have the solution in master branch.

Bevis

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: WIP: Resolve/Add/Remove extra Host by MmsService per MMS connection.

Hi Jesscia,

Set |feedback?| to you to inform you this WIP patch for your reference.
Here is the summary of this patch:
1. In NetworkManager, 
   - deprecate |resolveHostname|, |setExtraHostRoute| and |removeExtraHostRoute|.
   - define new |addExtraHostRoute(type, serviceId, host)| and 
     |removeExtraHostRoute(type, serviceId, hosts)| for outer module to 
     set routing when needed.
2. In NetworkService,
   - define 2 new Promise APIs of |addExtraHostRoute(network, hosts)| and
     |removeExtraHostRoute(network, hosts)| used by NetworkManager.
3. In NetworkUtils,
   - post Dummy response to allow caller to way the async response of 
     addHostRoute/removeHostRoute.
4. In MmsService,
   - Apply the new APIs in NetworkManager to ensure routing before 
     transaction start.

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

After off-lined discussion, there is some redundant work to bug 1043114 regarding the change in
NetworkManager/Networkservice.
Hence, we'd like to refine NetworkManager first and apply new APIs from NetworkManager to fix this bug.

p.s. The WIP patch is already workable to fix this bug.

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

set dependency to bug 1043114 according to comment 8.

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng [:bevistseng] ([e-mail]) from comment #7)
> 2. In NetworkService,
>    - define 2 new Promise APIs of |addExtraHostRoute(network, hosts)| and
>      |removeExtraHostRoute(network, hosts)| used by NetworkManager.
One more thing, new NetworkService.resolveHostname() has applied DNSService.asyncResolve().

Comment 11

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8463832 [details] [diff] [review]
WIP: Resolve/Add/Remove extra Host by MmsService per MMS connection.

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

Bevis, thanks for the patches. As discussed offline, NetworkService's API will change to take single gateway and single hostname (resolved) as parameters.
I have referred part of your patches and uploaded my WIP patches in Bug 1043114. In Bug 1043114 we focus on the changes in NetworkService and its related implementation in NetworkUtils.
I am thinking adding the new API, the one that takes network type as parameter, in NetworkManager in this bug, what do you think?

BTW, the regex of IPv6 is not robust enough, it will only match one that is in the complete format, maybe we should use a more robust one.

Comment 12

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #11)
> Comment on attachment 8463832 [details] [diff] [review]
> WIP: Resolve/Add/Remove extra Host by MmsService per MMS connection.
> 
> Review of attachment 8463832 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Bevis, thanks for the patches. As discussed offline, NetworkService's API
> will change to take single gateway and single hostname (resolved) as
> parameters.
> I have referred part of your patches and uploaded my WIP patches in Bug
> 1043114. In Bug 1043114 we focus on the changes in NetworkService and its
> related implementation in NetworkUtils.
> I am thinking adding the new API, the one that takes network type as
> parameter, in NetworkManager in this bug, what do you think?
Sure, 
Mms seems to be the 1st caller of this API. I am okay to take it in this bug :D
> 
> BTW, the regex of IPv6 is not robust enough, it will only match one that is
> in the complete format, maybe we should use a more robust one.

Comment 13

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 1 v1: remove the unnecessary dependency of MOZ_B2G_RIL from the related Util APIs.

This is to move related utilities from the protection of MOZ_B2G_RIL.

Hi Edgar,

May I have your review for this change?

Thanks!

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2 v1: Refactor setHostRoutes/removeHostRoutes.

This is to refactor the common part of setHostRoutes/removeHostRoutes.

Hi Edgar,

May I have your review for this change?

Thanks!

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 3 v1: Replace setExtraHostRoute/removeExtraHostRoute with exposed APIs addExtraHostRoute/removeExtraHostRoute.

This is to replace setExtraHostRoute/removeExtraHostRoute with newly exposed APIs addExtraHostRoute/removeExtraHostRoute.

Hi Edgar,

May I have your review for this change?

Thanks!

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 4 v1: Adopt addExtraHostRoute/removeExtraHostRoute in MmsService.

This is to Adopt newly-exposed APIs addExtraHostRoute/removeExtraHostRoute in MmsService.

Hi Vicamo,

May I have your review for this change?

Thanks!

Comment 17

[Bevis Tseng[:bevis][:btseng](Exited)]

Further discussion is needed for how XMLHttpRequest/ProtocolProxyService send the request to the resolved ip:
1. replace hostname to the resolved ip before providing the url to XMLHttpRequest/ProtocolProxyService.
   (What if the url is meaningful in the server-side)
2. Can DNSCache be reused in this case?

Comment 18

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng [:bevistseng] ([e-mail]) from comment #17)
> Further discussion is needed for how XMLHttpRequest/ProtocolProxyService
> send the request to the resolved ip:
> 1. replace hostname to the resolved ip before providing the url to
> XMLHttpRequest/ProtocolProxyService.
>    (What if the url is meaningful in the server-side)
See VirtualHost as an example:
http://httpd.apache.org/docs/2.2/vhosts/examples.html

Comment 19

[Bevis Tseng[:bevis][:btseng](Exited)]

Per offline discussion,
replacing hostname to resolved ip is risky if service-side enables virtualhost.
DNS-related problems will be discussed in bug 992772 or in new bugs to follow up if needed.

This bug will be dedicated in add/remove routing per mms connection based on current design.

Comment 20

[Henry Chang [:hchang]]

(In reply to Bevis Tseng [:bevistseng] ([e-mail]) from comment #17)
> Further discussion is needed for how XMLHttpRequest/ProtocolProxyService
> send the request to the resolved ip:
> 1. replace hostname to the resolved ip before providing the url to
> XMLHttpRequest/ProtocolProxyService.
>    (What if the url is meaningful in the server-side)

This can be solved by setting the field 'host' on our own:

xhr.setRequesHeader('host', mmsc);

based on the following implementation detail:

1) Even though the 'host' field is not permitted to set in general,
   we are still able to mutate it since the MmsService.js is privileged
   to do it.

2) The host used to initiate the socket connection is according to 
   the URL (which is set whenever we call XMLHttpRequest.open()) 
   but not the header. ([2], where mConnInfo->Host() is set from mURI)

3) The host field is initialized at the very beginning [4] so that 
   it won't be overridden after we change it.

:mcmanus, 

Do you think if there is any risk of doing the following: 

---------------------------------------------------------------
let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
                  .createInstance(Ci.nsIXMLHttpRequest);
xhr.open('GET', http://175.22.25.547/test.html, true);

// www.some-domain-name.com resolved as 175.22.25.547
xhr.setRequestHeader('host', 'www.some-domain-name.com'); 

xhr.send(...);
---------------------------------------------------------------

I did some basic testing and everything went well.
I am wondering if something would be broken for other
more complicated cases.

Thanks!


[1] http://hg.mozilla.org/mozilla-central/file/7fc96293ada8/content/base/src/nsXMLHttpRequest.cpp#l3132
[2] http://hg.mozilla.org/mozilla-central/file/7fc96293ada8/netwerk/protocol/http/nsHttpConnectionMgr.cpp#l2912
[3] http://hg.mozilla.org/mozilla-central/file/7fc96293ada8/netwerk/protocol/http/nsHttpChannel.cpp#l4540
[4] http://hg.mozilla.org/mozilla-central/file/7fc96293ada8/netwerk/protocol/http/HttpBaseChannel.cpp#l140

Comment 21

[Patrick McManus [:mcmanus]]

I'm coming late to this bug - but it looks like you want to use XHR and have the app do DNS resolution itself, right?

Can you help me understand why? And why not just hook the DNS Service to do the override?

The way you have it written will probably work for that single transaction, but the transaction's concept of origin and everything that flows from that (e.g. SSL certificate auth, hsts, tracking protection, etc) will likely be mapped to the dotted decimal version which is not what you want.

Comment 22

[Henry Chang [:hchang]]

(In reply to Patrick McManus [:mcmanus] from comment #21)
> I'm coming late to this bug - but it looks like you want to use XHR and have
> the app do DNS resolution itself, right?
> 
> Can you help me understand why? 

Because we need a specific name server for mms XMLHttpRequest 
transaction while a general purpose name server is still required
at the same time to resolve other domain names.

> And why not just hook the DNS Service to do the override?
> 

That's exactly what we need! Could you please point the 
where the hook is? I checked nsXMLHttpRequest but didn't find.
The place that DNS is resolved through all the XMLHttpRequest
seems here [1].

[1] http://hg.mozilla.org/mozilla-central/file/d7e78f0c1465/netwerk/base/src/nsSocketTransport2.cpp#l1043

> The way you have it written will probably work for that single transaction,
> but the transaction's concept of origin and everything that flows from that
> (e.g. SSL certificate auth, hsts, tracking protection, etc) will likely be
> mapped to the dotted decimal version which is not what you want.

If we only use the response of the XHR [2], will it be fine?
The response isn't even parsed to a DOM tree and run client side
code.

[2] http://hg.mozilla.org/mozilla-central/file/d7e78f0c1465/dom/mobilemessage/src/gonk/MmsService.js#l704

Thanks!

Comment 23

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Henry Chang [:henry] from comment #22)
> (In reply to Patrick McManus [:mcmanus] from comment #21)
> > I'm coming late to this bug - but it looks like you want to use XHR and have
> > the app do DNS resolution itself, right?
> > 
> > Can you help me understand why? 
> 
> Because we need a specific name server for mms XMLHttpRequest 
> transaction while a general purpose name server is still required
> at the same time to resolve other domain names.
> 
> > And why not just hook the DNS Service to do the override?
> > 
> 
> That's exactly what we need! Could you please point the 
> where the hook is? I checked nsXMLHttpRequest but didn't find.
> The place that DNS is resolved through all the XMLHttpRequest
> seems here [1].
> 
> [1]
> http://hg.mozilla.org/mozilla-central/file/d7e78f0c1465/netwerk/base/src/
> nsSocketTransport2.cpp#l1043
> 
> > The way you have it written will probably work for that single transaction,
> > but the transaction's concept of origin and everything that flows from that
> > (e.g. SSL certificate auth, hsts, tracking protection, etc) will likely be
> > mapped to the dotted decimal version which is not what you want.
> 
> If we only use the response of the XHR [2], will it be fine?
> The response isn't even parsed to a DOM tree and run client side
> code.
> 
> [2]
> http://hg.mozilla.org/mozilla-central/file/d7e78f0c1465/dom/mobilemessage/
> src/gonk/MmsService.js#l704
> 
> Thanks!

In addition to the url to access in HTTPRequest, the proxy configuration is also required to take into consideration, because, for some carriers, a named mms proxy is also provided for mms transaction.

Besides, this solution looks mms-specific.
Shall we have a more general DNS solution not only for MMS but also for other service which goes through specific data connection like IMS in the future? (Where more protocols like SIP, RTCP, etc will be applied.)

Comment 24

[Henry Chang [:hchang]]

> In addition to the url to access in HTTPRequest, the proxy configuration is
> also required to take into consideration, because, for some carriers, a
> named mms proxy is also provided for mms transaction.
> 

My initial investigation is proxy would not be an issue.
Simply using ip based URL is good enough. But I have to
dig deeper to confirm this.

> Besides, this solution looks mms-specific.
> Shall we have a more general DNS solution not only for MMS but also for
> other service which goes through specific data connection like IMS in the
> future? (Where more protocols like SIP, RTCP, etc will be applied.)

Even we have a "general DNS solution", we still need to adapt
different applications to the "general DNS solution". For example,
Android has "per-process name server" design and it seems general.
But what if there are two applications which MUST run in the same 
process but still require different name servers? 

We could only build the most fundamental function 
"capability of resolving on designated server" and have each 
application adapt to it....

Thanks!

Comment 25

[Bevis Tseng[:bevis][:btseng](Exited)]

Forward the NI? to bug 1053650.

Comment 26

[Edgar Chen [:edgar]]

Comment on attachment 8470742 [details] [diff] [review]
Patch Part 3 v1: Replace setExtraHostRoute/removeExtraHostRoute with exposed APIs addExtraHostRoute/removeExtraHostRoute.

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

Sorry for being late.

::: dom/system/gonk/NetworkManager.js
@@ +644,5 @@
>        Services.io.offline = !this.active;
>      }
>    },
>  
> +  resolveHostname: function(hostname, network) {

network is an unused argument.

::: dom/system/gonk/nsINetworkManager.idl
@@ +210,5 @@
> +   * @return a Promise
> +   *         resolved if added; rejected, otherwise.
> +   */
> +  jsval addExtraHostRoute(in long type,
> +                          in unsigned long serviceId,

As per discussion, please take nsINetworkInterface as argument.
And addHostRoute seems enough, no need "Extra".

@@ +227,5 @@
> +   * @return a Promise
> +   *         resolved if removed; rejected, otherwise.
> +   */
> +  jsval removeExtraHostRoute(in long type,
> +                             in unsigned long serviceId,

Ditto

Comment 27

[Edgar Chen [:edgar]]

Comment on attachment 8470740 [details] [diff] [review]
Patch Part 1 v1: remove the unnecessary dependency of MOZ_B2G_RIL from the related Util APIs.

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

::: dom/system/gonk/NetworkManager.js
@@ -455,5 @@
>      this.setAndConfigureActive();
>    },
>  
>  #ifdef MOZ_B2G_RIL
> -  isNetworkTypeSecondaryMobile: function(type) {

Why move all these functions to a different location? Can we just adjust "#ifdef MOZ_B2G_RIL"?
It makes review easier. Thank you.

@@ -712,5 @@
>        Services.io.offline = !this.active;
>      }
>    },
>  
> -#ifdef MOZ_B2G_RIL

I really want to clean up the MOZ_B2G_RIL in NetworkManager.
For the platform without RIL function, there will be no NetworkInterface with MOBILE type registered. It seems most of MOZ_B2G_RIL can be replaced by checking network type. Let's file a bug for the rest of them. :)

Comment 28

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8470744 [details] [diff] [review]
Patch Part 4 v1: Adopt addExtraHostRoute/removeExtraHostRoute in MmsService.

Cancel review due to API signature change in Patch Part 3 by replacing type/serviceId with networkInterface.

Comment 29

[Edgar Chen [:edgar]]

Comment on attachment 8470741 [details] [diff] [review]
Patch Part 2 v1: Refactor setHostRoutes/removeHostRoutes.

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

::: dom/system/gonk/NetworkManager.js
@@ +602,5 @@
>      }
>      return null;
>    },
>  
> +  _updateRoutes: function(action, ipAddresses, network) {

Just takes interface name as argument.

@@ +607,2 @@
>      let gateways = network.getGateways();
> +    let ifName = network.name;

Then we don't need this.

@@ +612,5 @@
> +      let gateway = this.selectGateway(gateways, aIpAddress);
> +      if (gateway) {
> +        promises.push((action === "add")
> +          ? gNetworkService.addHostRoute(ifName, gateway, aIpAddress)
> +          : gNetworkService.removeHostRoute(ifName, gateway, aIpAddress));

Use boolean as the type of |action| seems enough and maybe need to have a better naming.

Comment 30

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1 v2: Deprecate setExtraHostRoute/removeExtraHostRoute. r=echen

I've re-organized the patch in different order for easier review.
Part 1 is to deprecate setExtraHostRoute/removeExtraHostRoute.

Hi Edgar,

May I have your review again for this change?

Thanks!

Comment 31

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2 v2: Rewrite resolveHostname() with Promise and one host to be resolved instead of a list of hosts. r=echen

I've re-organized the patch in different order for easier review.
Part 2 is to re-write resolveHostname() with Promise and one host to be resolved instead of a list of hosts.

Hi Edgar,

May I have your review again for this change?

Thanks!

Comment 32

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 3 v2: Refactor setHostRoutes()/removeHostRoutes(). r=echen

I've re-organized the patch in different order for easier review.
Part 3 is to refactor setHostRoutes()/removeHostRoutes(). r=echen

Hi Edgar,

May I have your review again for this change?

Thanks!

Comment 33

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 4 v2: Expose new APIs addHostRoute/removeHostRoute from NetworkManager.

I've re-organized the patch in different order for easier review.
Part 4 is to expose new APIs addHostRoute/removeHostRoute from NetworkManager.

Hi Edgar,

May I have your review again for this change?

Thanks!

Comment 34

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 5 v2: Remove MOZ_B2G_RIL closure for selectGateway()/resolveHostname(). r=echen

I've re-organized the patch in different order for easier review.
Part 5 is to remove MOZ_B2G_RIL closure for selectGateway()/resolveHostname().

Hi Edgar,

May I have your review again for this change?

Thanks!

Comment 35

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6 v2: Adopt addHostRoute/removeHostRoute in MmsService.

Due to the API suggestion in Patch Part 4 to re-design the APIs from
  jsval addExtraHostRoute(in long type,
                          in unsigned long serviceId,
                          in DOMString host);
  jsval RemoveExtraHostRoute(in long type,
                             in unsigned long serviceId,
                             in DOMString host);
To
  jsval addHostRoute(in nsINetworkInterface network,
                     in DOMString host);
  jsval removeHostRoute(in nsINetworkInterface network,
                        in DOMString host);
I've re-write the part in MmsService accordingly.

Hi Vicamo,

May I have your review again for this change?

Thanks!

Comment 36

[Edgar Chen [:edgar]]

Comment on attachment 8474320 [details] [diff] [review]
Part 4 v2: Expose new APIs addHostRoute/removeHostRoute from NetworkManager.

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

::: dom/system/gonk/NetworkManager.js
@@ +462,5 @@
>  
>      return Promise.all(promises);
>    },
>  
> +  addHostRoute: function(network, host) {

Please add check for network.
This operation should not be allowed if network is not yet registered to NetworkManager.

@@ +463,5 @@
>      return Promise.all(promises);
>    },
>  
> +  addHostRoute: function(network, host) {
> +    return this.resolveHostname(host, network)

|resolveHostname| takes only one argument.

@@ +470,5 @@
> +                                                network.name,
> +                                                network.getGateways()));
> +  },
> +
> +  removeHostRoute: function(network, host) {

ditto, add check for network.

@@ +471,5 @@
> +                                                network.getGateways()));
> +  },
> +
> +  removeHostRoute: function(network, host) {
> +    return this.resolveHostname(host, network)

ditto, takes only one argument.

::: dom/system/gonk/nsINetworkManager.idl
@@ +202,5 @@
> +   *
> +   * @param network
> +   *        The network interface where the host to be routed to.
> +   * @param host
> +   *        The host to be added.

This API will help to resolve domain name if |host| is not ip address.
Please help add some comments about this.
Thank you.

@@ +216,5 @@
> +   *
> +   * @param network
> +   *        The network interface where the routing to be removed from.
> +   * @param host
> +   *        The host routed to the network.

ditto

Comment 37

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

Comment on attachment 8474326 [details] [diff] [review]
Part 6 v2: Adopt addHostRoute/removeHostRoute in MmsService.

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

r- because sendRequest calls ensureRouting -- a function that should really be called only once when a mms connection is up.

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +287,5 @@
>     */
>    onDisconnectTimerTimeout: function() {
>      if (DEBUG) debug("onDisconnectTimerTimeout: deactivate the MMS data call.");
>      if (this.connected) {
> +      let deactivateMmsDataCall = (aError) => {

if (!this.connected) {
  return;
}

....

@@ +301,5 @@
> +
> +      this.hostsToRoute.forEach((aHost) => {
> +        promise.push(gNetworkManager
> +          .removeHostRoute(this.networkInterface, aHost));
> +      });

let promises =
  this.hostsToRoute.map(function(aHost) {
    return gNetworkManager.removeHostRoute(this.networkInterface, aHost);
  }, this);

@@ +723,5 @@
> +                                                 url, istream, proxyFilter,
> +                                                 cancellable.done.bind(cancellable));
> +        }.bind(this);
> +
> +        mmsConnection.ensureRouting(url)

We should really call |mmsConnection.ensureRouting| in |MmsConnection.acquire| callback. This function maybe called multiple times to send M-Read-Rec.ind in a |MobileMessageDatabaseService.markMessageRead|, and since ensureRouting() calls |gNetworkManager.addHostRoute| every time when its called, it follows we will call |gNetworkManager.addHostRoute| as many time as sendRequest is called.

Comment 38

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

Comment on attachment 8474326 [details] [diff] [review]
Part 6 v2: Adopt addHostRoute/removeHostRoute in MmsService.

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

Per offline discuss, |url| is available when calling to acquire(), but the insertion of host routes in sendRequest or any of its descendent in the call path still can not be avoid because the host routes differ and don't really belong to a common setup function. Thank you!

Comment 39

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #38)
> Comment on attachment 8474326 [details] [diff] [review]
> Part 6 v2: Adopt addHostRoute/removeHostRoute in MmsService.
> 
> Review of attachment 8474326 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Per offline discuss, |url| is available when calling to acquire(), but the
> insertion of host routes in sendRequest or any of its descendent in the call
> path still can not be avoid because the host routes differ and don't really
> belong to a common setup function. Thank you!

Thanks for understanding!
I'll update the patch to address the remained suggestion in comment 37.

Comment 40

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 4 v3: Expose new APIs addHostRoute/removeHostRoute from NetworkManager.

Address the problem in comment 36.

Comment 41

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6 v3: Adopt addHostRoute/removeHostRoute in MmsService. r=vyang

address nits in comment 37.

Comment 42

[Edgar Chen [:edgar]]

Comment on attachment 8475684 [details] [diff] [review]
Part 4 v3: Expose new APIs addHostRoute/removeHostRoute from NetworkManager.

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

Thank you. :)

Comment 43

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1: Deprecate setExtraHostRoute/removeExtraHostRoute. r=echen

update final patch with positive try server result.

Comment 44

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2: Rewrite resolveHostname() with Promise and one host to be resolved instead of a list of hosts. r=echen

update final patch with positive try server result.

Comment 45

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 3: Refactor setHostRoutes()/removeHostRoutes(). r=echen

update final patch with positive try server result.

Comment 46

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 4: Expose new APIs addHostRoute/removeHostRoute from NetworkManager. r=echen

update final patch with positive try server result.

Comment 47

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 5: Remove MOZ_B2G_RIL closure for selectGateway()/resolveHostname(). r=echen

update final patch with positive try server result.

Comment 48

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6: Adopt addHostRoute/removeHostRoute in MmsService. r=vyang

update final patch with positive try server result.

Comment 49

[Bevis Tseng[:bevis][:btseng](Exited)]

update try server result:
https://tbpl.mozilla.org/?tree=Try&rev=69e7f757714c

Comment 50

[Bevis Tseng[:bevis][:btseng](Exited)]

Hi Lissy,

May I ask for your help to double confirm if this patch can resolve the receiving problem in Free Mobile?

Thanks!

Comment 52

[:gerard-majax]

(In reply to Bevis Tseng [:bevistseng] ([e-mail]) from comment #50)
> Hi Lissy,
> 
> May I ask for your help to double confirm if this patch can resolve the
> receiving problem in Free Mobile?
> 
> Thanks!

After applying those patch I could download all MMS I wanted without any issue, with WiFi connected and no data connected. So I think it fixes everything \o/.

Thanks for the huge work!

Comment 53

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Alexandre LISSY :gerard-majax from comment #52)
> After applying those patch I could download all MMS I wanted without any
> issue, with WiFi connected and no data connected. So I think it fixes
> everything \o/.
> 
> Thanks for the huge work!

Thanks for double confirming this! \o/

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/59c75c97481c
https://hg.mozilla.org/integration/b2g-inbound/rev/806a037479be
https://hg.mozilla.org/integration/b2g-inbound/rev/3c7c668e92a7
https://hg.mozilla.org/integration/b2g-inbound/rev/aa746d0f8e52
https://hg.mozilla.org/integration/b2g-inbound/rev/4bdcfc5817d3
https://hg.mozilla.org/integration/b2g-inbound/rev/d9e5a211e546

Comment 55

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

https://hg.mozilla.org/mozilla-central/rev/59c75c97481c
https://hg.mozilla.org/mozilla-central/rev/806a037479be
https://hg.mozilla.org/mozilla-central/rev/3c7c668e92a7
https://hg.mozilla.org/mozilla-central/rev/aa746d0f8e52
https://hg.mozilla.org/mozilla-central/rev/4bdcfc5817d3
https://hg.mozilla.org/mozilla-central/rev/d9e5a211e546

Comment 56

[Bevis Tseng[:bevis][:btseng](Exited)]

[Blocking Requested - why for this release]:
1. In comment 86 of Bug 1031656(https://bugzilla.mozilla.org/show_bug.cgi?id=1031656#c86),
   We saw bad UX of our product of receiving MMS in the Free Mobile.
2. To Fix Bug 1031656, bug 1032097 & its dependency bug 1043114 is required.
3. ZTE device was sold in v1.3, so a solution for v1.3 has been requested for their MR release.
4. Set 1.4? to see the necessity of uplifting fix in 1.4 branch.

Note: patch for 1.3v, 1.4v, 2.0v has to been created accordingly instead of landing the fix in m-c directly.

Comment 57

[shawn ku [:sku]]

only Free@French hit this case, to reduce the risk, cancel 1.4?

Comment 58

[Bevis Tseng[:bevis][:btseng](Exited)]

[Blocking Requested - why for this release]:
1. In Bug 1031656 comment 86 (https://bugzilla.mozilla.org/show_bug.cgi?id=1031656#c86),
   We saw bad UX of our product of receiving MMS in the Free Mobile in French.
2. To Fix Bug 1031656, bug 1032097 & its dependency bug 1043114 is required.
3. ZTE device was sold in v1.3, so a solution for v1.3 has been requested for their MR release.
4. Set 2.0? to see the necessity of uplifting fix in 2.0 branch.

Note: 2.0v has to been created accordingly instead of landing the fix in m-c directly.

Comment 59

[Dietrich Ayala (:dietrich)]

Putting in TAM triage for Rochelle to make the decision.

Comment 60

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Agree to block on 2.0 since partners might launch 2.0 devices in France with Free. For 1.3 devices many France users already complained about this issue on some forums, we do need to fix this one in 2.0

Thanks

Vance

Comment 61

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [1.3v] Part 1: Add Promise support for addHostRouteWithResolve/removeHostRouteWithResolve in NetworkService.js r=echen

Hi Edgar,

This to have a quick fix of supporting promise of setExtraHostRoute/removeExtraHostRoute in NetworkService.js.

May I have your review for this change in 1.3 branch?

Thanks!

Comment 62

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [1.3v] Part 2: Expose new APIs addHostRoute/removeHostRoute from NetworkManager. r=echen

Hi Edgar,

This is to expose addHostRoute/removeHostRoute from NetworkManager for MmsService to support adding/removing host routes per mms connection.

May I have your review for this change in 1.3v branch?

Thanks!

Comment 63

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [1.3v] Part 3: Adopt addHostRoute/removeHostRoute in MmsService. r=vyang

Rebase Patch Part 6 for 1.3v branch.

Comment 64

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Triage: 2.0+ since lots of France users complained about this one on 1.3 devices and we really should fix it for 2.0

Thanks

Comment 65

[Edgar Chen [:edgar]]

Comment on attachment 8492952 [details] [diff] [review]
[1.3v] Part 1: Add Promise support for addHostRouteWithResolve/removeHostRouteWithResolve in NetworkService.js r=echen

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

Looks good, thank you.

::: dom/system/gonk/NetworkService.js
@@ +332,5 @@
>        hostnames: hosts
>      };
> +
> +    this.controlMessage(options, function() {
> +      deferred.resolve();

Maybe we need to reject the deferred promise if the operation is failed. But actually we don't handle the error case now, either. So I am ok with this for 1.3 branch.

@@ +349,5 @@
>        hostnames: hosts
>      };
> +
> +    this.controlMessage(options, function() {
> +      deferred.resolve();

Ditto.

Comment 66

[Ryan VanderMeulen [:RyanVM]]

I'm a bit confused by the recent comments about whether this needs a b2g32 approval request or not? Is this intended to land on v2.0 proper or just a random partner branch?

Comment 67

[Bevis Tseng[:bevis][:btseng](Exited)]

Sorry for confusing.
Yes, I'll have to provide another patch for v2.0 and request for b2g32 approval before landing it. :)

Comment 68

[Bevis Tseng[:bevis][:btseng](Exited)]

wontfix for 1.4v according to comment 57.

Comment 69

[Edgar Chen [:edgar]]

Comment on attachment 8492954 [details] [diff] [review]
[1.3v] Part 2: Expose new APIs addHostRoute/removeHostRoute from NetworkManager. r=echen

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

Thank you.

Comment 70

[Bevis Tseng[:bevis][:btseng](Exited)]

Update ETA to 10/3 for 2.0+

Comment 71

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [2.0v] Patch - Resolve/Add/Remove extra Host by MmsService per MMS connection. r=echen,vyang,a=2.0+

refine patch for 2.0v branch.

Comment 72

[Bevis Tseng[:bevis][:btseng](Exited)]

update try server result for 2.0v patch:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=66d5ac0c45af

Comment 73

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8495802 [details] [diff] [review]
[2.0v] Patch - Resolve/Add/Remove extra Host by MmsService per MMS connection. r=echen,vyang,a=2.0+

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined:
Subscribers of Free@France was not able to receive MMS if mobile data connection is off. Free@France provides special plan for messaging/data. With this plan, users prefer to disable the data plan by default. Please see Bug 1031656 comment 86 for more detailed discussion of the user impact.
Testing completed: Yes.
Risk to taking this patch (and alternatives if risky): No
String or UUID changes made by this patch:
28babd1c-4491-11e4-97f1-f708c17b2926 for nsINetworkService change.
30fceeb8-4492-11e4-b00c-b7c279da6be7 for nsINetworkManager change.

Comment 74

[bhavana bajaj [:bajaj]]

(In reply to Bevis Tseng [:bevistseng] ([e-mail]) from comment #73)
> Comment on attachment 8495802 [details] [diff] [review]
> [2.0v] Patch - Resolve/Add/Remove extra Host by MmsService per MMS
> connection. r=echen,vyang,a=2.0+
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): N/A
> User impact if declined:
> Subscribers of Free@France was not able to receive MMS if mobile data
> connection is off. Free@France provides special plan for messaging/data.
> With this plan, users prefer to disable the data plan by default. Please see
> Bug 1031656 comment 86 for more detailed discussion of the user impact.
> Testing completed: Yes.
> Risk to taking this patch (and alternatives if risky): No
> String or UUID changes made by this patch:
> 28babd1c-4491-11e4-97f1-f708c17b2926 for nsINetworkService change.
> 30fceeb8-4492-11e4-b00c-b7c279da6be7 for nsINetworkManager change.

I see a lot of red flags in landing this on 2.0 at this point
a) The change is huge and looks risky
b) has idl changes.(typically these are not recommended this late in the game)..
c) Don't see any until tests on the patch..

Bevis can you comment on all the above?

Vance, for what partner/device do we exactly need this. Is this something that needs to land for CAF partner?

Comment 75

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to bhavana bajaj [:bajaj] from comment #74)
> (In reply to Bevis Tseng [:bevistseng] ([e-mail]) from comment #73)
> > Comment on attachment 8495802 [details] [diff] [review]
> > [2.0v] Patch - Resolve/Add/Remove extra Host by MmsService per MMS
> > connection. r=echen,vyang,a=2.0+
> > 
> > [Approval Request Comment]
> > Bug caused by (feature/regressing bug #): N/A
> > User impact if declined:
> > Subscribers of Free@France was not able to receive MMS if mobile data
> > connection is off. Free@France provides special plan for messaging/data.
> > With this plan, users prefer to disable the data plan by default. Please see
> > Bug 1031656 comment 86 for more detailed discussion of the user impact.
> > Testing completed: Yes.
> > Risk to taking this patch (and alternatives if risky): No
> > String or UUID changes made by this patch:
> > 28babd1c-4491-11e4-97f1-f708c17b2926 for nsINetworkService change.
> > 30fceeb8-4492-11e4-b00c-b7c279da6be7 for nsINetworkManager change.
> 
> I see a lot of red flags in landing this on 2.0 at this point
> a) The change is huge and looks risky
This change has been verified in 2.1/master already.
> b) has idl changes.(typically these are not recommended this late in the
> game)..
Yes, but they are mandatory change to fix this bug. :(
> c) Don't see any until tests on the patch..
Sorry, currently, this use case has to be verified in live network instead and I've has verified it  locally.
> 
> Bevis can you comment on all the above?
> 
> Vance, for what partner/device do we exactly need this. Is this something
> that needs to land for CAF partner?
The alternative solution is to have vendor to cherry pick this patch when needed.

NI Vance for last question.

Comment 76

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

> 
> Vance, for what partner/device do we exactly need this. Is this something
> that needs to land for CAF partner?

Hi Bhavana -

As far as I know, there are already many Open C users in France complained about this issue, for example:

http://forums.mozfr.org/viewtopic.php?f=35&t=119228&sid=d99796b25ec702d595eba8666ae9d22b

ni Natalia, also Julien and Alexandre here since they are in France and might know better than me if this one is really a serious issue in France

As Bevis mentioned, this patch has already landed on 2.1 and master, also ZTE is going to cheery-pick into their own 1.3 source tree. I see no reason to skip 2.0, hence I do suggest to land it in 2.0

Thanks

Comment 77

[Julien Wajsberg [:julienw]]

This issue prevents users of Free Mobile (~13% of french mobile users) from using MMS correctly in some situations. I won't enter in much more details because I don't have all information (we also had bug 1029453 in the past, which was resolved by Free Mobile without change from our side, and I don't know which of the issue this particular bug aims to fix).

Note that it could happen in other carriers too.

I think it's important enough to include this patch in v2.0. I agree it's creepy, especially because it touches other areas than MMS and tests are seldom. We'll need a solid QA around network connections (combinations of 3G + Wi-FI + sending MMS + sending SMS + using data + connection sharing).

Comment 78

[Bevis Tseng[:bevis][:btseng](Exited)]

Actually, in 2.0v patch, the API changes in NetworkService/NetworkManager are the methods merely used by MmsService. There shall be no impact to other other data connections or Wifi. :)
The major purpose of this change is to move the logic of adding/removing routing from NetworkManager to MmsService to have more flexibility of accessing the host per MMS transaction.

Comment 79

[:gerard-majax]

Julien already documented the situation quite well.

Comment 80

[Natalia Martinez-Winter]

in France the marketshare of Free is around 15%. but because the Openc C is an affordable phone, Free mobile is over represented among those users, if you look at the FTU activations in France https://metrics.mozilla.com/protected/dashboards/fxos-ftu/# you'll notice that Free represent a third of our french users.

Comment 81

[bhavana bajaj [:bajaj]]

Comment on attachment 8495802 [details] [diff] [review]
[2.0v] Patch - Resolve/Add/Remove extra Host by MmsService per MMS connection. r=echen,vyang,a=2.0+

Approving given the justified number of users this is impacting. Can QA please help verify this. I am hoping johan can help here, so NI him.

Comment 82

[bhavana bajaj [:bajaj]]

Honestly, I'd also like to get feeback from given this huge change so there are aware and can point any major red flags. Ni inder.

Comment 83

[bhavana bajaj [:bajaj]]

(In reply to bhavana bajaj [:bajaj] from comment #82)
> Honestly, I'd also like to get feeback from
from "CAF" given the RIL changes...
> given this huge change so there
> are aware and can point any major red flags. Ni inder.

Comment 84

[Johan Lorenzo [:jlorenzo]]

(In reply to bhavana bajaj [:bajaj] from comment #81)
> Approving given the justified number of users this is impacting. Can QA
> please help verify this. I am hoping johan can help here, so NI him.

Moving to Eric as he's the owner the Messaging part. Since the issue is easily reproducible in France, do not hesitate to NI me again if you can't test the patch, Eric.

Comment 85

[Inder]

Based on the feedback that this is customer impacting issue we are okay landing this in 2.0. We did a quick test with our RIL and didn't show any issues.

Comment 86

[bhavana bajaj [:bajaj]]

(In reply to Inder from comment #85)
> Based on the feedback that this is customer impacting issue we are okay
> landing this in 2.0. We did a quick test with our RIL and didn't show any
> issues.

Thanks removing the NO_UPLIFT, so this can land now. QA, please make sure to do exploratory testing around this area of code. thanks!

Comment 87

[Eric Chang [:ericcc] [:echang]]

Hi Alexandre, the basic functionaliity is okay, but in this case, we need a SIM from http://mobile.free.fr/ to test MMS, Could you help to verify this, thanks...

Comment 88

[Eric Chang [:ericcc] [:echang]]

I will also run some regression test against this.

Comment 89

[Johan Lorenzo [:jlorenzo]]

Alexandre and I tested the 2.0 patch with a SIM card from the Free provider. A single or two MMSes can be downloaded without any issue, from a user perspective. This error still appears in the logcat though:
>10-03 11:37:41.663   206   206 I Gecko   : -@- MmsService: Failed to ensureRouting: Error: Failed to resolve 'mms.free.fr', with status: 2152398878

Do you have any idea, Bevis?

For this feature, I think this patch is good enough to be uplifted in 2.0. Let's wait for the regression test by Eric to signoff it.

Comment 90

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Johan Lorenzo [:jlorenzo] from comment #89)
> Alexandre and I tested the 2.0 patch with a SIM card from the Free provider.
> A single or two MMSes can be downloaded without any issue, from a user
> perspective. This error still appears in the logcat though:
> >10-03 11:37:41.663   206   206 I Gecko   : -@- MmsService: Failed to ensureRouting: Error: Failed to resolve 'mms.free.fr', with status: 2152398878
> 
> Do you have any idea, Bevis?
> 
> For this feature, I think this patch is good enough to be uplifted in 2.0.
> Let's wait for the regression test by Eric to signoff it.

Hi Johan,

This is a known issue in Flame and has been addressed in Bug 1058282.
The work around already applied by vendor is to have the ip address of mms.free.fr instead of the host name in the APN settings.

Thanks for your testing. :)

Regards,
Bevis Tseng

Comment 91

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/7af11e563f53

Comment 92

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/7af11e563f53