1302447 - Implement IRCv3.2 server-time capability

Status: RESOLVED FIXED

(Chat Core :: IRC, enhancement)

Description

[Martin Giger [:freaktechnik]]

Implement the server-time capability as http://ircv3.net/specs/extensions/server-time-3.2.html specifies.

It should also request znc.in/server-time-iso, since that's what ZNC used before the standard was created and the tag it adds works exactly the same from what I can find.

Comment 1

[Martin Giger [:freaktechnik]]

While testing the time stamps from the spec with my draft patch, I noticed, that leap seconds will result in an "Invalid Date" time instead of an actual time. I wonder what the best thing to do here would be. Either the leap second is ignored (which means parsing the date string and removing the leap second), or the server time is ignored.

Comment 2

[Martin Giger [:freaktechnik]]

Attachment: bug1302447-taghandler.patch

This patch adds handler infrastructure to handle tags independent of the command they are on.

Comment 3

[Martin Giger [:freaktechnik]]

Attachment: bug1302447-server-time.patch

This patch implements the capability negotiation and parsing of the tag. Leap seconds are rounded down to the previous non-leap second.

Comment 4

[Patrick Cloke [:clokep]]

Comment on attachment 8791148 [details] [diff] [review]
bug1302447-taghandler.patch

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

This is not nearly as gross as I thought it was going to be! I wish there was a way we didn't have to pass tags all over the place though...

::: chat/protocols/irc/irc.js
@@ +179,5 @@
>                        " :\r\n";
>      return this._account.maxMessageLength -
>             this._account.countBytes(baseMessage);
>    },
> +  writeMessage: function(aWho, aMessage, aObject) {

aObject here is an IRC message object?

@@ +193,5 @@
> +        ircHandlers.handleTag(this._account, new TagMessage(msg, tag));
> +      }
> +
> +      delete msg.originalMessage;
> +      aObject = msg;

Why reassign msg to aObject? This seems confusing.

@@ +196,5 @@
> +      delete msg.originalMessage;
> +      aObject = msg;
> +    }
> +    // Remove the IRC tags, as those were added just for this step.
> +    delete aObject.tags;

I think what you're trying to say here is to delete tags since they're an IRC only thing?

Comment 5

[Patrick Cloke [:clokep]]

Comment on attachment 8791149 [details] [diff] [review]
bug1302447-server-time.patch

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

Thanks for writing tests. :)

::: chat/protocols/irc/ircServerTime.jsm
@@ +20,5 @@
> +
> +  commands: {
> +    "time": function(aMsg) {
> +      // Normalize leap seconds to the next second before it.
> +      const time = aMsg.tagValue.replace(/60.\d{3}(?=Z$)/, "59.999");

This is because JavaScript doesn't support leap seconds?

@@ +40,5 @@
> +      }
> +      return true;
> +    },
> +    "znc.in/server-time-iso": function(aMessage) {
> +      if(aMessage.cap.subcommand == "LS") {

Nit: (Across both patches...) Space after "if", before "(".

::: chat/protocols/irc/test/test_serverTime.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +Components.utils.import("resource:///modules/ircServerTime.jsm");
> +
> +function getTags(aRawMsg) {

It'd be nice if we could use the "real" parsing for this.

@@ +52,5 @@
> +      tagName: "time",
> +      tagValue: msg.tags.get("time")
> +    };
> +    tagServerTime.commands.time(tagMessage);
> +    for(let i in msg) {

Nit: Space after for.

@@ +53,5 @@
> +      tagValue: msg.tags.get("time")
> +    };
> +    tagServerTime.commands.time(tagMessage);
> +    for(let i in msg) {
> +      do_check_eq(tagMessage.message[i], msg[i]);

I've been trying to use the Assert.jsm ones now! Let's use those for new code.

Comment 6

[Martin Giger [:freaktechnik]]

(In reply to Patrick Cloke [:clokep] from comment #4)
> This is not nearly as gross as I thought it was going to be! I wish there
> was a way we didn't have to pass tags all over the place though...

They are being passed all over the place.
 
> ::: chat/protocols/irc/irc.js
> @@ +179,5 @@
> >                        " :\r\n";
> >      return this._account.maxMessageLength -
> >             this._account.countBytes(baseMessage);
> >    },
> > +  writeMessage: function(aWho, aMessage, aObject) {
> 
> aObject here is an IRC message object?

No, it is a plain object passed in from the handlers. It contains properties to set on the imMessage. It defines things like "incoming" or "system". I added "tags" to all call sites that might be interesting for tag handlers, which is why I had to modify ircCommands.jsm. And why I delete the property later on.

 
> @@ +193,5 @@
> > +        ircHandlers.handleTag(this._account, new TagMessage(msg, tag));
> > +      }
> > +
> > +      delete msg.originalMessage;
> > +      aObject = msg;
> 
> Why reassign msg to aObject? This seems confusing.

This is to preserve potential changes/additions handlers make to the imMessage. I will add a comment explaining that.

> @@ +196,5 @@
> > +      delete msg.originalMessage;
> > +      aObject = msg;
> > +    }
> > +    // Remove the IRC tags, as those were added just for this step.
> > +    delete aObject.tags;
> 
> I think what you're trying to say here is to delete tags since they're an
> IRC only thing?

No, that the tags are passed in on aObject just for the tag handling. I've slightly reworded the comment to make it more obvious.

When I've been able to test an updated version of the server time patch, I'll also attach a patch that has the updated comments.

Comment 7

[Martin Giger [:freaktechnik]]

(In reply to Martin Giger [:freaktechnik] from comment #6)
> added "tags" to all call sites that might be interesting for tag handlers,
> which is why I had to modify ircCommands.jsm.
 Sorry, not ircCommands.jsm; ircBase.jsm, ircCTCP.jsm and ircNonStandard.jsm are the ones affected by that. It means an IRC message handler can decide whether or not the tag handlers should be ran on the message by passing in the tags property on the object or omitting it.

Comment 8

[Martin Giger [:freaktechnik]]

Attachment: bug1302447-taghandler-v2.patch

Tried to re-word some comments and add some to clarify how tag handling works. Fixed up code style.

Comment 9

[Martin Giger [:freaktechnik]]

Attachment: bug1302447-server-time-v2.patch

> This is because JavaScript doesn't support leap seconds?

Exactly. It's the best method I could come up with to handle them. And since unix timestamps don't have leap seconds either and just do time dilation this is the rightest way to do it.

Switched to Assert.jsm and addressed code style nits.

Comment 10

[Martin Giger [:freaktechnik]]

Attachment: bug1302447-server-time-v2.1.patch

Noticed that I had the wrong license header in the test.

Comment 11

[Patrick Cloke [:clokep]]

Comment on attachment 8807836 [details] [diff] [review]
bug1302447-server-time-v2.1.patch

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

::: chat/protocols/irc/test/test_serverTime.js
@@ +2,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +Components.utils.import("resource:///modules/ircServerTime.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://testing-common/Assert.jsm");

You don't need to import Assert.jsm, you can just directly use the methods.

@@ +44,5 @@
> +      message: "John joined #chan",
> +      get originalMessage() { return "John joined #chan"; },
> +      system: true,
> +      incoming: true
> +    }

How hard would it be to add a test for one WITHOUT tags? (And maybe one with tags being an empty list?)

Comment 12

[Martin Giger [:freaktechnik]]

Attachment: bug1302447-server-time-v3.patch

Comment 13

[Martin Giger [:freaktechnik]]

Comment on attachment 8807836 [details] [diff] [review]
bug1302447-server-time-v2.1.patch

># HG changeset patch
># User Martin Giger <[e-mail]>
># Parent  44ddb16812f8bfbda141e7f3f0defa051cee42d6
>Bug 1302447 - Add IRCv3.2 server-time capability support r?clokep
>
>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
>--- a/chat/protocols/irc/irc.js
>+++ b/chat/protocols/irc/irc.js
>@@ -1947,6 +1947,7 @@ function ircProtocol() {
>   Cu.import("resource:///modules/ircMultiPrefix.jsm", tempScope);
>   Cu.import("resource:///modules/ircNonStandard.jsm", tempScope);
>   Cu.import("resource:///modules/ircSASL.jsm", tempScope);
>+  Cu.import("resource:///modules/ircServerTime.jsm", tempScope);
>   Cu.import("resource:///modules/ircWatchMonitor.jsm", tempScope);
> 
>   // Register default IRC handlers (IRC base, CTCP).
>@@ -1973,6 +1974,8 @@ function ircProtocol() {
>   ircHandlers.registerISUPPORTHandler(tempScope.isupportMONITOR);
>   ircHandlers.registerHandler(tempScope.ircSASL);
>   ircHandlers.registerCAPHandler(tempScope.capSASL);
>+  ircHandlers.registerCAPHandler(tempScope.capServerTime);
>+  ircHandlers.registerTagHandler(tempScope.tagServerTime);
> }
> ircProtocol.prototype = {
>   __proto__: GenericProtocolPrototype,
>diff --git a/chat/protocols/irc/ircServerTime.jsm b/chat/protocols/irc/ircServerTime.jsm
>new file mode 100644
>--- /dev/null
>+++ b/chat/protocols/irc/ircServerTime.jsm
>@@ -0,0 +1,49 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+/*
>+ * This implements server-time for IRC.
>+ *   http://ircv3.net/specs/extensions/server-time-3.2.html
>+ */
>+
>+this.EXPORTED_SYMBOLS = ["capServerTime", "tagServerTime"];
>+
>+const Cu = Components.utils;
>+
>+Cu.import("resource:///modules/ircHandlers.jsm");
>+
>+var tagServerTime = {
>+  name: "server-time Tags",
>+  priority: ircHandlers.DEFAULT_PRIORITY,
>+  isEnabled: () => true,
>+
>+  commands: {
>+    "time": function(aMsg) {
>+      // Normalize leap seconds to the next second before it.
>+      const time = aMsg.tagValue.replace(/60.\d{3}(?=Z$)/, "59.999");
>+      aMsg.message.time = Math.floor(Date.parse(time) / 1000);
>+      aMsg.message.delayed = true;
>+    }
>+  }
>+};
>+
>+var capServerTime = {
>+  name: "server-time CAP",
>+  priority: ircHandlers.DEFAULT_PRIORITY,
>+  isEnabled: () => true,
>+
>+  commands: {
>+    "server-time": function(aMessage) {
>+      if (aMessage.cap.subcommand == "LS") {
>+        this.sendMessage("CAP", ["REQ", "server-time"]);
>+      }
>+      return true;
>+    },
>+    "znc.in/server-time-iso": function(aMessage) {
>+      if (aMessage.cap.subcommand == "LS") {
>+        this.sendMessage("CAP", ["REQ", "znc.in/server-time-iso"]);
>+      }
>+    }
>+  }
>+};
>diff --git a/chat/protocols/irc/moz.build b/chat/protocols/irc/moz.build
>--- a/chat/protocols/irc/moz.build
>+++ b/chat/protocols/irc/moz.build
>@@ -21,6 +21,7 @@ EXTRA_JS_MODULES += [
>     'ircMultiPrefix.jsm',
>     'ircNonStandard.jsm',
>     'ircSASL.jsm',
>+    'ircServerTime.jsm',
>     'ircServices.jsm',
>     'ircUtils.jsm',
>     'ircWatchMonitor.jsm',
>diff --git a/chat/protocols/irc/test/test_serverTime.js b/chat/protocols/irc/test/test_serverTime.js
>new file mode 100644
>--- /dev/null
>+++ b/chat/protocols/irc/test/test_serverTime.js
>@@ -0,0 +1,71 @@
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/ */
>+
>+Components.utils.import("resource:///modules/ircServerTime.jsm");
>+Components.utils.import("resource://gre/modules/Services.jsm");
>+Components.utils.import("resource://testing-common/Assert.jsm");
>+
>+var irc = {};
>+Services.scriptloader.loadSubScript("resource:///components/irc.js", irc);
>+
>+var assert = new Assert();
>+assert.setReporter(function customReporter(err, message, stack) {
>+  if (err) {
>+    do_report_result(false, err.message, err.stack);
>+  } else {
>+    do_report_result(true, message, stack);
>+  }
>+});
>+
>+function getTags(aRawMsg) {
>+  const { tags } = irc.ircMessage(aRawMsg);
>+
>+  return tags;
>+}
>+
>+function run_test() {
>+  add_test(specMessages);
>+
>+  run_next_test();
>+}
>+
>+function specMessages() {
>+  const kMessages = [
>+    {
>+      tags: getTags("@time=2011-10-19T16:40:51.620Z :Angel![e-mail] PRIVMSG #test :Hello"),
>+      who: "Angel![e-mail]",
>+      get originalMessage() { return "Hello"; },
>+      message: "Hello",
>+      incoming: true
>+    },
>+    {
>+      tags: getTags("@time=2012-06-30T23:59:60.419Z :!John!~john@1.2.3.4 JOIN #chan"),
>+      who: "John!~john@1.2.3.4",
>+      message: "John joined #chan",
>+      get originalMessage() { return "John joined #chan"; },
>+      system: true,
>+      incoming: true
>+    }
>+  ];
>+
>+  const kExpectedTimes = [
>+    Math.floor(Date.parse(kMessages[0].tags.get("time")) / 1000),
>+    Math.floor(Date.parse("2012-06-30T23:59:59.999Z") / 1000)
>+  ];
>+
>+  for (let m in kMessages) {
>+    const msg = kMessages[m];
>+    const tagMessage = {
>+      message: Object.assign({}, msg),
>+      tagName: "time",
>+      tagValue: msg.tags.get("time")
>+    };
>+    tagServerTime.commands.time(tagMessage);
>+    for (let i in msg) {
>+      assert.equal(tagMessage.message[i], msg[i]);
>+    }
>+    assert.equal(kExpectedTimes[m], tagMessage.message.time);
>+  }
>+
>+  run_next_test();
>+}
>diff --git a/chat/protocols/irc/test/xpcshell.ini b/chat/protocols/irc/test/xpcshell.ini
>--- a/chat/protocols/irc/test/xpcshell.ini
>+++ b/chat/protocols/irc/test/xpcshell.ini
>@@ -10,6 +10,7 @@ tail =
> [test_ircMessage.js]
> [test_ircNonStandard.js]
> [test_sendBufferedCommand.js]
>+[test_serverTime.js]
> [test_setMode.js]
> [test_splitLongMessages.js]
> [test_tryNewNick.js]

Comment 14

[Patrick Cloke [:clokep]]

Comment on attachment 8808424 [details] [diff] [review]
bug1302447-server-time-v3.patch

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

::: chat/protocols/irc/irc.js
@@ +185,4 @@
>        const msg = Object.assign({
>          who: aWho,
>          message: aMessage,
> +        get originalMessage() {

This should be in the first patch.

::: chat/protocols/irc/test/test_ircServerTime.js
@@ +7,5 @@
> +var irc = {};
> +Services.scriptloader.loadSubScript("resource:///components/irc.js", irc);
> +
> +function getTags(aRawMsg) {
> +  const { tags } = irc.ircMessage(aRawMsg, "doesnt@matter");

I think we don't normally put spaces around this, but I'm unsure.

@@ +68,5 @@
> +      tagName: "time",
> +      tagValue: msg.tags.get("time")
> +    };
> +    tagServerTime.commands.time(tagMessage);
> +    for (let i in msg) {

Can you add a comment above this for loop about what we're checking for equality.

@@ +71,5 @@
> +    tagServerTime.commands.time(tagMessage);
> +    for (let i in msg) {
> +      equal(tagMessage.message[i], msg[i]);
> +    }
> +    equal("time" in tagMessage.message, kExpectedTimes[m] !== undefined);

And above this for what we're checking.

@@ +73,5 @@
> +      equal(tagMessage.message[i], msg[i]);
> +    }
> +    equal("time" in tagMessage.message, kExpectedTimes[m] !== undefined);
> +
> +    if(kExpectedTimes[m] !== undefined) {

Nit: Space after "if".

Comment 15

[Patrick Cloke [:clokep]]

Comment on attachment 8807834 [details] [diff] [review]
bug1302447-taghandler-v2.patch

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

::: chat/protocols/irc/irc.js
@@ +180,4 @@
>      return this._account.maxMessageLength -
>             this._account.countBytes(baseMessage);
>    },
> +  writeMessage: function(aWho, aMessage, aObject) {

Please add a comment above this for what the inputs are (specifically aObject).

@@ +184,5 @@
> +    if ("tags" in aObject && ircHandlers.hasTagHandlers) {
> +      const msg = Object.assign({
> +        who: aWho,
> +        message: aMessage,
> +        get originalMessage {

This has a bug fixed in the next patch.

@@ +185,5 @@
> +      const msg = Object.assign({
> +        who: aWho,
> +        message: aMessage,
> +        get originalMessage {
> +          return aMessage;

Should this be making a copy of aMessage? Why do we need originalMessage here? We seem to create it and then delete it, unless you expect one of the tag handlers to want to access it?

@@ +196,5 @@
> +      }
> +
> +      delete msg.originalMessage;
> +      // Make sure any changes made by the handlers get written to the imMessage.
> +      aObject = msg;

This still seems really confusing to me. I think it'd be clearer if you had an else after the if-statement that said msg = aObject, then just used msg below this line.

Comment 16

[Martin Giger [:freaktechnik]]

Attachment: bug1302447-taghandler-v3.patch

Addresses the nits. I decided not to go for an else due to how scoping works. Alternatively we could just declare messageProps and then conditionally set it.

I think I already know one suggestion you'll have: to not rely on messageProps.who and messageProps.message to just override aWho and aMessage.

Comment 17

[Martin Giger [:freaktechnik]]

Attachment: bug1302447-sever-time-v4.patch

I also added a test for the znc server-time tag.

Comment 18

[Patrick Cloke [:clokep]]

Comment on attachment 8810213 [details] [diff] [review]
bug1302447-taghandler-v3.patch

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

Can you use 8-lines of context next time?

::: chat/protocols/irc/irc.js
@@ +204,5 @@
> +
> +      // Remove helper prop for tag handlers. We don't want to remove the other
> +      // ones, since they might have been changed and will override aWho and
> +      // aMessage in the imMessage constructor.
> +      delete msg.originalMessage;

msg doesn't exist anymore, I think you wanted this to be messageProps?

Comment 19

[Martin Giger [:freaktechnik]]

Attachment: bug1302447-taghandler-v4.patch

Yep, totally forgot that when refactoring to better names.

Comment 20

[Patrick Cloke [:clokep]]

Comment on attachment 8810214 [details] [diff] [review]
bug1302447-sever-time-v4.patch

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

::: chat/protocols/irc/ircServerTime.jsm
@@ +19,5 @@
> +  isEnabled: () => true,
> +
> +  commands: {
> +    "time": function(aMsg) {
> +      if(aMsg.tagValue) {

Nit: This should have a space after "if".

::: chat/protocols/irc/test/test_ircServerTime.js
@@ +36,5 @@
> +      system: true,
> +      incoming: true
> +    },
> +    {
> +      tags: getTags("@znc.in/server-time-iso=2016-11-13T19:20:45.284Z :John!~john@1.2.3.4 JOIN #chan"),

This case is failing for me, I think generating the tag message isn't working correctly...

Comment 21

[Martin Giger [:freaktechnik]]

Attachment: bug1302447-server-time-v5.patch

That test for znc.in/server-time-iso was shoddily copy-pasted, but now everything works as expected, I also verified on my ZNC after seeing that the tag handler wasn't even implemented...

Comment 22

[Martin Giger [:freaktechnik]]

Attachment: bug1302447-server-time-v5.1.patch

Oops, forgot to remove a debugging dump().

Comment 23

[Patrick Cloke [:clokep]]

Comment on attachment 8811999 [details] [diff] [review]
bug1302447-server-time-v5.1.patch

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

::: chat/protocols/irc/ircServerTime.jsm
@@ +13,5 @@
> +
> +Cu.import("resource:///modules/ircHandlers.jsm");
> +
> +function handleServerTimeTag(aMsg) {
> +  if(aMsg.tagValue) {

Nit: Add a space after "if".

::: chat/protocols/irc/test/test_ircServerTime.js
@@ +83,5 @@
> +
> +    // Ensuring that the expected properties and their values as given in
> +    // kMessages are still the same after the handler.
> +    for (let i in msg) {
> +      equal(tagMessage.message[i], msg[i], "Property '"+i+"' was not modified");

Nit: Spaces around the +.

@@ +86,5 @@
> +    for (let i in msg) {
> +      equal(tagMessage.message[i], msg[i], "Property '"+i+"' was not modified");
> +    }
> +    // The time should only be adjusted when we expect a valid server-time tag.
> +    equal("time" in tagMessage.message, kExpectedTimes[m] !== undefined, "Message time was set when expected");

I think we also need to check for the ZNC tag here?

Comment 24

[Martin Giger [:freaktechnik]]

I'll never learn to put spaces after keywords, except maybe import ;)

(In reply to Patrick Cloke [:clokep] from comment #23)
> Comment on attachment 8811999 [details] [diff] [review]
> bug1302447-server-time-v5.1.patch
> 
> Review of attachment 8811999 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +86,5 @@
> > +    for (let i in msg) {
> > +      equal(tagMessage.message[i], msg[i], "Property '"+i+"' was not modified");
> > +    }
> > +    // The time should only be adjusted when we expect a valid server-time tag.
> > +    equal("time" in tagMessage.message, kExpectedTimes[m] !== undefined, "Message time was set when expected");
> 
> I think we also need to check for the ZNC tag here?

Nope, that's checking if the imMessage has the time set, not if the tag exists.

Comment 25

[Martin Giger [:freaktechnik]]

Attachment: bug1302447-server-time-v6.patch

Addressed the two nits on the last patch.

Comment 26

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/7111a1bd9fa8fd043791eb4f4fb9d43ba802239e
https://hg.mozilla.org/comm-central/rev/08af9c37ea6aea0d51253e3fda5837d994f8d7d8

Thanks! Would it make sense to uplift either of these to 52 to be in the ESR?