1018088 - PUSH wakeup compatible multiple mobile networks carriers

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Multiple carriers with more than 16M users manages multiple private networks under the same MCC-MNC pair.

So the current mobile-network identification used in simple push is incomplete since it not identify each network in an unique way.

The proposed idea is to add a new parameter to the payload sent to the push notification server (PNS), the "netid", so push agent should sent IP, Port, MCC, MNC and if available, the netid.

The netid will be recovered from the DNS querying the TXT record based on a well-known URL.

Comment 1

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

http://telefonicaid.github.io/wakeup_platform_documentation/doc/html_chunked/ch03.html

Comment 2

[Jason Smith [:jsmith]]

Is this is a feature request or a bug?

Comment 3

[Doug Turner (:dougt)]

jsmith: both

Comment 4

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Jason Smith [:jsmith] from comment #2)
> Is this is a feature request or a bug?

Hi Jason,

As Doug told, is "both" :) I mean:

Feature request because it's something new we didn't considered in the first stages of the simplepush development

A bug because without this new feature, simplepush cann't use the wakeup advantages in some big carriers like O2-UK, AT&T, Vivo, ... who have more than only one private network under the same MCC-MNC

The path is not too big, push agent should make a DNS query and send the result to the Push server.

Comment 5

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Proposed patch

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8434218 [details] [diff] [review]
Proposed patch

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

Is it possible to locally cache the netid once, every time the mobile network changes, rather than on every reconnect?

::: b2g/app/b2g.js
@@ +466,5 @@
> +// This value should be the prefix to be added to the current PDP context domain
> +// or a full-qualified domain name
> +// If finished with a dot, will be added as a prefix to the PDP context domain
> +// if not, will be used directly into the DNS query.
> +pref("services.push.udp.well-known_netidAddress", "_wakeup_.");

Will anything go wrong if this is not customized but udp wakeup is enabled? i.e. if the DNS entry is not found for _wakeup_.mycustomcarrier.com, will push still work using just mcc+mnc? Will the websocket variant still work? Please describe that here.

::: dom/push/src/PushService.jsm
@@ +1565,5 @@
> +    function queryDNSForDomain(domain) {
> +      let retval = [];
> +      try {
> +        debug("[_getMobileNetworkId] Quering DNS for " + domain);
> +        let hostnameIps = gDNSService.resolve(domain, 0);

We are running on the main thread. Please use the async variants.

Comment 7

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #6)
> Comment on attachment 8434218 [details] [diff] [review]
> Proposed patch
> 
> Review of attachment 8434218 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is it possible to locally cache the netid once, every time the mobile
> network changes, rather than on every reconnect?
> 

I don't think so ... since the operator can assing you in one or another sub-network each time.
So that's the reason I check it each time a mobile connection is detected.

> ::: b2g/app/b2g.js
> @@ +466,5 @@
> > +// This value should be the prefix to be added to the current PDP context domain
> > +// or a full-qualified domain name
> > +// If finished with a dot, will be added as a prefix to the PDP context domain
> > +// if not, will be used directly into the DNS query.
> > +pref("services.push.udp.well-known_netidAddress", "_wakeup_.");
> 
> Will anything go wrong if this is not customized but udp wakeup is enabled?
> i.e. if the DNS entry is not found for _wakeup_.mycustomcarrier.com, will
> push still work using just mcc+mnc? Will the websocket variant still work?
> Please describe that here.
> 

It will send a null netid and is a server decission what to do with the mobile. If the MCC-MNC identifies a unique network the server will change to UDP mode, in other case, a websocket connection will be maintained.

> ::: dom/push/src/PushService.jsm
> @@ +1565,5 @@
> > +    function queryDNSForDomain(domain) {
> > +      let retval = [];
> > +      try {
> > +        debug("[_getMobileNetworkId] Quering DNS for " + domain);
> > +        let hostnameIps = gDNSService.resolve(domain, 0);
> 
> We are running on the main thread. Please use the async variants.

I'll change this :)

Thanks!

Comment 8

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Patch V2

Async. version :)

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8435684 [details] [diff] [review]
Patch V2

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

The patch is almost there, but I have several comments and there are a couple of logical flaws.

::: b2g/app/b2g.js
@@ +462,5 @@
>  // How long before a DOMRequest errors as timeout
>  pref("services.push.requestTimeout", 10000);
>  // enable udp wakeup support
>  pref("services.push.udp.wakeupEnabled", true);
> +// This value should be the prefix to be added to the current PDP context domain

Nit: Please expand PDP here since not all readers of the pref will be mobile network experts :) I'm assuming it means Packet Data Protocol

@@ +463,5 @@
>  pref("services.push.requestTimeout", 10000);
>  // enable udp wakeup support
>  pref("services.push.udp.wakeupEnabled", true);
> +// This value should be the prefix to be added to the current PDP context domain
> +// or a full-qualified domain name

Period at the end.

@@ +464,5 @@
>  // enable udp wakeup support
>  pref("services.push.udp.wakeupEnabled", true);
> +// This value should be the prefix to be added to the current PDP context domain
> +// or a full-qualified domain name
> +// If finished with a dot, will be added as a prefix to the PDP context domain

Nit: s/will/it will/

@@ +465,5 @@
>  pref("services.push.udp.wakeupEnabled", true);
> +// This value should be the prefix to be added to the current PDP context domain
> +// or a full-qualified domain name
> +// If finished with a dot, will be added as a prefix to the PDP context domain
> +// if not, will be used directly into the DNS query.

Ditto. and also s/directly into/as/

@@ +466,5 @@
> +// This value should be the prefix to be added to the current PDP context domain
> +// or a full-qualified domain name
> +// If finished with a dot, will be added as a prefix to the PDP context domain
> +// if not, will be used directly into the DNS query.
> +// If the DNS query is unsuccesfull, the push agent will send a null netid and

Nit: Type unsuccessful

@@ +468,5 @@
> +// If finished with a dot, will be added as a prefix to the PDP context domain
> +// if not, will be used directly into the DNS query.
> +// If the DNS query is unsuccesfull, the push agent will send a null netid and
> +// is a server decission what to do with the mobile. If the MCC-MNC identifies a
> +// unique network the server will change to UDP mode, in other case, a websocket

Period after UDP mode and then s/in other case/Otherwise.

Have you guys tested all these possibilities?

::: dom/push/src/PushService.jsm
@@ +654,5 @@
>      }
>  
> +    // Read the APN data from the settings DB.
> +    let lock = gSettingsService.createLock();
> +    lock.get("ril.data.apnSettings", this);

From what I read on MDN and how b2g locks seem to work, this lock will automatically be released on the setting is retrieved. Correct? I just want to double check.

@@ +670,5 @@
> +  handle: function(name, result) {
> +    if (name !== "ril.data.apnSettings") {
> +      return;
> +    }
> +    var apn = result[0].filter(function(e) {

Can result ever be empty? For example on tablets? If so, you should check for it.

@@ +1341,5 @@
>        this._wsSendMessage(data);
>        this._currentState = STATE_WAITING_FOR_HELLO;
>      }
>  
> +    var self = this;

Do you just want to use the fat-arrow syntax for the callback below now that we support it? That way you can get rid of this self/this annoyance.

They shipped in Fx22, I hope b2g 2.0 branched after that.

@@ -1428,5 @@
>        debug("UDP Server already running");
>        return;
>      }
>  
> -    if (!this._getNetworkState().ip) {

If this check is removed, please add a comment to _listenForUDPWakeup() saying callers should ensure that the device is on a mobile network! I wish there was a non-expensive way to assert this. Is there? (If there isn't, please don't consider that as blocking this)

@@ +1505,5 @@
>     * with an IP).
>     */
> +  _getNetworkState: function(callback) {
> +    if (!callback || typeof callback !== 'function') {
> +      return;

I think you should throw instead, because not passing a callback will mean the websocket is not started above, and we don't have a functioning push service. First condition made redundant by the second.

@@ +1511,5 @@
>      debug("getNetworkState()");
>      try {
>        if (!prefs.get("udp.wakeupEnabled")) {
>          debug("UDP support disabled, we do not send any carrier info");
>          throw "UDP disabled";

While you are here, let's make this |throw new Error("UDP disabled");| Thanks!

@@ +1531,5 @@
> +            debug("Recovered netID = " + netid);
> +
> +            let ips = {};
> +            let prefixLengths = {};
> +            nm.active.getAddresses(ips, prefixLengths);

These three lines can be moved out of the closure since they don't depend on netid.

@@ +1544,2 @@
>          }
> +        return;

If iccInfo can be null, the return should be moved inside the check. If it can't ever be null, we should 'assert' that here and replace the if() with throwing an error.

@@ +1546,2 @@
>        }
>      } catch (e) {}

While you are here, I don't like how we are silently ignoring all errors in this block. Could you change it to at least dump/log the error message.

@@ +1566,5 @@
> +
> +  // Get the mobile network ID (netid)
> +  _getMobileNetworkId: function(callback) {
> +    if (!callback || typeof callback !== 'function') {
> +      return;

Throw. The first condition is also made redundant by the second.

@@ +1570,5 @@
> +      return;
> +    }
> +
> +    function queryDNSForDomain(domain) {
> +      debug("[_getMobileNetworkId:queryDNSForDomain] Quering DNS for " +

Nit: Querying

@@ +1572,5 @@
> +
> +    function queryDNSForDomain(domain) {
> +      debug("[_getMobileNetworkId:queryDNSForDomain] Quering DNS for " +
> +        domain);
> +      var netIDDNSListener = {

Nit: s/var/let. Also use let everywhere else I have missed pointing it out.

@@ +1586,5 @@
> +          }
> +        }
> +      };
> +      gDNSService.asyncResolve(domain, 0, netIDDNSListener,
> +        threadManager.currentThread);

Just a comment: I'm surprised this is required since JS can only ever run in the same thread so the callback should end up being called in main thread, but whatever :)

@@ +1594,5 @@
> +    debug("[_getMobileNetworkId:queryDNSForDomain] Getting mobile network ID (I'm " +
> +       gDNSService.myHostName + ")");
> +
> +    let netidAddress = prefs.get("udp.well-known_netidAddress");
> +    if (netidAddress[netidAddress.length - 1] === '.' && this._apnDomain) {

This conditional is wrong. If apnDomain is unset (which is very much possible if the setting service is somehow busy so that the ordering of these async calls is non-ideal), then you are going to attempt querying just by netidAddress, even if it ends with a period.

Also, just use netidAddress.endsWith("."). While considered 'experimental', it's been around since Fx17 and used all over the codebase.

Comment 10

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #9)

> Have you guys tested all these possibilities?
> 

yes :)

> ::: dom/push/src/PushService.jsm
> @@ +654,5 @@
> >      }
> >  
> > +    // Read the APN data from the settings DB.
> > +    let lock = gSettingsService.createLock();
> > +    lock.get("ril.data.apnSettings", this);
> 
> From what I read on MDN and how b2g locks seem to work, this lock will
> automatically be released on the setting is retrieved. Correct? I just want
> to double check.
> 

Yes, I've the same opinion

> If this check is removed, please add a comment to _listenForUDPWakeup() saying callers should ensure that > the device is on a mobile network! I wish there was a non-expensive way to assert this. Is there?
> (If there isn't, please don't consider that as blocking this)

Currently it's an async. method and without cache (as I mentioned before). This method (_listenForWakeUp) is only needed when the device is in a mobile network, so the check is not necessary if called in the good moment.

Comment 11

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #9)

> @@ +1586,5 @@
> > +          }
> > +        }
> > +      };
> > +      gDNSService.asyncResolve(domain, 0, netIDDNSListener,
> > +        threadManager.currentThread);
> 
> Just a comment: I'm surprised this is required since JS can only ever run in
> the same thread so the callback should end up being called in main thread,
> but whatever :)
> 

Agree :)

Comment 12

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Patch V3

Hi Nikhil,

All comments addressed.

Regards

Comment 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8436434 [details] [diff] [review]
Patch V3

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

r=me with comments addressed.

::: b2g/app/b2g.js
@@ +467,5 @@
> +// domain or a full-qualified domain name.
> +// If finished with a dot, it will be added as a prefix to the PDP context
> +// domain. If not, will be used as the DNS query.
> +// If the DNS query is unsuccessful, the push agent will send a null netid and
> +// is a server decission what to do with the mobile. If the MCC-MNC identifies a

Nit: s/mobile/device and decision.

::: dom/push/src/PushService.jsm
@@ +670,5 @@
> +  handle: function(name, result) {
> +    if (name !== "ril.data.apnSettings" || !result) {
> +      return;
> +    }
> +    let apn = result[0].filter(function(e) {

See comment 9 about result potentially being null.

@@ +1455,5 @@
>      }
>    },
>  
> +  /**
> +   * This method shall be called only if the device is on a mobile network!

Nit: s/shall/should

@@ +1598,5 @@
> +    debug("[_getMobileNetworkId:queryDNSForDomain] Getting mobile network ID (I'm " +
> +       gDNSService.myHostName + ")");
> +
> +    let netidAddress = prefs.get("udp.well-known_netidAddress");
> +    if (netidAddress.endsWith(".") && this._apnDomain) {

The apnDomain check here should be removed. If I understand correctly, you want to say:
if (address ends with '.') {
  if we can get an apnDomain, use that plus netidAddress, otherwise abort
} else (address does not end with '.', but is present) {
  assume FQDN
}

Comment 14

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #13)
> Comment on attachment 8436434 [details] [diff] [review]
> Patch V3
> 
> Review of attachment 8436434 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed.
> 

Thank your Nikhil, I'll fix these last comments just now.

> 
> ::: dom/push/src/PushService.jsm
> @@ +670,5 @@
> > +  handle: function(name, result) {
> > +    if (name !== "ril.data.apnSettings" || !result) {
> > +      return;
> > +    }
> > +    let apn = result[0].filter(function(e) {
> 
> See comment 9 about result potentially being null.
> 

Yes, that's the reason !result was added to the previous check...

> 
> @@ +1598,5 @@
> > +    debug("[_getMobileNetworkId:queryDNSForDomain] Getting mobile network ID (I'm " +
> > +       gDNSService.myHostName + ")");
> > +
> > +    let netidAddress = prefs.get("udp.well-known_netidAddress");
> > +    if (netidAddress.endsWith(".") && this._apnDomain) {
> 
> The apnDomain check here should be removed. If I understand correctly, you
> want to say:
> if (address ends with '.') {
>   if we can get an apnDomain, use that plus netidAddress, otherwise abort
> } else (address does not end with '.', but is present) {
>   assume FQDN
> }

Yes, I added the check bellow and I forgot to remove it here...
Final result will be:

+    if (netidAddress.endsWith(".")) {
+      if (this._apnDomain) {
+        queryDNSForDomain(netidAddress + this._apnDomain, callback);
+      } else {
+        callback(null);   // No netid could be recovered
+      }
+    } else if(netidAddress) {
+      queryDNSForDomain(netidAddress, callback);
+    }

Regards

Comment 15

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #14)
> > 
> > ::: dom/push/src/PushService.jsm
> > @@ +670,5 @@
> > > +  handle: function(name, result) {
> > > +    if (name !== "ril.data.apnSettings" || !result) {
> > > +      return;
> > > +    }
> > > +    let apn = result[0].filter(function(e) {
> > 
> > See comment 9 about result potentially being null.
> > 
> 
> Yes, that's the reason !result was added to the previous check...

I'm sorry, I didn't notice that.

Comment 16

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Patch V4

r+ on https://bugzilla.mozilla.org/show_bug.cgi?id=1018088#c13

Nits addressed.

Comment 17

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

https://tbpl.mozilla.org/?tree=Try&rev=55d73bf7e014

Comment 18

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #17)
> https://tbpl.mozilla.org/?tree=Try&rev=55d73bf7e014

Not much point in running this on try since it's pretty much impossible to test on try ;) Please ask for QA on this. At least to test all the code paths.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/d22673bf54dd

Comment 20

[bhavana bajaj [:bajaj]]

Its a feature request not a release blocking issue so clearing the nom. Feel free to nominate it for uplift and we can take it for 2.0 depending on the risk

Comment 21

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 8436730 [details] [diff] [review]
Patch V4

This feature is needed to be able to use the WakeUp platform in carriers with more than one mobile network

Bug caused by (feature/regressing bug #): -
User impact if declined: We cann't deploy in some countries like USA, Brasil, UK, Germany, ...
Testing completed (on m-c, etc.): Yes
Risk to taking this patch (and alternatives if risky): Low risk, is isolated only for Push servers on carriers with wakeup feature enabled
String or IDL/UUID changes made by this patch: None

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Sorry, somehow this bug got missed when it was merged to m-c.
https://hg.mozilla.org/mozilla-central/rev/d22673bf54dd

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #21)
> Testing completed (on m-c, etc.): Yes
This seems difficult to test. Can you elaborate on the testing that you have completed? Has QA been able to verify?

Comment 24

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Lawrence Mandel [:lmandel] from comment #23)
> (In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from
> comment #21)
> > Testing completed (on m-c, etc.): Yes
> This seems difficult to test. Can you elaborate on the testing that you have
> completed? Has QA been able to verify?

The push code hasn't unit tests yet, also it will be refactored in a near future in favor of HTTP2 instead WebSockets.

Current tests were manual ones, using an own DNS server to check the mobile responses under different circunstances (record found, record not found, ...)

Comment 25

[Lawrence Mandel [:lmandel] (use needinfo)]

Asking some further questions as I want to be careful about what we're landing on 2.0 at this point.

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #21)
> User impact if declined: We cann't deploy in some countries like USA,
> Brasil, UK, Germany, ...

Further questions. Can you clarify what "can't deploy" means in this context? Does this mean that the feature won't work in these countries? That the OS in not viable in these countries? If the latter, is this a regression?

> Risk to taking this patch (and alternatives if risky): Low risk, is isolated
> only for Push servers on carriers with wakeup feature enabled

I see some code refactoring and cleanup. Any chance of this breaking existing functionality or is all of this code isolated for the path with Push servers on carriers with wakeup feature enabled?

> The push code hasn't unit tests yet, also it will be refactored in a near
> future in favor of HTTP2 instead WebSockets.

How near future? Is it preferable to wait for the refactor rather than take this patch and the associated support cost?

> Current tests were manual ones, using an own DNS server to check the mobile
> responses under different circunstances (record found, record not found, ...)

Thanks for clarifying.

Comment 26

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Lawrence Mandel [:lmandel] from comment #25)
> Asking some further questions as I want to be careful about what we're
> landing on 2.0 at this point.
> 
> (In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from
> comment #21)
> > User impact if declined: We cann't deploy in some countries like USA,
> > Brasil, UK, Germany, ...
> 
> Further questions. Can you clarify what "can't deploy" means in this
> context? Does this mean that the feature won't work in these countries? That
> the OS in not viable in these countries? If the latter, is this a regression?
> 

I'll work push but with WebSocket protocol only and cann't take the wakeup advantage.

> > Risk to taking this patch (and alternatives if risky): Low risk, is isolated
> > only for Push servers on carriers with wakeup feature enabled
> 
> I see some code refactoring and cleanup. Any chance of this breaking
> existing functionality or is all of this code isolated for the path with
> Push servers on carriers with wakeup feature enabled?
> 

Existing functionality is not broken and this patch is fully compatible with previous protocol

> > The push code hasn't unit tests yet, also it will be refactored in a near
> > future in favor of HTTP2 instead WebSockets.
> 
> How near future? Is it preferable to wait for the refactor rather than take
> this patch and the associated support cost?
> 

This is WIP at IETF, I don't know when it will be refactored for 2.1, 2.2 or 2.x.
We cann't wait so this path should be uplifted for 2.0

> > Current tests were manual ones, using an own DNS server to check the mobile
> > responses under different circunstances (record found, record not found, ...)
> 
> Thanks for clarifying.

You're welcome

Comment 27

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8436730 [details] [diff] [review]
Patch V4

Aurora approval granted.

Comment 28

[Lawrence Mandel [:lmandel] (use needinfo)]

Does this issue impact b2g branches prior to 2.0?

Comment 29

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Lawrence Mandel [:lmandel] from comment #28)
> Does this issue impact b2g branches prior to 2.0?

No, 2.0 should be the first release with netid support.
Thank you

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7a707f09673f