Closed Bug 1298574 Opened 4 years ago Closed 4 years ago

vcard-temp infinite loop

Categories

(Chat Core :: XMPP, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Instantbird 51

People

(Reporter: arlolra, Unassigned)

References

Details

Attachments

(1 file, 4 obsolete files)

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>
Attachment #8785540 - Attachment is patch: true
Attachment #8785540 - Attachment mime type: text/x-patch → text/plain
Attachment #8785540 - Flags: review?(aleth)
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 ?
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.
Attachment #8785540 - Attachment is obsolete: true
Attachment #8785540 - Flags: review?(aleth)
Attachment #8785603 - Flags: review?(aleth)
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-
Attachment #8785603 - Attachment is obsolete: true
Attachment #8785633 - Flags: review?(aleth)
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();
LGTM.  Do you want to amend or should I?
Attachment #8785633 - Attachment is obsolete: true
Attachment #8785633 - Flags: review?(aleth)
Attachment #8785634 - Flags: review?(aleth)
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+
https://hg.mozilla.org/comm-central/rev/2540c39cc95816f35f99e9b5f52bf21d435143f1
Bug 1298574 - Set _userVCard own property when downloading vCard fails. r=aleth
Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 51
Duplicate of this bug: 1317425
Attachment #8785634 - Attachment is obsolete: true
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 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?
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.
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.
Attachment #8810824 - Flags: approval-comm-esr45?
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.