Closed Bug 955655 Opened 10 years ago Closed 10 years ago

Stats ids are not set consistently

Categories

(Instantbird Graveyard :: Other, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

(Whiteboard: [1.5-blocking])

Attachments

(1 file, 1 obsolete file)

*** Original post on bio 2210 at 2013-10-09 20:36:00 UTC ***

Since the buddy name read from the header contains the resource, we create tons of id's for log files from the same XMPP buddies and the same log dir.
Blocks: 955013, 955582
*** Original post on bio 2210 at 2013-10-12 13:43:34 UTC ***

So I've been trying to look into this.

It seems you use account.name everywhere rather than account.normalizedName. This will cause trouble - note logger.js uses the normalizedName.

When sweeping the logs, the data from which the ID is set comes from the header. For whatever reason (probably because we might want to display it in the UI), the header contains the normalizedName for everything but the conv. Hence the bug.

The log folder name comes from the normalizedName of the conv. However, it is then further encoded (encodeName) to make sure it is a valid filename. This means it's not possible to just use the log folder name to get the conv normalizedName when sweeping.

So you have two options for fixing this bug:

1) (Hack) Find out if encodeName is invertible and if so, use the log folder name to get the conv normalizedName on log sweeping. Check with flo if he will r- this before trying ;)

2) (Proper Fix, for which people will be Very Grateful) Fix bug 955553 (bio 2115) and use the result on the name you get from the header. (As far as I could check, the libpurple purple_normalize version for jabber strips the resource.)
*** Original post on bio 2210 at 2013-10-12 20:42:42 UTC ***

(In reply to comment #1)

> 1) (Hack) Find out if encodeName is invertible and if so, use the log folder
> name to get the conv normalizedName on log sweeping. Check with flo if he will
> r- this before trying ;)

Unless there are broken edge cases, I don't think I would r-.

> 2) (Proper Fix, for which people will be Very Grateful) Fix bug 955553 (bio 2115)

Would this work for MUCs?
*** Original post on bio 2210 at 2013-10-14 16:41:55 UTC ***

> Would this work for MUCs?

(1) would need to carefully strip the .chat we add. (2) should be fine, as what is needed here is consistency, and if we use the normalizedName of everything everywhere there should be no problems (fingers crossed).
Depends on: 955553
*** Original post on bio 2210 at 2013-10-30 10:33:33 UTC ***

We also want the normalizedName here instead of the displayName http://lxr.instantbird.org/instantbird/source/instantbird/components/ibConvStatsService.js#672
Whiteboard: [1.5-blocking]
Summary: XMPP buddies with changing resource don't get their stats counted correctly → Stats id's are not set consistently
Summary: Stats id's are not set consistently → Stats ids are not set consistently
*** Original post on bio 2210 at 2013-11-22 07:36:28 UTC ***

Not fully sure what the status is here, but I suspect the ball is in aleth's court; either to write a patch, or explain better what needs to happen ;).
Whiteboard: [1.5-blocking] → [1.5-blocking][needs patch aleth]
*** Original post on bio 2210 at 2013-11-22 11:25:35 UTC ***

(In reply to comment #5)
> Not fully sure what the status is here, but I suspect the ball is in aleth's
> court; either to write a patch, or explain better what needs to happen ;).

It's completely clear what needs to happen. I can't write a patch for this as doing a decent patch for bug 955553 (bio 2115) requires being able to build.
Whiteboard: [1.5-blocking][needs patch aleth] → [1.5-blocking][needs patch]
Assignee: nobody → aleth
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2210 as attmnt 3104 at 2013-12-01 16:22:00 UTC ***

The if (!name)... code really wants to be in logger.js. If someone adds a Services.accounts.getAccountByName(aNormalizedName) API, it could be moved there.
Attachment #8354887 - Flags: review?(florian)
Whiteboard: [1.5-blocking][needs patch] → [1.5-blocking]
Comment on attachment 8354887 [details] [diff] [review]
Patch

*** Original change on bio 2210 attmnt 3104 at 2013-12-01 17:59:49 UTC ***

>diff --git a/instantbird/components/ibConvStatsService.js b/instantbird/components/ibConvStatsService.js

>   sweep: function(aStatsService) {

> 
>+    function getIter(aEnumerator)
>+    {
>+      while (aEnumerator.hasMoreElements())
>+        yield aEnumerator.getNext();
>+    }
>+    let accounts = getIter(Services.accounts.getAccounts());
>+    this._accountMap =
>+      new Map([account.normalizedName, account] for (account of accounts));

This code seems obfuscated to me.


>+          let accountName = header.account;
>+          let name = header.normalizedName;
>+          if (!name) {

This needs a comment saying that the 'normalizedName' for conversations was added in the log files with Ib 1.5, and that for backward compatibility we will re-normalize the name for older log files.


> function PossibleConvFromContact(aContact) {
>   this._displayName = aContact.displayName;
>   this._statusType = aContact.statusType;
>   this._statusText = aContact.statusText;
>-  let buddy = aContact.preferredBuddy;
>   this._contactId = aContact.id;
>-  this.id = getConversationId(buddy.protocol.normalizedName,
>-                              buddy.preferredAccountBuddy.account.name,
>-                              buddy.normalizedName, false);
> }
> PossibleConvFromContact.prototype = {
>   __proto__: PossibleConversation,
>   get statusText() this._statusText,
>   get source() "contact",
>+  get id() {
>+    let buddy = this.contact.preferredBuddy;
>+    return getConversationId(buddy.protocol.normalizedName,
>+                             buddy.preferredAccountBuddy.account.normalizedName,
>+                             buddy.normalizedName, false);
>+  },

I wonder if this change has performance implications. You just said on IRC that it's never used, but I haven't checked and think I'll need to (unless nhnt11 can confirm).


>   get computedScore() {
>-    let id = this._contactId;
>-    if (gStatsByContactId && gStatsByContactId[id])
>-      return gStatsByContactId[id].computedScore;
>+    let contactid = this._contactId;
>+    if (gStatsByContactId && gStatsByContactId[contactid])
>+      return gStatsByContactId[contactid].computedScore;
>     // Contacts may have multiple buddies attached to them, so we sum their
>     // individual message counts before arriving at the final score.
>     let stats = new ConversationStats();
>     for (let id of this.buddyIds) {
>       let buddyStats = gStatsByConvId[id];
>       if (buddyStats)
>         stats = stats.mergeWith(buddyStats);
>     }
>     if (gStatsByContactId)
>-      gStatsByContactId[id] = stats;
>+      gStatsByContactId[contactid] = stats;

I don't see what motivated this change, but if you think "id" really wasn't explicit enough, make the new name contactId (camel case).


> function PossibleChat(aRoomInfo) {
>   this._roomInfo = aRoomInfo;
>   let account = this.account;
>   this.id = getConversationId(account.protocol.normalizedName,
>-                              account.name, this.displayName, true);
>+                              account.normalizedName,
>+                              account.normalize(this.displayName), true);

Normalizing a display name seems scary (in the future the display name here may contain some localizable strings, or whatever we feel like displaying in the UI). Maybe use aRoomInfo.name instead of this.displayName?


Overall, this seems reasonable, but I would really like nhnt11 to have a look. This review was focused on looking at the changes you made; if I need to do the final review here, I'll need to focus instead on what may have been missed.
Attachment #8354887 - Flags: review?(florian) → review-
Attached patch Patch2Splinter Review
*** Original post on bio 2210 as attmnt 3105 at 2013-12-01 18:14:00 UTC ***

(In reply to comment #8)

> This needs a comment saying that the 'normalizedName' for conversations was
> added in the log files with Ib 1.5, and that for backward compatibility we will
> re-normalize the name for older log files.

Any thoughts on comment#7?

> >+  get id() {
> >+    let buddy = this.contact.preferredBuddy;
> >+    return getConversationId(buddy.protocol.normalizedName,
> >+                             buddy.preferredAccountBuddy.account.normalizedName,
> >+                             buddy.normalizedName, false);
> >+  },
> 
> I wonder if this change has performance implications. You just said on IRC that
> it's never used, but I haven't checked and think I'll need to (unless nhnt11
> can confirm).

It's the computedScore that is frequently accessed.

> I don't see what motivated this change, but if you think "id" really wasn't
> explicit enough, make the new name contactId (camel case).

The motivation was to avoid confusion with the possibleConv ids.

> Normalizing a display name seems scary (in the future the display name here may
> contain some localizable strings, or whatever we feel like displaying in the
> UI). Maybe use aRoomInfo.name instead of this.displayName?

Good idea, though in fact currently this.displayName just returns aRoomInfo.name, this might change.
Attachment #8354889 - Flags: review?(florian)
Attachment #8354889 - Flags: review?(nhnt11)
Comment on attachment 8354887 [details] [diff] [review]
Patch

*** Original change on bio 2210 attmnt 3104 at 2013-12-01 18:14:26 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354887 - Attachment is obsolete: true
*** Original post on bio 2210 at 2013-12-01 18:23:43 UTC ***

(In reply to comment #9)
> (In reply to comment #8)
> 
> > This needs a comment saying that the 'normalizedName' for conversations was
> > added in the log files with Ib 1.5, and that for backward compatibility we will
> > re-normalize the name for older log files.
> 
> Any thoughts on comment#7?

I agree with the "The if (!name)... code really wants to be in logger.js." statement.

I don't care about it for 1.5 though.

And I wouldn't be surprised if logger.js got a significant rewrite in 1.5-next, to make its I/O fully async.
Comment on attachment 8354889 [details] [diff] [review]
Patch2

*** Original change on bio 2210 attmnt 3105 at 2013-12-02 22:47:13 UTC ***

(In reply to comment #8)
> Overall, this seems reasonable, but I would really like nhnt11 to have a look.
> This review was focused on looking at the changes you made; if I need to do the
> final review here, I'll need to focus instead on what may have been missed.

I haven't done this (I just looked at the interdiff between the 2 patches), but given that nhnt11 looked at this and said on IRC that it seemed fine, I think at this point we should just try it on nightlies ASAP to have time to catch regressions if any.
Attachment #8354889 - Flags: review?(nhnt11)
Attachment #8354889 - Flags: review?(florian)
Attachment #8354889 - Flags: review+
*** Original post on bio 2210 at 2013-12-02 22:55:24 UTC ***

http://hg.instantbird.org/instantbird/rev/c77bcd2c8c3a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5
You need to log in before you can comment on or make changes to this bug.