Closed Bug 980846 Opened 6 years ago Closed 6 years ago

Second wakeup message does not send the port

Categories

(Core :: DOM: Push Notifications, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
blocking-b2g 1.3+
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: willyaranda, Assigned: willyaranda)

Details

Attachments

(1 file, 2 obsolete files)

When device is running in a mobile device, it opens a UDP port to send the port number to the Push Server, so it can be used to be woken up in case of a wakeup UDP server is available.

STR:

1) Open the device with a SIM card and register for push notifications in a app.
2) See the first message sends correctly all the data:

03-07 11:12:41.729 I/Gecko   (  137): -*- PushService.jsm: Sending message: {"messageType":"hello","uaid":"999e2fbe-7bfa-4ae7-8de4-c74b402735be@d0f805836bf8693cd1ef6c7c01156620b77a2b6e","wakeup_hostport":{"ip":"10.74.247.152","port":38981},"mobilenetwork":{"mcc":"214","mnc":"07"},"channelIDs":["02f4c22d-cbe6-4d08-a809-209456241dc9","04fbdd8e-9158-462b-9f68-419cce759cc5","2f058f84-cf4e-4e45-9dcd-3d5ba0b6cf02","45ad2aa6-16d9-49be-ab1b-6071a6ce66ff","77c4d618-c0f0-4c03-be62-f6580b9ce479","79ba749a-cce7-40bd-a1af-79e64062adee","92bb5383-c9a7-46d8-af53-b853a8af2e1b","c4f86ad9-0651-43c5-a882-a902618706f8","de7dd447-02ee-414d-b760-677727d188dc","dfa2a0f7-372e-4ddd-aeb9-35dcc41e461e","f9bb3cfb-b8f7-423c-835c-aeacb07df74a"]}

3) Force-change the network technology (3G to 2G, for example), and see the next message DOES NOT contain the "port" attribute in the "mobilenetwork".

I am working on a patch.
This has to be fixed on 1.3. (either by fixing it or by reverting whatever broke it). Otherwise the wakeup system for push will not be used at all.
blocking-b2g: --- → 1.3?
Assignee: nobody → willyaranda
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Attached patch bug980846.diff (obsolete) — Splinter Review
I'm compiling this right now, but I think this solves. I removed all statements for _udpPort and keep the _udpServer, and access there for the port property, which I think it is better.
Attachment #8387505 - Flags: review?(nsm.nikhil)
Comment on attachment 8387505 [details] [diff] [review]
bug980846.diff

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

::: dom/push/src/PushService.jsm
@@ +1291,4 @@
>      this._retryFailCount = 0;
>  
>      // Openning an available UDP port.
> +    this._listenForUDPWakeup();

If no more needed, we can also remove the return line from _listenForUDPWakeup function since is only used here.
Attachment #8387505 - Flags: feedback+
Comment on attachment 8387505 [details] [diff] [review]
bug980846.diff

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

r=me with comments fixed.

::: dom/push/src/PushService.jsm
@@ +1305,4 @@
>        // Hostport is apparently a thing.
>        data["wakeup_hostport"] = {
>          ip: networkState.ip,
> +        port: this._udpServer.port

If udpserver is uninitialised (udp disabled or various other reasons) this will error.

@@ +1448,4 @@
>     * reconnect the WebSocket and get the actual data.
>     */
>    onPacketReceived: function(aServ, aMessage) {
> +    debug("Recv UDP datagram on port: " + this._udpServer.port);

same here
Attachment #8387505 - Flags: review?(nsm.nikhil) → review+
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #4)
> Comment on attachment 8387505 [details] [diff] [review]

> 
> @@ +1448,4 @@
> >     * reconnect the WebSocket and get the actual data.
> >     */
> >    onPacketReceived: function(aServ, aMessage) {
> > +    debug("Recv UDP datagram on port: " + this._udpServer.port);
> 
> same here

This should work, right? This is called by the UDP Server as a handler to manage received packets, so I think it should mean that there is a valid UDP server, right?
(oops, ni on comment #5)
Status: NEW → ASSIGNED
Flags: needinfo?(nsm.nikhil)
(In reply to Guillermo López (:willyaranda) from comment #5)
> (In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #4)
> > Comment on attachment 8387505 [details] [diff] [review]
> 
> > 
> > @@ +1448,4 @@
> > >     * reconnect the WebSocket and get the actual data.
> > >     */
> > >    onPacketReceived: function(aServ, aMessage) {
> > > +    debug("Recv UDP datagram on port: " + this._udpServer.port);
> > 
> > same here
> 
> This should work, right? This is called by the UDP Server as a handler to
> manage received packets, so I think it should mean that there is a valid UDP
> server, right?

Yes, you're right. So only the one fix.
Flags: needinfo?(nsm.nikhil)
Attached patch V2 (obsolete) — Splinter Review
Attachment #8387505 - Attachment is obsolete: true
Attachment #8387712 - Flags: review+
Comment on attachment 8387712 [details] [diff] [review]
V2

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: WakeUP platform can be used once when the device boots. It requires to reboot the phone to enable it again.
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:
Attachment #8387712 - Flags: approval-mozilla-b2g28?
Attached patch 980846.patchSplinter Review
Attachment #8388558 - Flags: review+
Attachment #8388558 - Flags: approval-mozilla-b2g28?
Flags: needinfo?(ryanvm)
Attachment #8387712 - Attachment is obsolete: true
Attachment #8387712 - Flags: approval-mozilla-b2g28?
Please don't needinfo? for a checkin request unless it's super-urgent (and in that case, an IRC ping is probably still the better option). The keyword is checked on a regular basis. Also, for future reference, your commit message should typically be summarizing what the patch is doing, not restating the bug title:
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

Thanks :)
Flags: needinfo?(ryanvm)
Dear triage drivers, this is a blocker for TEF release( because it allows apps to use push notifications successfully), please set the flag 1.3+
Thanks.
Blocking as this would is breaking the push notification functionality
blocking-b2g: 1.3? → 1.3+
(In reply to Guillermo López (:willyaranda) from comment #9)
> Comment on attachment 8387712 [details] [diff] [review]
> V2
> 
> NOTE: This flag is now for security issues only. Please see
> https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand
> the B2G approval process and landings.
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
> User impact if declined: WakeUP platform can be used once when the device
> boots. It requires to reboot the phone to enable it again.
> Testing completed: 
> Risk to taking this patch (and alternatives if risky): 
> String or UUID changes made by this patch:

Can you please help with information on what sort of testing was completed here ? Did we verify this on master/trunk ? Also how risky is this patch and anything else we can do in addition here to mitigate the risk ? Can we add more unit/integration tests in this area of code ?
Flags: needinfo?(willyaranda)
https://hg.mozilla.org/mozilla-central/rev/efc8568dcf36
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Attachment #8388558 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28+
Flags: needinfo?(willyaranda)
You need to log in before you can comment on or make changes to this bug.