1298574 - vcard-temp infinite loop

Status: RESOLVED FIXED

Description

[arlolra]

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='[e-mail]/roundabout'
    type='error'>
  <vCard xmlns='vcard-temp'/>
  <error type='cancel'>
    <item-not-found xmlns='urn:ietf:params:xml:ns:xmpp-stanzas'/>
  </error>
</iq>

Comment 1

[aleth [: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 ?

Comment 2

[arlolra]

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.

Comment 3

[arlolra]

Created attachment 8785603 [details] [diff] [review]
0001-Set-own-property-when-downloading-vCard-fails.patch

Comment 4

[aleth [: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

Comment 5

[arlolra]

Created attachment 8785633 [details] [diff] [review]
0001-Set-own-property-when-downloading-vCard-fails.patch from comment 4

Comment 6

[aleth [: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();

Comment 7

[arlolra]

LGTM.  Do you want to amend or should I?

Comment 8

[arlolra]

Created attachment 8785634 [details] [diff] [review]
0001-Set-own-property-when-downloading-vCard-fails.patch from comment 6

Comment 9

[aleth [: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 ;)

Comment 10

[aleth [:aleth]]

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

Comment 12

[aleth [:aleth]]

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

Landed version.

Comment 13

[aleth [:aleth]]

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.

Comment 14

[aleth [:aleth]]

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

(51 is already in beta now.)

Comment 15

[Kent James (:rkent)]

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

[Kent James (:rkent)]

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.

Comment 17

[arlolra]

This is already in 45.7.0,
https://hg.mozilla.org/releases/comm-esr45/rev/e0020d9361e5