All users were logged out of Bugzilla on October 13th, 2018

vcard-temp infinite loop

RESOLVED FIXED in Instantbird 51

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: arlolra, Unassigned)

Tracking

trunk
Instantbird 51

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8785540 [details] [diff] [review]
0001-Stop-requesting-user-vcard-when-item-not-found.patch

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.116 Safari/537.36

Steps to reproduce:

logged into an account on jabber.otr.im


Actual results:

with loglevel 1, my console is a never ending series of requests for user vcard


Expected results:

give up when not found

https://xmpp.org/extensions/xep-0054.html

<iq id='v1'
    to='stpeter@jabber.org/roundabout'
    type='error'>
  <vCard xmlns='vcard-temp'/>
  <error type='cancel'>
    <item-not-found xmlns='urn:ietf:params:xml:ns:xmpp-stanzas'/>
  </error>
</iq>
(Reporter)

Updated

2 years ago
Attachment #8785540 - Attachment is patch: true
Attachment #8785540 - Attachment mime type: text/x-patch → text/plain
(Reporter)

Updated

2 years ago
Attachment #8785540 - Flags: review?(aleth)

Comment 1

2 years ago
Comment on attachment 8785540 [details] [diff] [review]
0001-Stop-requesting-user-vcard-when-item-not-found.patch

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +2574,5 @@
>      delete this._downloadingUserVCard;
> +
> +    // Just ignore, _sendVCard below asks to downloadUserVCard again,
> +    // and around and around we go.
> +    if (this.handleErrors({ itemNotFound: () => true })(aStanza)) {

Clearly the infinite loop is a bug, but are you sure this is enough to ensure the vcard is set in the case when it doesn't exist yet, given https://dxr.mozilla.org/comm-central/source/chat/protocols/xmpp/xmpp.jsm?q=path%3Axmpp.jsm&redirect_type=single#2655 ?
(Reporter)

Comment 2

2 years ago
Yup, sorry, I was just jotting something down late last night.

Given,


>    // If the user currently doesn't have any vCard on the server or
>    // the download failed, an empty new one.
>    if (!this._userVCard)


The fix is probably to just set `this._userVCard = null;` so that `if (!this.hasOwnProperty("_userVCard"))` passes and the call to `this._downloadUserVCard();` is skipped.

I'll update, thanks.
(Reporter)

Comment 3

2 years ago
Created attachment 8785603 [details] [diff] [review]
0001-Set-own-property-when-downloading-vCard-fails.patch
Attachment #8785540 - Attachment is obsolete: true
Attachment #8785540 - Flags: review?(aleth)
Attachment #8785603 - Flags: review?(aleth)

Comment 4

2 years ago
Comment on attachment 8785603 [details] [diff] [review]
0001-Set-own-property-when-downloading-vCard-fails.patch

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +2576,5 @@
> +    // Downloading the vCard failed.  We set it so that we don't get into an
> +    // infinite loop trying to download it again before sending.  The check
> +    // there is for hasOwnProperty.
> +    if (this.handleErrors({
> +      // Do we want other error types here?

According to XEP-0054 item-not-found is the only one that is expected.

If there's any other error we should probably not try to set the vcard. So you can add something like

default: () => { this.WARN("Unexpected error retrieving the user's vcard"); return true; }

@@ +2580,5 @@
> +      // Do we want other error types here?
> +      itemNotFound: () => true
> +    })(aStanza)) {
> +      this._userVCard = null;
> +      this._sendVCard();

Looks like you could put this code in an else clause to https://dxr.mozilla.org/comm-central/source/chat/protocols/xmpp/xmpp.jsm#2594
Attachment #8785603 - Flags: review?(aleth) → review-
(Reporter)

Comment 5

2 years ago
Created attachment 8785633 [details] [diff] [review]
0001-Set-own-property-when-downloading-vCard-fails.patch from comment 4
Attachment #8785603 - Attachment is obsolete: true
Attachment #8785633 - Flags: review?(aleth)

Comment 6

2 years ago
Comment on attachment 8785633 [details] [diff] [review]
0001-Set-own-property-when-downloading-vCard-fails.patch from comment 4

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +2601,5 @@
> +        this.WARN("Unexpected error retrieving the user's vcard");
> +        return true;
> +      }
> +    })(aStanza))
> +      this._userVCard = null;

Hmm. If the error is something weird and not itemNotFound, we should probably not try to set the vcard either? What do you think?

ie. something like
else {
  if (this.handleErrors({
      itemNotFound: () => false, // OK, no vCard exists yet
      default: ... })
    return;  // Don't set the vCard after unexpected error
  else {
    // Avoid infinite loop
    this._userVCard = null;
  }
}
this._sendVCard();
(Reporter)

Comment 7

2 years ago
LGTM.  Do you want to amend or should I?
(Reporter)

Comment 8

2 years ago
Created attachment 8785634 [details] [diff] [review]
0001-Set-own-property-when-downloading-vCard-fails.patch from comment 6
Attachment #8785633 - Attachment is obsolete: true
Attachment #8785633 - Flags: review?(aleth)
Attachment #8785634 - Flags: review?(aleth)

Comment 9

2 years ago
Comment on attachment 8785634 [details] [diff] [review]
0001-Set-own-property-when-downloading-vCard-fails.patch from comment 6

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

I'll put back some of your better comments before landing ;)
Attachment #8785634 - Flags: review?(aleth) → review+

Comment 10

2 years ago
https://hg.mozilla.org/comm-central/rev/2540c39cc95816f35f99e9b5f52bf21d435143f1
Bug 1298574 - Set _userVCard own property when downloading vCard fails. r=aleth

Updated

2 years ago
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 51

Updated

2 years ago
Duplicate of this bug: 1317425

Comment 12

2 years ago
Created attachment 8810824 [details] [diff] [review]
Set _userVCard own property when downloading vCard fails

Landed version.

Updated

2 years ago
Attachment #8785634 - Attachment is obsolete: true

Comment 13

2 years ago
Comment on attachment 8810824 [details] [diff] [review]
Set _userVCard own property when downloading vCard fails

[Approval Request Comment]
User impact if declined: Complaints about excessive bandwidth usage, cf the duplicate bug
Testing completed (on c-c, etc.): yes
Risk to taking this patch (and alternatives if risky): just adds correct error handling.
Attachment #8810824 - Flags: approval-comm-esr45?
Attachment #8810824 - Flags: approval-comm-beta?
Attachment #8810824 - Flags: approval-comm-aurora?

Comment 14

2 years ago
Comment on attachment 8810824 [details] [diff] [review]
Set _userVCard own property when downloading vCard fails

(51 is already in beta now.)
Attachment #8810824 - Flags: approval-comm-beta?
Attachment #8810824 - Flags: approval-comm-aurora?

Comment 15

2 years ago
This was request for comm-esr45, but normal practice would be to wait until it goes through beta, which would result in landing in tb 45.6.0  Cloke asked about the bug, and agreed it could wait until then.

Comment 16

2 years ago
Somehow this patch got neglected by me. At this point I think that that right thing to do is to not land it for TB 45.8.0 which will come out at almost the same time as TB 52.0  Let's leave 45.8.0 for critical security bugs.

Updated

2 years ago
Attachment #8810824 - Flags: approval-comm-esr45?
(Reporter)

Comment 17

2 years ago
This is already in 45.7.0,
https://hg.mozilla.org/releases/comm-esr45/rev/e0020d9361e5
You need to log in before you can comment on or make changes to this bug.