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)
Tracking
()
People
(Reporter: willyaranda, Assigned: willyaranda)
Details
Attachments
(1 file, 2 obsolete files)
2.24 KB,
patch
|
willyaranda
:
review+
fabrice
:
approval-mozilla-b2g28+
|
Details | Diff | Splinter Review |
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.
Comment 1•11 years ago
|
||
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 | ||
Updated•11 years ago
|
Assignee: nobody → willyaranda
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Assignee | ||
Comment 2•11 years ago
|
||
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 3•11 years ago
|
||
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+
Assignee | ||
Comment 5•11 years ago
|
||
(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?
Assignee | ||
Comment 6•11 years ago
|
||
(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)
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #8387505 -
Attachment is obsolete: true
Attachment #8387712 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 9•11 years ago
|
||
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?
Comment 10•11 years ago
|
||
Please add commit information to the patch.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Keywords: checkin-needed
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #8388558 -
Flags: review+
Attachment #8388558 -
Flags: approval-mozilla-b2g28?
Flags: needinfo?(ryanvm)
Assignee | ||
Updated•11 years ago
|
Attachment #8387712 -
Attachment is obsolete: true
Attachment #8387712 -
Flags: approval-mozilla-b2g28?
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 12•11 years ago
|
||
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)
Comment 13•11 years ago
|
||
Keywords: checkin-needed
Comment 14•11 years ago
|
||
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.
Comment 15•11 years ago
|
||
Blocking as this would is breaking the push notification functionality
blocking-b2g: 1.3? → 1.3+
Comment 16•11 years ago
|
||
(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 ?
Updated•11 years ago
|
Flags: needinfo?(willyaranda)
Comment 17•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Updated•11 years ago
|
Attachment #8388558 -
Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28+
Comment 18•11 years ago
|
||
status-b2g-v1.3:
--- → fixed
status-b2g-v1.4:
--- → fixed
status-firefox28:
--- → wontfix
status-firefox29:
--- → wontfix
status-firefox30:
--- → fixed
Updated•11 years ago
|
status-b2g-v1.3T:
--- → fixed
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(willyaranda)
You need to log in
before you can comment on or make changes to this bug.
Description
•