From ff147d5052979185b9d59feb432cdd32cf32f7c3 Mon Sep 17 00:00:00 2001 From: Soonil Nagarkar Date: Fri, 6 Nov 2020 11:46:16 -0800 Subject: [PATCH] DO NOT MERGE Prevent isEnabled from mutating state When isEnabled sees a user it has not encountered before it may send out client updates, which can result in the client list and associated state being mutated unexpectedly, causing ConcurrentModificationExceptions. In order to prevent this, we now mutate the state asynchronously. Bug: 171910679 Test: unable to verify this manually, as this is a near impossible bug to reproduce intentionally Change-Id: I64ccdcdd50294b875d4ed22de60bff0823812ad7 --- .../location/LocationManagerService.java | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/services/core/java/com/android/server/location/LocationManagerService.java b/services/core/java/com/android/server/location/LocationManagerService.java index 5c9350a8452cb..3bb55a5af5c65 100644 --- a/services/core/java/com/android/server/location/LocationManagerService.java +++ b/services/core/java/com/android/server/location/LocationManagerService.java @@ -906,9 +906,26 @@ public class LocationManagerService extends ILocationManager.Stub { if (enabled == null) { // this generally shouldn't occur, but might be possible due to race conditions // on when we are notified of new users + + // hack to fix b/171910679. mutating the user enabled state within this method + // may cause unexpected changes to other state (for instance, this could cause + // provider enable/disable notifications to be sent to clients, which could + // result in a dead client being detected, which could result in the client + // being removed, which means that if this function is called while clients are + // being iterated over we have now unexpectedly mutated the iterated + // collection). instead, we return a correct value immediately here, and + // schedule the actual update for later. this has been completely rewritten and + // is no longer a problem in the next version of android. + enabled = mProvider.getState().allowed + && mUserInfoHelper.isCurrentUserId(userId) + && mSettingsHelper.isLocationEnabled(userId); + Log.w(TAG, mName + " provider saw user " + userId + " unexpectedly"); - onEnabledChangedLocked(userId); - enabled = Objects.requireNonNull(mEnabled.get(userId)); + mHandler.post(() -> { + synchronized (mLock) { + onEnabledChangedLocked(userId); + } + }); } return enabled;