Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#83 Activity.addActivityListener() is not thread safe

open
nobody
None
5
2012-05-29
2012-05-29
Arye Shapiro
No

Activity.addActivityListener() method is not synchronized, which makes it not thread safe.
A NullPointerException is thrown because of a race between adding and removing a listener from the Activity object.
One thread checks that the m_listeners is not null, then the activity manager thread removes a listener and nullifies the m_listeners, then the NPE is thrown when the first thread tries to add the listener...

public void addActivityListener(ActivityListener l) { public void removeActivityListener(ActivityListener l) {

if ( m_listeners == null )
m_listeners = new CopyOnWriteArrayList();

if ( m_listeners == null )
return;
m_listeners.remove(l);
if ( m_listeners.size() == 0 )
m_listeners = null;

if ( !m_listeners.contains(l) )
m_listeners.add(l);
}
}

Discussion

  • Arye Shapiro
    Arye Shapiro
    2012-05-29

    Attached is the proposed version of the Activity.java file.
    I added a synchronization lock for all m_listeners handling.

     
  • Arye Shapiro
    Arye Shapiro
    2012-05-30

     
    Attachments
  • Arye Shapiro
    Arye Shapiro
    2012-05-30

     
    Attachments
  • Arye Shapiro
    Arye Shapiro
    2012-05-30

    The proposed lock caused deadlocks so I propose now to make the methods synchronized.
    While testing it, I saw that in some cases the ActivityManager was stuck. When debugging, I saw that in some cases, there was a race in the run() method, it checked for "_activityCount() > 0", it was '0' and the thread went into "wait()". But the "m_nextTime" was not yet set to "Long.MAX_VALUE". Then, in "_schedule()", the "( startTime < m_nextTime )" was false, so no "notify()" was called to release the "run()" from the "wait()".
    To fix it, I propose to add the following statement before going into "wait()":
    m_nextTime = Long.MAX_VALUE;
    I attached the ActivityManager.java file too.