Closed Bug 1032097 Opened 6 years ago Closed 5 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set

Tracking

(blocking-b2g:2.0+, firefox32 wontfix, firefox33 wontfix, firefox34 fixed, b2g-v1.4 wontfix, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed)

VERIFIED FIXED
2.1 S3 (29aug)
blocking-b2g 2.0+
Tracking Status
firefox32 --- wontfix
firefox33 --- wontfix
firefox34 --- fixed
b2g-v1.4 --- wontfix
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed

People

(Reporter: bevis, Assigned: bevis)

References

Details

(Whiteboard: [p=3])

Attachments

(10 files, 13 obsolete files)

5.84 KB, patch
bevis
: review+
Details | Diff | Splinter Review
3.19 KB, patch
bevis
: review+
Details | Diff | Splinter Review
3.07 KB, patch
bevis
: review+
Details | Diff | Splinter Review
4.41 KB, patch
bevis
: review+
Details | Diff | Splinter Review
2.83 KB, patch
bevis
: review+
Details | Diff | Splinter Review
11.58 KB, patch
bevis
: review+
Details | Diff | Splinter Review
5.31 KB, patch
edgar
: review+
Details | Diff | Splinter Review
13.58 KB, patch
edgar
: review+
Details | Diff | Splinter Review
13.30 KB, patch
bevis
: review+
Details | Diff | Splinter Review
30.05 KB, patch
bevis
: review+
Details | Diff | Splinter Review
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.
Can't we just make use of NetworkService addHostRouteWithResolve in the MmsService ?
Update title per comment 1 and comment 41 in bug 1031656.
Summary: [B2G][MMS] Expose new API from NetworkManager to setExtraHostToRoute. → [B2G][MMS] Resolve/Add/Remove extra Host from the URL to be downloaded per MMS transaction.
Whiteboard: [p=3][ETA=7/21]
Target Milestone: --- → 2.1 S1 (1aug)
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.)
Summary: [B2G][MMS] Resolve/Add/Remove extra Host from the URL to be downloaded per MMS transaction. → [B2G][MMS] Resolve/Add/Remove extra Host by MmsService per MMS connection.
Whiteboard: [p=3][ETA=7/21] → [p=5]
Whiteboard: [p=5] → [p=5][ETA=7/25]
reset target milestone due to unexpected personal affairs last week.
Whiteboard: [p=5][ETA=7/25] → [p=5]
Target Milestone: 2.1 S1 (1aug) → ---
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.
Flags: needinfo?(nwinter)
Flags: needinfo?(btseng)
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
Flags: needinfo?(btseng)
Whiteboard: [p=5] → [p=5][ETA=8/8]
Target Milestone: --- → 2.1 S2 (15aug)
Blocks: 939026
Flags: needinfo?(nwinter)
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.
Attachment #8463832 - Flags: feedback?(jjong)
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.
Whiteboard: [p=5][ETA=8/8] → [p=3]
Target Milestone: 2.1 S2 (15aug) → ---
set dependency to bug 1043114 according to comment 8.
Depends on: 1043114
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) 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 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.
Attachment #8463832 - Flags: feedback?(jjong)
(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.
Target Milestone: --- → 2.1 S3 (29aug)
Blocks: 992772
This is to move related utilities from the protection of MOZ_B2G_RIL.

Hi Edgar,

May I have your review for this change?

Thanks!
Attachment #8463832 - Attachment is obsolete: true
Attachment #8470740 - Flags: review?(echen)
This is to refactor the common part of setHostRoutes/removeHostRoutes.

Hi Edgar,

May I have your review for this change?

Thanks!
Attachment #8470741 - Flags: review?(echen)
Attachment #8470740 - Attachment description: Patch Part 1 v1: remove the unnecessary dependency of MOZ_B2G_RIL from the related Util APIs. r=echen → Patch Part 1 v1: remove the unnecessary dependency of MOZ_B2G_RIL from the related Util APIs.
This is to replace setExtraHostRoute/removeExtraHostRoute with newly exposed APIs addExtraHostRoute/removeExtraHostRoute.

Hi Edgar,

May I have your review for this change?

Thanks!
Attachment #8470742 - Flags: review?(echen)
Attachment #8470741 - Attachment description: Patch Part 2 v2: Refactor setHostRoutes/removeHostRoutes. → Patch Part 2 v1: Refactor setHostRoutes/removeHostRoutes.
This is to Adopt newly-exposed APIs addExtraHostRoute/removeExtraHostRoute in MmsService.

Hi Vicamo,

May I have your review for this change?

Thanks!
Attachment #8470744 - Flags: review?(vyang)
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?
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) 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
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.
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) 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
Flags: needinfo?(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.
Flags: needinfo?(mcmanus)
(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!
Flags: needinfo?(mcmanus)
(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.)
> 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!
Forward the NI? to bug 1053650.
Flags: needinfo?(mcmanus)
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
Attachment #8470742 - Flags: review?(echen)
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. :)
Attachment #8470740 - Flags: review?(echen)
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.
Attachment #8470744 - Flags: review?(vyang)
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.
Attachment #8470741 - Flags: review?(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!
Attachment #8470740 - Attachment is obsolete: true
Attachment #8470741 - Attachment is obsolete: true
Attachment #8470742 - Attachment is obsolete: true
Attachment #8470744 - Attachment is obsolete: true
Attachment #8474317 - Flags: review?(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!
Attachment #8474318 - Flags: review?(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!
Attachment #8474319 - Flags: review?(echen)
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!
Attachment #8474320 - Flags: review?(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!
Attachment #8474321 - Flags: review?(echen)
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!
Attachment #8474326 - Flags: review?(vyang)
Attachment #8474317 - Flags: review?(echen) → review+
Attachment #8474318 - Flags: review?(echen) → review+
Attachment #8474319 - Flags: review?(echen) → review+
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
Attachment #8474320 - Flags: review?(echen)
Attachment #8474321 - Flags: review?(echen) → review+
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.
Attachment #8474326 - Flags: review?(vyang) → review-
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!
Attachment #8474326 - Flags: review- → review+
(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.
Address the problem in comment 36.
Attachment #8474320 - Attachment is obsolete: true
Attachment #8475684 - Flags: review?(echen)
Attachment #8474318 - Attachment description: Part 2 v2: Rewrite resolveHostname() with Promise and one host to be resolved instead of a list of hosts. → Part 2 v2: Rewrite resolveHostname() with Promise and one host to be resolved instead of a list of hosts. r=echen
Attachment #8474321 - Attachment description: Part 5 v2: Remove MOZ_B2G_RIL closure for selectGateway()/resolveHostname(). → Part 5 v2: Remove MOZ_B2G_RIL closure for selectGateway()/resolveHostname(). r=echen
address nits in comment 37.
Attachment #8474326 - Attachment is obsolete: true
Attachment #8475685 - Flags: review+
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. :)
Attachment #8475684 - Flags: review?(echen) → review+
update final patch with positive try server result.
Attachment #8474317 - Attachment is obsolete: true
Attachment #8477132 - Flags: review+
update final patch with positive try server result.
Attachment #8474318 - Attachment is obsolete: true
Attachment #8477134 - Flags: review+
update final patch with positive try server result.
Attachment #8474319 - Attachment is obsolete: true
Attachment #8477135 - Flags: review+
update final patch with positive try server result.
Attachment #8475684 - Attachment is obsolete: true
Attachment #8477137 - Flags: review+
update final patch with positive try server result.
Attachment #8474321 - Attachment is obsolete: true
Attachment #8477139 - Flags: review+
update final patch with positive try server result.
Attachment #8475685 - Attachment is obsolete: true
Attachment #8477141 - Flags: review+
Hi Lissy,

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

Thanks!
Flags: needinfo?(lissyx+mozillians)
Duplicate of this bug: 939026
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) 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!
Flags: needinfo?(lissyx+mozillians)
(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/
[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.
blocking-b2g: --- → 1.4?
only Free@French hit this case, to reduce the risk, cancel 1.4?
blocking-b2g: 1.4? → ---
[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.
blocking-b2g: --- → 2.0?
Putting in TAM triage for Rochelle to make the decision.
Flags: needinfo?(ryang)
Whiteboard: [p=3] → [p=3][TAM-triage?]
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
Flags: needinfo?(ryang)
Whiteboard: [p=3][TAM-triage?] → [p=3]
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!
Attachment #8492952 - Flags: review?(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!
Attachment #8492954 - Flags: review?(echen)
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
blocking-b2g: 2.0? → 2.0+
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.
Attachment #8492952 - Flags: review?(echen) → review+
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?
Flags: needinfo?(btseng)
Sorry for confusing.
Yes, I'll have to provide another patch for v2.0 and request for b2g32 approval before landing it. :)
Flags: needinfo?(btseng)
wontfix for 1.4v according to comment 57.
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.
Attachment #8492954 - Flags: review?(echen) → review+
Update ETA to 10/3 for 2.0+
Whiteboard: [p=3] → [p=3][ETA=10/3]
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.
Attachment #8495802 - Flags: approval-mozilla-b2g32?
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) 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?
(In reply to bhavana bajaj [:bajaj] from comment #74)
> (In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) 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.
Flags: needinfo?(vchen)
> 
> 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
Flags: needinfo?(vchen)
Flags: needinfo?(nwinter)
Flags: needinfo?(lissyx+mozillians)
Flags: needinfo?(felash)
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).
Flags: needinfo?(felash)
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.
Julien already documented the situation quite well.
Flags: needinfo?(lissyx+mozillians)
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.
Flags: needinfo?(nwinter)
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.
Attachment #8495802 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
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.
Flags: needinfo?(jlorenzo)
Flags: needinfo?(ikumar)
Whiteboard: [p=3][ETA=10/3] → [p=3][ETA=10/3] [NO_UPLIFT]
(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.
(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.
Flags: needinfo?(jlorenzo) → needinfo?(echang)
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.
Flags: needinfo?(ikumar)
(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!
Whiteboard: [p=3][ETA=10/3] [NO_UPLIFT] → [p=3][ETA=10/3]
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...
Flags: needinfo?(echang) → needinfo?(lissyx+mozillians)
I will also run some regression test against this.
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.
Flags: needinfo?(lissyx+mozillians)
Flags: needinfo?(echang)
Flags: needinfo?(btseng)
(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
Flags: needinfo?(btseng)
Flags: needinfo?(echang)
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1031656
Depends on: 1152531
Blocks: Woodduck
You need to log in before you can comment on or make changes to this bug.