Closed
Bug 1298574
Opened 6 years ago
Closed 6 years ago
vcard-temp infinite loop
Categories
(Chat Core :: XMPP, defect)
Chat Core
XMPP
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 1•6 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 ?
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 4•6 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-
Attachment #8785603 -
Attachment is obsolete: true
Attachment #8785633 -
Flags: review?(aleth)
Comment 6•6 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();
Attachment #8785633 -
Attachment is obsolete: true
Attachment #8785633 -
Flags: review?(aleth)
Attachment #8785634 -
Flags: review?(aleth)
Comment 9•6 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•6 years ago
|
||
https://hg.mozilla.org/comm-central/rev/2540c39cc95816f35f99e9b5f52bf21d435143f1 Bug 1298574 - Set _userVCard own property when downloading vCard fails. r=aleth
Updated•6 years ago
|
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 51
Comment 12•6 years ago
|
||
Landed version.
Updated•6 years ago
|
Attachment #8785634 -
Attachment is obsolete: true
Comment 13•6 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•6 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•6 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•5 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•5 years ago
|
Attachment #8810824 -
Flags: approval-comm-esr45?
Reporter | ||
Comment 17•5 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.
Description
•