1018164 - PUSH wakeup broken

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

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

Regression: Push agent doesn't sent the required wakeup parameters since IP is not found here.

  http://mxr.mozilla.org/mozilla-central/source/dom/push/src/PushService.jsm#1432

which should be recovered here:
http://mxr.mozilla.org/mozilla-central/source/dom/push/src/PushService.jsm#1500

But nm.active.ip doesn't exists anymore.
After some investigation I found that the nsINetworkManager changed the interface in this bug: 978709

"ip" parameter removed here:
https://github.com/mozilla/mozilla-central/commit/241d5797e409a400333fa46b36e8e76127231a66#diff-3b38ddb13a1e2797599d040fb1b0d873L51

Comment 1

[Maria Angeles Oteo (:oteo)]

Note for triages: We need to block on this and fix it, because wakeup is not working

Comment 2

[Doug Turner (:dougt)]

frsela, can you pick up?

Comment 3

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

(In reply to Doug Turner (:dougt) from comment #2)
> frsela, can you pick up?

Hi Doug, Ok, I'll work on it.

Comment 4

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

Attachment: Proposed patch

Comment 5

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

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

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

The code itself looks good, but I'm concerned because of the odd interface to access values. Clearing review for now.
If you could answer the questions and ask for review again, I'll take a look immediately.

::: dom/push/src/PushService.jsm
@@ +1495,5 @@
>          if (iccInfo) {
>            debug("Running on mobile data");
> +          let ips = {};
> +          let prefixLengths = {};
> +          nm.active.getAddresses(ips, prefixLengths);

Wow! I've never seen such 'C-like' JS code before :)
What is prefixLengths?

@@ +1500,4 @@
>            return {
>              mcc: iccInfo.mcc,
>              mnc: iccInfo.mnc,
> +            ip:  ips.value[0]

Is value guaranteed to be non-null? Otherwise this will throw an exception if there is no IP in which case you should add checks.

Comment 6

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

Hi Nikhil,
Thank you for your feedback. Responses inline:


(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #5)
> Comment on attachment 8432387 [details] [diff] [review]
> Proposed patch
> 
> Review of attachment 8432387 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The code itself looks good, but I'm concerned because of the odd interface
> to access values. Clearing review for now.
> If you could answer the questions and ask for review again, I'll take a look
> immediately.
> 

The new interface is defined into this IDL: http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsINetworkManager.idl#58

> ::: dom/push/src/PushService.jsm
> @@ +1495,5 @@
> >          if (iccInfo) {
> >            debug("Running on mobile data");
> > +          let ips = {};
> > +          let prefixLengths = {};
> > +          nm.active.getAddresses(ips, prefixLengths);
> 
> Wow! I've never seen such 'C-like' JS code before :)

Yeap, I thought exactly the same when I saw it, anyway it's used in other places in the same way:
  http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/NetworkManager.js#342
  http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/NetworkService.js#267

> What is prefixLengths?
> 

Into my device it returns 32 so I think is the CIDR mask.
This isn't needed for our agent but if you don't pass this parameter, the function launches an exception.

> @@ +1500,4 @@
> >            return {
> >              mcc: iccInfo.mcc,
> >              mnc: iccInfo.mnc,
> > +            ip:  ips.value[0]
> 
> Is value guaranteed to be non-null? Otherwise this will throw an exception
> if there is no IP in which case you should add checks.

If the code enters here it means there is a mobile connection since there is a check before. Anyway if this fails, the exception will be launched and it will work with the fallback solution (as if the mobile is in wifi network).

So into the worst case, the ip value will be null and it'll exit here:
http://mxr.mozilla.org/mozilla-central/source/dom/push/src/PushService.jsm#1432

Comment 7

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

Attachment: Patch to fix the IP address recovery

Same patch as https://bugzilla.mozilla.org/attachment.cgi?id=8432387 with only fixed the comment (added reviewer)

Previous patch has r+ by :nsm so I r+ this one too.

Comment 8

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

Try server running: https://tbpl.mozilla.org/?tree=Try&rev=6af919df6b78

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8433903 [details] [diff] [review]
Patch to fix the IP address recovery

Please stick to just using checkin-needed. It's less cumbersome for our bug marking tools. Also, try to be conscientious about the platforms/test suites you invoke on your Try pushes. "All" runs consume over 300hr of machine time and contribute to backlogs felt by all developers. Thanks :)
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66fb92de1240

I also pushed this with a tweaked commit message to hopefully say what the patch is doing. Keep in mind that in general, the commit message should be doing that instead of restating the bug it's fixing :)
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/66fb92de1240

Comment 12

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #9)
> Comment on attachment 8433903 [details] [diff] [review]
> Patch to fix the IP address recovery
> 
> Please stick to just using checkin-needed. It's less cumbersome for our bug
> marking tools. Also, try to be conscientious about the platforms/test suites
> you invoke on your Try pushes. "All" runs consume over 300hr of machine time
> and contribute to backlogs felt by all developers. Thanks :)
> https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Thank your Ryan,
I'll take that into account for the future ;)
Regards

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Does this need to be uplifted to v1.4?  When did the regression occur?  I recently have noticed I don't get push notifications with the line app while dogfooding v1.4.

Comment 14

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

(In reply to Ben Kelly [:bkelly] from comment #13)
> Does this need to be uplifted to v1.4?  When did the regression occur?  I
> recently have noticed I don't get push notifications with the line app while
> dogfooding v1.4.

Bug 978709 which made the change that this bug specifically fixes was not uplifted to 1.4.

Comment 15

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

(In reply to Ben Kelly [:bkelly] from comment #13)
> Does this need to be uplifted to v1.4?  When did the regression occur?  I
> recently have noticed I don't get push notifications with the line app while
> dogfooding v1.4.

As Nikhil said, the bug 978709 is a 2.0 feature, and also this bug broke the "wake up" feature, not all PUSH system.

Which Push server are your device using?

If it continues failing to you, please, open a new bug providing PUSH traces. To enable them, flash your own Gaia build adding the file:

 build/config/custom-prefs.js

With this line:

  pref("services.push.debug", true);

Thank you !