Opened 7 years ago

Closed 7 years ago

#14588 closed defect (fixed)

xmppSession.js has duplicate case but different code paths

Reported by: Douglas Hays Owned by: Colin Snover
Priority: high Milestone: 1.7.3
Component: Dojox Version: 1.7.1
Keywords: Cc: Bryan Forbes
Blocked By: Blocking:

Description

Starting with the initial checkin of xmppSession.js in [14275]

switch(n.nodeName){
        case 'status':
        case 'show':
                p[n.nodeName]=n.firstChild.nodeValue;
                break;
        case 'status':
                p.priority=parseInt(n.firstChild.nodeValue);
                break;

there's a duplicate case for status but the code paths are different and I'm not sure which is correct. This is breaking the RAD tooling build of Dojo.

Attachments (2)

14588.patch (729 bytes) - added by Brian Arnold 7 years ago.
Adjust the duplicate case
rfc_3921.patch (981 bytes) - added by haysmark 7 years ago.
Fixed ability to read priority. Changed priority to correct default.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 7 years ago by Colin Snover

Priority: highblocker

Bulk update of open ticket priorities.

comment:2 Changed 7 years ago by Colin Snover

Priority: blockerlow

Reducing priority.

comment:3 Changed 7 years ago by Colin Snover

Milestone: 1.7.21.7.3

1.7.2 RC released, bumping milestone on remaining tickets.

Changed 7 years ago by Brian Arnold

Attachment: 14588.patch added

Adjust the duplicate case

comment:4 Changed 7 years ago by Brian Arnold

I've attached a very small patch that would clean up the switch, while preserving current code functionality and restoring the intent that seems to be set by the original line 482. That being said, lines 481-483 will never be ran, so they could theoretically just be cut.

comment:5 Changed 7 years ago by haysmark

Keywords: needsreview added
Priority: lowhigh

Here is the relevant section from the XMPP RFC:

2.2.2.2.  Status

   The OPTIONAL <status/> element contains XML character data specifying
   a natural-language description of availability status.  It is
   normally used in conjunction with the show element to provide a
   detailed description of an availability state (e.g., "In a meeting").
   The <status/> element MUST NOT possess any attributes, with the
   exception of the 'xml:lang' attribute.  Multiple instances of the
   <status/> element MAY be included but only if each instance possesses
   an 'xml:lang' attribute with a distinct language value.

2.2.2.3.  Priority

   The OPTIONAL <priority/> element contains non-human-readable XML
   character data that specifies the priority level of the resource. The
   value MUST be an integer between -128 and +127.  A presence stanza
   MUST NOT contain more than one <priority/> element.  The <priority/>
   element MUST NOT possess any attributes.  If no priority is provided,
   a server SHOULD consider the priority to be zero.  For information
   regarding the semantics of priority values in stanza routing within
   instant messaging and presence applications, refer to Server Rules
   for Handling XML Stanzas (Section 11).

Here's what happened: someone copied the status line with the intent to change it to "priority" (hence the parseInt and assignment to priority), wrote the right action, but forgot to change the label. So the supplied patch actually horribly breaks the code.

Last edited 7 years ago by haysmark (previous) (diff)

Changed 7 years ago by haysmark

Attachment: rfc_3921.patch added

Fixed ability to read priority. Changed priority to correct default.

comment:6 Changed 7 years ago by haysmark

I attached what I believe to be a correct patch.

comment:7 Changed 7 years ago by bill

#15335 is a duplicate of this ticket.

comment:8 Changed 7 years ago by Colin Snover

In [28626]:

Fix incorrect case value in xmppSession. Refs #14588. !strict

comment:9 Changed 7 years ago by Colin Snover

In [28627]:

Fix incorrect case value in xmppSession. Refs #14588. !strict

comment:10 Changed 7 years ago by Colin Snover

Keywords: needsreview removed
Owner: changed from Dustin Machi to Colin Snover
Status: newassigned

I’ve committed a one-line fix to the case that just fixes the switch-case. haysmark, according to the RFC quote you provided, since this is a client setting the priority it doesn’t need to be zero by default to conform to the RFC, servers just have to assume no priority means zero. Since that would be a non-bugfix behavioural change I don’t know if it should make it into 1.x or not, I don’t know if there is really a problem with priority being non-zero.

If you‘re cool with the fix I committed please resolve fixed. Otherwise let me know and we can work something out.

Cheers,

comment:11 Changed 7 years ago by haysmark

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.