Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove namespace from ContentPacketExtension #498

Merged
merged 2 commits into from Jun 27, 2018

Conversation

jahndis
Copy link

@jahndis jahndis commented Jun 18, 2018

This commit sets the namespace of the ContentPacketExtension to null. This was causing issues when parsing the jingle session-initiate message with stanza.io. The namespace was being set on content elements inside a group which stanza.io would fail to parse.

According the group XEP there usually isn't a namespace on the content elements (https://xmpp.org/extensions/xep-0338.html), and the namespace currently doesn't show up on the content elements inside the jingle element in the session-initiate message. (smack 4.2 might be doing something smart to remove a child element's namespace if it is the same as the parent).

More details in this discussion: https://community.jitsi.org/t/jitsi-dev-custom-client-fails-to-parse-jingle-session-initiate-with-new-jicofo/13992

Ran tests in jitsi-videobridge, jicofo, jicoco, and jigasi and all passed.

@damencho
Copy link
Member

@bgrozev @bbaldino WDYT?
I think this is safe and this was the way it used to be prior smack4 update.

@bbaldino
Copy link
Member

so do we not need to set the namespace based on its usage? (i.e. ingo's comment here)

@damencho
Copy link
Member

Yep, I'm not sure about that.
But if smack is removing namespace when they match the parent as @jahndis reported,
then defining two namespaces in ContentPacketExtension and make namespace as a required constructor parameter and passing the correct one depending on the usage will be a better approach. WDYT?
@jahndis can you try that?

@bgrozev
Copy link
Member

bgrozev commented Jun 19, 2018

How does this affect parsing?

@ibauersachs ibauersachs self-requested a review June 19, 2018 21:26
Copy link
Member

@ibauersachs ibauersachs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Smack doesn't do much in this case, since we're generating the XML ourselves (see AbstractPacketExtension.java#L123)
  • Parsing isn't affected if the namespace matches the namespace of the outer element (i.e. it is redundant)
  • It's causing issues for XEP-0338 because the namespace is not redundant, but simply the wrong one (not that a namespace is present at all)

Regardless, could you please make some changes?

  • Remove the NAMESPACE field entirely, having it like this is confusing
  • Pass null into the constructor, like it was before c7affc2

@jahndis
Copy link
Author

jahndis commented Jun 26, 2018

Sorry for the delay in response. I was sick for a couple days and then on vacation through the rest of the week.

I made the changes you recommended, and reran the tests (all passed). Let me know if there's anything else needed.

@ibauersachs ibauersachs merged commit 5f26de0 into jitsi:smack4-2 Jun 27, 2018
@ibauersachs
Copy link
Member

Note that the other projects need to update the version of Jitsi-Desktop that is referenced in their pom.xml. You might want to open separate pull requets for that or wait until another change will pull things in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants