722570 - BaseResource.java getConnectionManager/enableTLSConnectionManager is not threadsafe

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Android Sync, defect, P2)

Description

[David Chan [:dchan]]

BaseResource implements an unsafe singleton pattern in getConnection() [1]


Threads (T1, T2, T3)
T1 calls getConnectionMonitor()
T1 obtains lock on connManagerMonitor
T1 connManager == null
T1 calls enableTLSConnectionManager()
T1 continues until return cm; (connManager == cm#1)
T1 is context switched out
T2 calls getConnectionMonitor()
T2 must wait on connManagerMonitor
T2 gets context switched out
T3 calls enabledTLSConnectionMonitor() directly
T3 connManager = cm#2
T3 gets reference to cm#2
T3 is context switched out
T1 returns and has reference to cm#1
T1 exits synchronization block
T1 is context switched out
T2 obtains the lock for connManagerMonitor
T2 sees that connManager != null (set most recently by T3)
T2 gets reference to cm#2


A synchronization block around enableTLSConnectionManager() should be sufficient.

[1] - https://github.com/mozilla-services/android-sync/blob/develop/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java#L192

Comment 1

[Richard Newman [:rnewman]]

I think this is actually safe, because we aren't depending on the values being the same. But it would be nice to fix.

Comment 2

[Nick Alexander :nalexander [he/him]]

Merged in develop:

https://github.com/mozilla-services/android-sync/commit/fe693ca8755706456fc36084d45ce08e949d2389

Comment 3

[Richard Newman [:rnewman]]

https://hg.mozilla.org/services/services-central/rev/df9a4129745e

Comment 4

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6827710a19f1

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6827710a19f1

Comment 6

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/df9a4129745e