Bug 40518 - Contacts appear in local pending and then immediately disappear again on GTalk
Summary: Contacts appear in local pending and then immediately disappear again on GTalk
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: gabble (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL: http://cgit.collabora.com/git/user/ka...
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2011-08-31 09:42 UTC by Marco Barisione
Modified: 2011-11-15 07:35 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
remove unused symbol (860 bytes, patch)
2011-11-02 06:32 UTC, Cosimo Alfarano
Details | Splinter Review
Alice is requesting a subscription (945 bytes, patch)
2011-11-02 06:33 UTC, Cosimo Alfarano
Details | Splinter Review
expect both signals and forbid them only for publish (1.88 KB, patch)
2011-11-02 06:34 UTC, Cosimo Alfarano
Details | Splinter Review
test subscription cancellation (2.01 KB, patch)
2011-11-02 06:35 UTC, Cosimo Alfarano
Details | Splinter Review
Marco's patch, first part of the test case, without Will's fix (4.38 KB, patch)
2011-11-02 06:37 UTC, Cosimo Alfarano
Details | Splinter Review

Description Marco Barisione 2011-08-31 09:42:59 UTC
When a contact adds us to their roster Google sends to gabble a presence update (with type="subscribe") followed by a roster update (with subscribe="none"). The result of this is that the contact appears as local pending because of the <presence> and then it disappears from stored because of the roster update.
Comment 1 Marco Barisione 2011-08-31 09:43:58 UTC
http://cgit.collabora.com/git/user/bari/telepathy-gabble.git/log/?h=local-pending-flicker prevents Gabble from removing local pending contacts from the stored list. It's a trick similar to what we already to in the case of remote pending contacts.
Comment 2 Will Thompson 2011-09-07 03:47:32 UTC
+    # We add Alice
+    presence = domish.Element(('jabber:client', 'presence'))
+    presence['from'] = contact
+    presence['type'] = 'subscribe'
+    stream.send(presence)

This comment is wrong. What's happening here is that Alice is trying to subscribe to *us*.

I think it's worth explicitly saying in either the code comment or the test that Google's not striiiictly speaking buggy:subscription='none' on the roster item is not technically a change if they weren't on the roster before, it just means that they're now explicitly on the roster with subscription='none', as opposed to not on the roster (implicitly with subscription='none').

+    When somebody asks to subscribe to us, Google sends the subscription
+    request and then a roster update saying there is no subscription.

So here, for instance: the roster update doesn't technically contradict the request.

We should test that receiving <presence type='unsubscribe'/> followed by a roster push like the one Google sends is correctly treated as notification that Alice has cancelled her subscription request: http://xmpp.org/rfcs/rfc6121.html#sub-cancel-inbound

It may already be tested, but I'm tempted to say it should be tested in this test case, too, to guarantee that ignoring the roster push doesn't have knock-on effects.
Comment 3 Cosimo Alfarano 2011-10-21 02:19:17 UTC
After a chat with Will on IRC, we agreed that the RFC section he suggested to test is 3.3.3 and not 3.2.3

http://xmpp.org/rfcs/rfc6121.html#sub-unsub-inbound

in the scenario for which Alice sends a un-subscription request right after her subscription request and before any approval/rejection by the user.
Comment 4 Cosimo Alfarano 2011-10-21 04:37:33 UTC
[Bugzilla is complaining the file I'm attaching is empty, workaround:]

http://pastebin.com/9YW4VkLB has the dbus logs for the extented test which do:
- unsubscribe
- expect ContactChanged
- send the maybe-redundant iq
- actually sees MembersChanged raised for the handle removed from storedm, which should be ignored

My commit with the extended test:
http://cgit.collabora.com/git/user/kalfa/telepathy-gabble.git/commit/?id=c32031008828651720a5661452b0e0a998ab8685

What I seems odd is the ContactsChanged with NO -> REMOVED_REMOTELY (1->2), while the handle should've been in ASK state according to the last signal.
Since Marco's fix is based on the ASK state, it might be it.
Comment 5 Will Thompson 2011-10-28 03:57:53 UTC
-    # We add Alice
+    # Alice asks to subscribes to us

This should be a separate patch.

-    self_handle = conn.GetSelfHandle()

As should this.

+    iq = make_set_roster_iq(stream, 'test@localhost/Resource', contact,
+            "none", False)
+    stream.send(iq)

You should move the comment

    # Now we send the spurious roster update with subscribe="none" and verify
    # that nothing happens in reaction to that

up to before you test this new case.

(In reply to comment #4)
> What I seems odd is the ContactsChanged with NO -> REMOVED_REMOTELY (1->2),
> while the handle should've been in ASK state according to the last signal.
> Since Marco's fix is based on the ASK state, it might be it.

I don't think it's changing from No to Removed_Remotely.


+    q.expect('dbus-signal', signal='ContactsChanged',
+            args=[{handle: (cs.SUBSCRIPTION_STATE_NO,
+                cs.SUBSCRIPTION_STATE_REMOVED_REMOTELY, '')}, []])

The first two fields of the tuple mean:

• We are not subscribing to the contact's presence;
• The contact removed their request to see our presence.

The previous state change, in response to their subscription request, was:

    q.expect('dbus-signal', signal='ContactsChanged',
            args=[{handle: (cs.SUBSCRIPTION_STATE_NO,
                cs.SUBSCRIPTION_STATE_ASK, '')}, []])

which means:

• We are not subscribing to the contact's presence;
• The contact has asked to see our presence.

So I think this test looks right.

-if test $sleep != 0; then
+if [ "$sleep" -ne 0 ]; then

What's this for?
Comment 6 Cosimo Alfarano 2011-10-31 08:15:21 UTC
(In reply to comment #5)
> -    # We add Alice
> +    # Alice asks to subscribes to us
> 
> This should be a separate patch.
> 
> -    self_handle = conn.GetSelfHandle()
> As should this.

OK.
This test has been introduced by Marco, so I guess that it means it will be committed as two separate patch sets, one from Marco and the second from me atop Marco's work.



> +    iq = make_set_roster_iq(stream, 'test@localhost/Resource', contact,
> +            "none", False)
> +    stream.send(iq)
> 
> You should move the comment
> 
>     # Now we send the spurious roster update with subscribe="none" and verify
>     # that nothing happens in reaction to that
> 
> up to before you test this new case.
> 
> (In reply to comment #4)
> > What I seems odd is the ContactsChanged with NO -> REMOVED_REMOTELY (1->2),
> > while the handle should've been in ASK state according to the last signal.
> > Since Marco's fix is based on the ASK state, it might be it.
> 
> I don't think it's changing from No to Removed_Remotely.
> 
> 
> +    q.expect('dbus-signal', signal='ContactsChanged',
> +            args=[{handle: (cs.SUBSCRIPTION_STATE_NO,
> +                cs.SUBSCRIPTION_STATE_REMOVED_REMOTELY, '')}, []])
> 
> The first two fields of the tuple mean:
> 
> • We are not subscribing to the contact's presence;
> • The contact removed their request to see our presence.
> 
> The previous state change, in response to their subscription request, was:
> 
>     q.expect('dbus-signal', signal='ContactsChanged',
>             args=[{handle: (cs.SUBSCRIPTION_STATE_NO,
>                 cs.SUBSCRIPTION_STATE_ASK, '')}, []])
> 
> which means:
> 
> • We are not subscribing to the contact's presence;
> • The contact has asked to see our presence.

Right, the first field is the sate of "subscribe" and the second of "publish"

http://telepathy.freedesktop.org/spec/Connection_Interface_Contact_List.html#Struct:Contact_Subscriptions

it does not represent a transition. my bad.

> So I think this test looks right.

It the test looks correct, the fact that it fails means that we need to look a way to fix it in gabble.

I can look at it, if Marco agrees (he's quite busy AFAIK).

> -if test $sleep != 0; then
> +if [ "$sleep" -ne 0 ]; then
> 
> What's this for?

The shell complained and I had to fix to be able to run the test.

It should be in a different commit as well, if the patch is OK.
Comment 7 Will Thompson 2011-11-01 10:21:02 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > So I think this test looks right.
> 
> It the test looks correct, the fact that it fails means that we need to look a
> way to fix it in gabble.

I found an error in the test, and fixed it: http://cgit.collabora.com/git/user/wjt/telepathy-gabble-wjt.git/commit/?h=local-pending-flicker&id=69e9f76b6c736e9e1bed148c23eb45a583862559
Comment 8 Cosimo Alfarano 2011-11-02 06:32:50 UTC
Created attachment 53048 [details] [review]
remove unused symbol
Comment 9 Cosimo Alfarano 2011-11-02 06:33:24 UTC
Created attachment 53049 [details] [review]
Alice is requesting a subscription
Comment 10 Cosimo Alfarano 2011-11-02 06:34:25 UTC
Created attachment 53050 [details] [review]
expect both signals and forbid them only for publish

First part of Will's fix, related to Marco's part
Comment 11 Cosimo Alfarano 2011-11-02 06:35:13 UTC
Created attachment 53051 [details] [review]
test subscription cancellation

My part of the test + Will's fixes
Comment 12 Cosimo Alfarano 2011-11-02 06:37:12 UTC
Created attachment 53052 [details] [review]
Marco's patch, first part of the test case, without Will's fix

this is actually the first one fo the serie, Marco's first patch.

then there are the others, in the attached order.
Comment 13 Will Thompson 2011-11-07 01:57:29 UTC
these patches look good now.
Comment 14 Will Thompson 2011-11-15 07:35:25 UTC
Merged to 0.12 (will be in 0.12.8), 0.14 (will be in 0.14.1) and master (will be in 0.15.0).


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.