Closed Bug 955540 Opened 10 years ago Closed 10 years ago

Support Server Pings

Categories

(Chat Core :: Yahoo! Messenger, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: qheaden, Assigned: qheaden)

References

Details

Attachments

(1 file, 2 obsolete files)

*** Original post on bio 2102 at 2013-08-12 19:08:00 UTC ***

Yahoo! Messenger clients ping the pager server they are connected to periodically to inform the server that they are still alive and wish to maintain the TCP connection. Our plug-in needs to do the same.
Assignee: nobody → qheaden
Status: NEW → ASSIGNED
*** Original post on bio 2102 at 2013-09-02 19:19:19 UTC ***

After looking at the libpurple source for this, it seems that there are two packets sent: keepalive and ping. It seems that the timeout for a keepalive packet is 1 miniute (60 seconds), and the timeout for a ping is 1 hour (3600 seconds) as defined here http://lxr.instantbird.org/instantbird/source/purple/libpurple/protocols/yahoo/libymsg.c#57.

The actual sending of the packets happen here http://lxr.instantbird.org/instantbird/source/purple/libpurple/protocols/yahoo/libymsg.c#4926, and it is explained that sending a second ping before 1 hour will cause the servers to disconnect the client.

I'm just adding this comment for later reference. :)
Attached patch Patch 1 (obsolete) — Splinter Review
*** Original post on bio 2102 as attmnt 2814 at 2013-09-02 22:23:00 UTC ***

I tested this patch by watching the packets in WireShark, and I can verify that the KeepAlive and ping packets are being sent, and they are well-formed.
Attachment #8354584 - Flags: review?(clokep)
Comment on attachment 8354584 [details] [diff] [review]
Patch 1

*** Original change on bio 2102 attmnt 2814 at 2013-09-03 13:28:15 UTC ***

Shouldn't the keep-alive packets be using the abstracted code in socket.jsm?
Attachment #8354584 - Flags: review?(clokep) → review-
Blocks: 955574
*** Original post on bio 2102 at 2013-09-10 03:39:32 UTC ***

(In reply to comment #3)
> Comment on attachment 8354584 [details] [diff] [review] (bio-attmnt 2814) [details]
> Patch 1
> 
> Shouldn't the keep-alive packets be using the abstracted code in socket.jsm?

I'm not so sure that the socket.jsm ping code would fit the bill for the Yahoo protocol. First, Yahoo sends both ping and KeepAlive packets. libpurple sends KeepAlives every minute, but I don't think there is a strict timing requirement for them. Pings on the other hand cannot be sent at a rate higher than 1 per hour. According to the libpurple comments, sending more than one ping per hour will cause the servers to disconnect the client.

Looking at the ping code in socket.jsm, it is designed to send pings only when no data has been received in a certain length of time. libpurple's Yahoo impelmentation sends pings even when messages are being sent back and forth. I'm not sure if this is a bug or not, but in copying that functionality, I think we need to keep the patch as-is and use separate timers.
Comment on attachment 8354584 [details] [diff] [review]
Patch 1

*** Original change on bio 2102 attmnt 2814 at 2013-09-11 10:49:04 UTC ***

I assume you wanted to re-request review on this from your latest comment.
Attachment #8354584 - Flags: review- → review?
Comment on attachment 8354584 [details] [diff] [review]
Patch 1

*** Original change on bio 2102 attmnt 2814 at 2013-09-11 17:38:41 UTC ***

>diff --git a/chat/protocols/yahoo/yahoo.js b/chat/protocols/yahoo/yahoo.js
>+// These timeouts are in seconds.
>+const kKeepAliveTimeout = 60; // One minute.
>+const kPingTimeout = 3600; // One hour.
I'd prefer these in milliseconds instead of multiplying by 1000 below. (60 * 1000 is fine though!)

>@@ -211,16 +217,22 @@ YahooAccount.prototype = {
>       this.reportDisconnecting(Ci.prplIAccount.NO_ERROR, "");
>       if (this._session.isConnected)
>         this._session.disconnect();
>       this.reportDisconnected();
>     }
>     // buddy[1] is the actual object.
>     for (let buddy of this._buddies)
>       buddy[1].setStatus(Ci.imIStatusInfo.STATUS_UNKNOWN, "");
>+
>+    // No need for anymore pinging.
I think this comment should really be more of "Clear the ping timers to avoid leaks."

>@@ -425,16 +437,36 @@ YahooAccount.prototype = {
>+  // Callbacks.
>+  onLoginComplete: function() {
>+    // Now that we are connected, get ready to start to sending pings and
>+    // keepalive packets.
>+    this._keepAliveTimer = Cc["@mozilla.org/timer;1"]
>+                           .createInstance(Components.interfaces.nsITimer);
>+    this._pingTimer = Cc["@mozilla.org/timer;1"]
>+                      .createInstance(Components.interfaces.nsITimer);
Nit: The spacing is wrong on both of these, line up the . and the [. Do we have Ci = Components.interfaces defined already?

>+    let s = this._session;
>+    this._keepAliveTimer
>+        .initWithCallback(s.sendKeepAlive.bind(s), kKeepAliveTimeout * 1000,
>+                          this._keepAliveTimer.TYPE_REPEATING_SLACK);
I think this is correct as is, but what's the reasoning behind TYPE_REPEATING_SLACK instead of TYPE_REPEATING_PRECISE or TYPE_REPEATING_PRECISE_CAN_SKIP?
Attachment #8354584 - Flags: review? → review-
Attached patch Patch 2 (obsolete) — Splinter Review
*** Original post on bio 2102 as attmnt 2875 at 2013-09-12 07:44:00 UTC ***

Addressing review feedback.
Attachment #8354645 - Flags: review?(clokep)
Comment on attachment 8354584 [details] [diff] [review]
Patch 1

*** Original change on bio 2102 attmnt 2814 at 2013-09-12 07:44:40 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354584 - Attachment is obsolete: true
Comment on attachment 8354645 [details] [diff] [review]
Patch 2

*** Original change on bio 2102 attmnt 2875 at 2013-09-12 12:09:05 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354645 - Flags: review?(clokep) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 2102 at 2013-09-22 23:10:49 UTC ***

Attachment 8354645 [details] [diff] (bio-attmnt 2875) doesn't apply cleanly:
applying attachment.cgi?id=2875
patching file chat/protocols/yahoo/yahoo-session.jsm
Hunk #3 FAILED at 346
1 out of 3 hunks FAILED -- saving rejects to file chat/protocols/yahoo/yahoo-session.jsm.rej
abort: patch failed to apply


Please also include "JS-Yahoo: " in the commit message for the updated patch. Thanks!
Whiteboard: [checkin-needed]
Attached patch Patch 3Splinter Review
*** Original post on bio 2102 as attmnt 2900 at 2013-09-24 01:58:00 UTC ***

Here is the new un-bitrotted patch.
Attachment #8354670 - Flags: review?(florian)
Comment on attachment 8354645 [details] [diff] [review]
Patch 2

*** Original change on bio 2102 attmnt 2875 at 2013-09-24 01:58:38 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354645 - Attachment is obsolete: true
Comment on attachment 8354670 [details] [diff] [review]
Patch 3

*** Original change on bio 2102 attmnt 2900 at 2013-09-24 10:18:58 UTC ***

If all you're doing is fixing context, you can usually just carry a review forward.
Attachment #8354670 - Flags: review?(florian) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 2102 at 2013-09-25 00:33:49 UTC ***

http://hg.instantbird.org/instantbird/rev/6b1fa55e89a3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
You need to log in before you can comment on or make changes to this bug.