[jitsi-dev] [PATCH] Fixed a bug in OTR - ScOtrEngineImpl.contactsMap doesn't get cleaned up.


#1

Hello,

I noticed a bug in OTR.
net.java.sip.communicator.plugin.otr.ScOtrEngineImpl.contactsMap maps from
SessionID to Contact. It makes sense that this map gets cleaned up from
Contact's after ProtocolProviderService gets unregistered, otherwise old
Contacts that contain references to the unregistered
ProtocolProviderService will remain and accessing these Contacts leads to
exceptions and failure in the OTR plugin.

One way to reproduce the bug is as follows:
1. Log in you XMPP account
2. Disable your XMPP account
3. Enable your XMPP account
-> The OTR plugin is no longer working for contacts in your XMPP account
contact list.

I'm sending you a patch with a fix.

Regards,
Marin

OTR bugfix1.patch (4.2 KB)


#2

I noticed a bug in OTR.
net.java.sip.communicator.plugin.otr.ScOtrEngineImpl.contactsMap maps from
SessionID to Contact. It makes sense that this map gets cleaned up from
Contact's after ProtocolProviderService gets unregistered, otherwise old
Contacts that contain references to the unregistered

ProtocolProviderService

will remain and accessing these Contacts leads to exceptions and failure

in

the OTR plugin.

One way to reproduce the bug is as follows:
1. Log in you XMPP account
2. Disable your XMPP account
3. Enable your XMPP account
-> The OTR plugin is no longer working for contacts in your XMPP account
contact list.

I'm sending you a patch with a fix.

- Please combine the two patches into one (you can use git's --amend fort
hat)

+ for (Map.Entry<ScSessionID, Contact> contactEntry:
contactsMap.entrySet())
+ {
+ if
(contactEntry.getValue().getProtocolProvider().equals(provider))
+ {
+ contactsMap.remove(contactEntry.getKey());
+ }
+ }

- Have you tested that this works if you had multiple entries for the
affected provider? If I remember that correctly, the iterator will throw a
ModifiedException when the list is modified from within the loop's body.

- Keep an eye on the 80 character line length limit
- The trace logging is a bit excessive, even if it is normally disabled

Regards,
Marin

Ingo


#3

Thank you, Marin! The patch looks good on first reading. I'd like to
remind you that our code convention dictates that lines should have a
maximum length of 80 characters.


#4

Thanks for the feedback!

- Have you tested that this works if you had multiple entries for the
affected provider? If I remember that correctly, the iterator will throw a
ModifiedException when the list is modified from within the loop's body.

No, actually I haven't and you were right for the
ConcurrentModificationException. Thanks for noticing!

- Please combine the two patches into one (you can use git's --amend fort
hat)

Done, plus fixes for everything mentioned.

Marin

OTRbugfix.patch (3.46 KB)

ยทยทยท

On Wed, Aug 28, 2013 at 11:56 AM, Ingo Bauersachs <ingo@jitsi.org> wrote:

> I noticed a bug in OTR.
> net.java.sip.communicator.plugin.otr.ScOtrEngineImpl.contactsMap maps
from
> SessionID to Contact. It makes sense that this map gets cleaned up from
> Contact's after ProtocolProviderService gets unregistered, otherwise old
> Contacts that contain references to the unregistered
ProtocolProviderService
> will remain and accessing these Contacts leads to exceptions and failure
in
> the OTR plugin.
>
> One way to reproduce the bug is as follows:
> 1. Log in you XMPP account
> 2. Disable your XMPP account
> 3. Enable your XMPP account
> -> The OTR plugin is no longer working for contacts in your XMPP account
> contact list.
>
>
> I'm sending you a patch with a fix.

- Please combine the two patches into one (you can use git's --amend fort
hat)

+ for (Map.Entry<ScSessionID, Contact> contactEntry:
contactsMap.entrySet())
+ {
+ if
(contactEntry.getValue().getProtocolProvider().equals(provider))
+ {
+ contactsMap.remove(contactEntry.getKey());
+ }
+ }

- Have you tested that this works if you had multiple entries for the
affected provider? If I remember that correctly, the iterator will throw a
ModifiedException when the list is modified from within the loop's body.

- Keep an eye on the 80 character line length limit
- The trace logging is a bit excessive, even if it is normally disabled

> Regards,
> Marin

Ingo

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev


#5

Pushed into jitsi/jitsi's master as commit
6f01b31e24a782d59db2dde241cab94739294390 and acknowledged on our Team
and Contributors page at https://jitsi.org.