Status

()

defect
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: frsela, Assigned: frsela)

Tracking

({regression})

unspecified
mozilla32
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0+, b2g-v2.0 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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
blocking-b2g: --- → 2.0?
Keywords: regression
Note for triages: We need to block on this and fix it, because wakeup is not working
frsela, can you pick up?
blocking-b2g: 2.0? → 2.0+
Flags: needinfo?(frsela)
(In reply to Doug Turner (:dougt) from comment #2)
> frsela, can you pick up?

Hi Doug, Ok, I'll work on it.
Assignee: nobody → frsela
Flags: needinfo?(frsela)
Posted patch Proposed patch (obsolete) — Splinter Review
Attachment #8432387 - Flags: review?(nsm.nikhil)
Attachment #8432387 - Flags: feedback?(dougt)
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.
Attachment #8432387 - Flags: review?(nsm.nikhil)
Flags: needinfo?(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
Flags: needinfo?(frsela)
Flags: needinfo?(nsm.nikhil)
Attachment #8432387 - Flags: review+
Flags: needinfo?(nsm.nikhil)
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.
Attachment #8432387 - Attachment is obsolete: true
Attachment #8432387 - Flags: feedback?(dougt)
Attachment #8433903 - Flags: review+
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
Attachment #8433903 - Flags: checkin?
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
https://hg.mozilla.org/mozilla-central/rev/66fb92de1240
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(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
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.
(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.
(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 !
You need to log in before you can comment on or make changes to this bug.