Closed Bug 722570 Opened 8 years ago Closed 8 years ago

BaseResource.java getConnectionManager/enableTLSConnectionManager is not threadsafe

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: dchanm+bugzilla, Assigned: nalexander)

References

Details

(Whiteboard: [qa-])

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
I think this is actually safe, because we aren't depending on the values being the same. But it would be nice to fix.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Priority: -- → P2
Blocks: 723230
https://hg.mozilla.org/services/services-central/rev/df9a4129745e
Assignee: rnewman → nalexander
Whiteboard: [fixed in services][qa-]
https://hg.mozilla.org/mozilla-central/rev/6827710a19f1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/df9a4129745e
Whiteboard: [fixed in services][qa-] → [qa-]
Product: Mozilla Services → Android Background Services
Product: Android Background Services → Firefox for Android
You need to log in before you can comment on or make changes to this bug.