xmpp4moz should not have default event handler

Bug #307009 reported by Jehan
4
Affects Status Importance Assigned to Milestone
SamePlace
New
Undecided
Unassigned

Bug Description

I made a handler for service discovery request. Then by testing it, I discovered that there is already a default handler for it and nowhere to deactivate it (or at least it is not documented anywhere).
Hence when I sent my service discovery request to my agent using xmpp4moz, I got two response handlers, mine and the default one:

<iq from='my@plugin/resource1' to='my@domain/resource2 type='result' id='disco1'>
<query xmlns='http://jabber.org/protocol/disco#info'>
<identity category='client' type='pc' name='xmpp4moz'/>
<feature var='http://jabber.org/protocol/disco#info'/>
</query>
</iq>

Of course this is a wrong behaviour. There must not be two (different!) responses for a same request. I think this event handler should be moved to sameplace, for which I guess it must have been created, and should not be present in the bare xmpp4moz library (on my tests, sameplace is deactivated, so I know this comes from xmpp4moz).

Revision history for this message
Jehan (jehan-launchpad) wrote :

Hi again,

looking to the code, the file to modify is content/service/client_service.js, and lines 223 to 241 must be removed. These are the lines which generates a disco#info result. I would suggest you to move this to sameplace so that it will be used only for sameplace, not for the generic XMPP lib xmpp4moz.

Note that I am reading this in the downloadable released and development version, not from any current trunk (if you use a revision manager, I have not found it). So maybe the lines are different in your current branch. But anyway this is easy to find by searching for the "disco#info" string.

Thanks for this nice API.
Regards,

Jehan

Revision history for this message
Massimiliano Mirra (bard-hyperstruct) wrote :

How would you handle the case where more than one extension is registering a disco#info handler and how would you handle the case where no extension is registering it?

That handler is in the lowest layer because it needs to be there (per spec) and unique, not because it is needed by SamePlace. If you want to affect it, simply pass a <features/> element in XMPP.createChannel() like SamePlace does for e.g. MUC.

Revision history for this message
Jehan (jehan-launchpad) wrote :

Yes someone pointed me to this possibility this morning. I had no time to test it before now. Note that I hadn't seen it
because the full version of createChannel is not documented in API: channel or API: event on your wiki.

So it seems to work fine with a few "issues":

1/ I wanted to be able to redefine the identity. Mine would be a category "client", but type "bot". And I wanted to rename the name (here it displays "xmpp4moz").

2/ My plugin was made to support caps (XEP-0115) as well. The issue is that caps work by getting a service discovery as well and xmpp4moz doesn't support it, but my plugin did. Now with this method, my plugin doesn't.

I understand your concern about using xmpp4moz for another plugin. Anyway mine is made to have its resource on its own anyway. So even with another plugin using it on the same Firefox instance, it won't disturb... They can cohabit, but still are not connecting with the same resource.

Revision history for this message
kael (kael) wrote :

Here are some tests :

- Xmpp4moz is installed, SamePlace is not, and the extension using x4m advertises some additional features with XMPP.createChannel() :

<iq type="result">
  <query xmlns="http://jabber.org/protocol/disco#info">
    <identity category="client" type="pc" name="xmpp4moz"/>
    <feature var="http://jabber.org/protocol/disco#info"/>
    <feature var="http://jabber.org/protocol/tune+notify"/>
  </query>
</iq>

- same case than previously but with SP installed and no account configured with SP :

<iq type="result">
  <query xmlns="http://jabber.org/protocol/disco#info">
    <identity category="client" type="pc" name="xmpp4moz"/>
    <feature var="http://jabber.org/protocol/disco#info"/>
    <feature var="http://jabber.org/protocol/tune+notify"/>
    <feature var="http://jabber.org/protocol/muc"/>
    <feature var="http://jabber.org/protocol/muc#user"/>
    <feature var="http://jabber.org/protocol/xhtml-im"/>
    <feature var="http://jabber.org/protocol/chatstates"/>
  </query>
</iq>

So apparently, SP publishes its features whereas it should not.

Was wondering if there would be a way to bind a JID, a connection and a set of features and identities. This would help to avoid connection conflicts and would allow to customize the identity element.

@Jehan

Regarding capabilities-based disco#info, the response is handled automatically by x4m, except the result doesn't contain the node :

<iq from="jabber.org" type="get">
  <query node="http://example.com/foobar#jG5aerzetzTYwT/WM94uAlu0=" xmlns="http://jabber.org/protocol/disco#info"/>
</iq>

<iq type="result" to="jabber.org">
  <query xmlns="http://jabber.org/protocol/disco#info">
    <identity category="client" type="pc" name="xmpp4moz"/>
    <feature var="http://jabber.org/protocol/disco#info"/>
    <feature var="http://jabber.org/protocol/tune+notify"/>
  </query>
</iq>

whereas it whould be :

<iq type="result" to="jabber.org">
  <query xmlns="http://jabber.org/protocol/disco#info" node="http://example.com/foobar#jG5aerzetzTYwT/WM94uAlu0=">
    <identity category="client" type="pc" name="xmpp4moz"/>
    <feature var="http://jabber.org/protocol/disco#info"/>
    <feature var="http://jabber.org/protocol/tune+notify"/>
  </query>
</iq>

It looks like Entity Capabilities would be a nice feature request for x4m, wouldn't it ? :)

Revision history for this message
Massimiliano Mirra (bard-hyperstruct) wrote :

> So apparently, SP publishes its features whereas it should not.

It's installed and active, thus it should.

> Was wondering if there would be a way to bind a JID, a connection and a set of features and identities. This would help to avoid connection conflicts and would allow to customize the identity element.

No, and it is not planned.

> It looks like Entity Capabilities would be a nice feature request for x4m, wouldn't it ? :)

Just one of very very many. :)

Revision history for this message
Jehan (jehan-launchpad) wrote :

Hi Bard,

> It's installed and active, thus it should.

What Kael was pointed is the case when Sameplace is active... but is currently not connected on the same account (or same resource!) than Xbookmarks for instance. In this case, the client will send wrong features, just because Sameplace is active (but maybe "just" active, and not connected, or on other full jids).

This global gestion of features is very nice, now that I understood it. But so that it is really and completely meaningfull, it must be "full jid"-based, and not global. Your lib must be able to differentiate features depending on the full jids currently online.

Hence maybe XMPP.createchannel () should be taking a full jid in parameters?

Revision history for this message
Massimiliano Mirra (bard-hyperstruct) wrote :

> What Kael was pointed is the case when Sameplace is active... but is
> currently not connected on the same account (or same resource!)

There is a fundamental misunderstanding here. SamePlace isn't connected (or not connected) on a certain account. The host application is.

> But so that it is really and completely meaningfull, it must be "full
> jid"-based, and not global.

I discussed this with Kael already. There are design trade-offs in the current architecture, namely the case where all extensions running on xmpp4moz in the same host application want to contribute to a unitary client is facilitated, at the expense of the case where multiple extensions want to behave like separate applications inside an OS. If we moved to the latter case, it would be at the expense of the former.

This is not going to change because 1) it would involve major rewrite and 2) the former case is the one I care the most about.

Also, "your lib must do X" is a poor way to win open source developers to one's cause.

Revision history for this message
Jehan (jehan-launchpad) wrote :

Hi,

sorry if my comment was giving the impression that I knew better than you. This was absolutely not what I meant. I was only giving suggestions and ideas, rather just like a brain-storming, not like "the good way to do". Maybe am I wrong, but then I still want to understand and propose new ideas or give opinions.
And to conclude on this subject, I don't want to win anyone to my cause. I just think that we can help each other, and this is why I give you my feedback on how I use or would like to use your lib, so that both of us can be happy, without the need from anyone to change one's cause. Purely functional: make anyone happy. This is my vision of open source for this ticket, not to win anything else. :-)

To come back to the current case, I still think it is possible that you can handle both use cases: multiple extensions running as an unitary client and as separate applications. And making XMPP channels full-jid aware must be one of the solutions, from what I understood in your lib design... This won't block extensions working together but will give new possibilities for separate applications.
Yet I understand that it is not your priority if it involves a lot of work...

Sorry again if you got upset by my previous messages.

Revision history for this message
Massimiliano Mirra (bard-hyperstruct) wrote :

Not at all, don't worry. I understand your "best for everyone" attitude and it would be great to attain that ideal, yet in software like in the tangible world there are competing forces at play in a single entity and part of my task here is to make sure that such forces stay in a productive balance for xmpp4moz instead of making it go boom. :)

That said, consider taking a look inside xmpp4moz. No, it's not an easy piece of code, and it's probably also ugly here and there. But since you're more motivated toward that second case, you may see paths I wouldn't, and I certainly wouldn't discard a solid implementation proposal lightly.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.