Closed Bug 980846 Opened 11 years ago Closed 11 years ago

Second wakeup message does not send the port

Categories

(Core :: DOM: Push Subscriptions, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

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)
Status: ASSIGNED → RESOLVED
Closed: 11 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.

Attachment

General

Created:
Updated:
Size: