Merge "Fix a concurrency bug in the ranking reconsideration." (e3dd9118) · Commits · e / os / android_frameworks_base

services/core/java/com/android/server/notification/NotificationIntrusivenessExtractor.java

+8 −4

Original line number	Diff line number	Diff line
		@@ -20,7 +20,6 @@ import android.app.Notification;
		import android.content.Context;
		import android.util.Slog;

		import com.android.internal.R;
		import com.android.server.notification.NotificationManagerService.NotificationRecord;

		/**
		@@ -39,7 +38,7 @@ public class NotificationIntrusivenessExtractor implements NotificationSignalExt
		if (DBG) Slog.d(TAG, "Initializing " + getClass().getSimpleName() + ".");
		}

		public RankingFuture process(NotificationRecord record) {
		public RankingReconsideration process(NotificationRecord record) {
		if (record == null \|\| record.getNotification() == null) {
		if (DBG) Slog.d(TAG, "skipping empty notification");
		return null;
		@@ -54,10 +53,15 @@ public class NotificationIntrusivenessExtractor implements NotificationSignalExt
		record.setRecentlyIntusive(true);
		}

		return new RankingFuture(record, HANG_TIME_MS) {
		return new RankingReconsideration(record.getKey(), HANG_TIME_MS) {
		@Override
		public void work() {
		mRecord.setRecentlyIntusive(false);
		// pass
		}

		@Override
		public void applyChangesLocked(NotificationRecord record) {
		record.setRecentlyIntusive(false);
		}
		};
		}

services/core/java/com/android/server/notification/NotificationManagerService.java

+32 −33

Original line number	Diff line number	Diff line
		@@ -111,7 +111,6 @@ import java.util.Collections;
		import java.util.HashSet;
		import java.util.Iterator;
		import java.util.NoSuchElementException;
		import java.util.concurrent.ExecutionException;
		import java.util.concurrent.TimeUnit;

		/** {@hide} */
		@@ -453,8 +452,7 @@ public class NotificationManagerService extends SystemService {



		public static final class NotificationRecord
		{
		public static final class NotificationRecord {
		final StatusBarNotification sbn;
		SingleNotificationStats stats;
		boolean isCanceled;
		@@ -556,13 +554,13 @@ public class NotificationManagerService extends SystemService {
		return mContactAffinity;
		}

		public boolean isRecentlyIntrusive() {
		return mRecentlyIntrusive;
		}

		public void setRecentlyIntusive(boolean recentlyIntrusive) {
		mRecentlyIntrusive = recentlyIntrusive;
		}

		public boolean isRecentlyIntrusive() {
		return mRecentlyIntrusive;
		}
		}

		private static final class ToastRecord
		@@ -1635,8 +1633,8 @@ public class NotificationManagerService extends SystemService {
		if (!mSignalExtractors.isEmpty()) {
		for (NotificationSignalExtractor extractor : mSignalExtractors) {
		try {
		RankingFuture future = extractor.process(r);
		scheduleRankingReconsideration(future);
		RankingReconsideration recon = extractor.process(r);
		scheduleRankingReconsideration(recon);
		} catch (Throwable t) {
		Slog.w(TAG, "NotificationSignalExtractor failed.", t);
		}
		@@ -1982,37 +1980,38 @@ public class NotificationManagerService extends SystemService {
		}
		}

		private void scheduleRankingReconsideration(RankingFuture future) {
		if (future != null) {
		Message m = Message.obtain(mRankingHandler, MESSAGE_RECONSIDER_RANKING, future);
		long delay = future.getDelay(TimeUnit.MILLISECONDS);
		private void scheduleRankingReconsideration(RankingReconsideration recon) {
		if (recon != null) {
		Message m = Message.obtain(mRankingHandler, MESSAGE_RECONSIDER_RANKING, recon);
		long delay = recon.getDelay(TimeUnit.MILLISECONDS);
		mRankingHandler.sendMessageDelayed(m, delay);
		}
		}

		private void handleRankingReconsideration(Message message) {
		if (!(message.obj instanceof RankingFuture)) return;

		RankingFuture future = (RankingFuture) message.obj;
		future.run();
		try {
		NotificationRecord record = future.get();
		if (!(message.obj instanceof RankingReconsideration)) return;
		RankingReconsideration recon = (RankingReconsideration) message.obj;
		recon.run();
		boolean orderChanged;
		synchronized (mNotificationList) {
		int before = mNotificationList.indexOf(record);
		if (before != -1) {
		Collections.sort(mNotificationList, mRankingComparator);
		int after = mNotificationList.indexOf(record);

		if (before != after) {
		scheduleSendRankingUpdate();
		final NotificationRecord record = mNotificationsByKey.get(recon.getKey());
		if (record == null) {
		return;
		}
		int before = findNotificationRecordIndexLocked(record);
		recon.applyChangesLocked(record);
		Collections.sort(mNotificationList, mRankingComparator);
		int after = findNotificationRecordIndexLocked(record);
		orderChanged = before != after;
		}
		if (orderChanged) {
		scheduleSendRankingUpdate();
		}
		} catch (InterruptedException e) {
		// we're running the future explicitly, so this should never happen
		} catch (ExecutionException e) {
		// we're running the future explicitly, so this should never happen
		}

		// lock on mNotificationList
		private int findNotificationRecordIndexLocked(NotificationRecord target) {
		return Collections.binarySearch(mNotificationList, target, mRankingComparator);
		}

		private void scheduleSendRankingUpdate() {

services/core/java/com/android/server/notification/NotificationSignalExtractor.java

+6 −4

Original line number	Diff line number	Diff line
		@@ -18,6 +18,8 @@ package com.android.server.notification;

		import android.content.Context;

		import com.android.server.notification.NotificationManagerService.NotificationRecord;

		/**
		* Extracts signals that will be useful to the {@link NotificationComparator} and caches them
		* on the {@link NotificationManagerService.NotificationRecord} object. These annotations will
		@@ -32,10 +34,10 @@ public interface NotificationSignalExtractor {
		* Called once per notification that is posted or updated.
		*
		* @return null if the work is done, or a future if there is more to do. The
		* {@link RankingFuture} will be run on a worker thread, and if notifications are re-ordered
		* by that execution, the {@link NotificationManagerService} may send order update
		* events to the {@link android.service.notification.NotificationListenerService}s.
		* {@link RankingReconsideration} will be run on a worker thread, and if notifications
		* are re-ordered by that execution, the {@link NotificationManagerService} may send order
		* update events to the {@link android.service.notification.NotificationListenerService}s.
		*/
		public RankingFuture process(NotificationManagerService.NotificationRecord notification);
		public RankingReconsideration process(NotificationRecord notification);

		}

services/core/java/com/android/server/notification/RankingFuture.java→services/core/java/com/android/server/notification/RankingReconsideration.java

+29 −48

Original line number	Diff line number	Diff line
		@@ -15,14 +15,16 @@
		*/
		package com.android.server.notification;

		import java.util.concurrent.Delayed;
		import java.util.concurrent.ExecutionException;
		import java.util.concurrent.ScheduledFuture;
		import com.android.server.notification.NotificationManagerService.NotificationRecord;

		import java.util.concurrent.TimeUnit;
		import java.util.concurrent.TimeoutException;

		public abstract class RankingFuture
		implements ScheduledFuture<NotificationManagerService.NotificationRecord> {
		/**
		* Represents future work required to extract signals from notifications for ranking.
		*
		* {@hide}
		*/
		public abstract class RankingReconsideration implements Runnable {
		private static final long IMMEDIATE = 0l;

		private static final int START = 0;
		@@ -32,18 +34,22 @@ public abstract class RankingFuture

		private int mState;
		private long mDelay;
		protected NotificationManagerService.NotificationRecord mRecord;
		protected String mKey;

		public RankingFuture(NotificationManagerService.NotificationRecord record) {
		this(record, IMMEDIATE);
		public RankingReconsideration(String key) {
		this(key, IMMEDIATE);
		}

		public RankingFuture(NotificationManagerService.NotificationRecord record, long delay) {
		public RankingReconsideration(String key, long delay) {
		mDelay = delay;
		mRecord = record;
		mKey = key;
		mState = START;
		}

		public String getKey() {
		return mKey;
		}

		public void run() {
		if (mState == START) {
		mState = RUNNING;
		@@ -57,18 +63,10 @@ public abstract class RankingFuture
		}
		}

		@Override
		public long getDelay(TimeUnit unit) {
		return unit.convert(mDelay, TimeUnit.MILLISECONDS);
		}

		@Override
		public int compareTo(Delayed another) {
		return Long.compare(getDelay(TimeUnit.MILLISECONDS),
		another.getDelay(TimeUnit.MILLISECONDS));
		}

		@Override
		public boolean cancel(boolean mayInterruptIfRunning) {
		if (mState == START) { // can't cancel if running or done
		mState = CANCELLED;
		@@ -77,42 +75,25 @@ public abstract class RankingFuture
		return false;
		}

		@Override
		public boolean isCancelled() {
		return mState == CANCELLED;
		}

		@Override
		public boolean isDone() {
		return mState == DONE;
		}

		@Override
		public NotificationManagerService.NotificationRecord get()
		throws InterruptedException, ExecutionException {
		while (!isDone()) {
		synchronized (this) {
		this.wait();
		}
		}
		return mRecord;
		}

		@Override
		public NotificationManagerService.NotificationRecord get(long timeout, TimeUnit unit)
		throws InterruptedException, ExecutionException, TimeoutException {
		long timeoutMillis = unit.convert(timeout, TimeUnit.MILLISECONDS);
		long start = System.currentTimeMillis();
		long now = System.currentTimeMillis();
		while (!isDone() && (now - start) < timeoutMillis) {
		try {
		wait(timeoutMillis - (now - start));
		} catch (InterruptedException e) {
		now = System.currentTimeMillis();
		}
		}
		return mRecord;
		}

		/**
		* Analyse the notification. This will be called on a worker thread. To
		* avoid concurrency issues, do not use held references to modify the
		* {@link NotificationRecord}.
		*/
		public abstract void work();

		/**
		* Apply any computed changes to the notification record. This method will be
		* called on the main service thread, synchronized on he mNotificationList.
		* @param record The locked record to be updated.
		*/
		public abstract void applyChangesLocked(NotificationRecord record);
		}

services/core/java/com/android/server/notification/ValidateNotificationPeople.java

+13 −10

Original line number	Diff line number	Diff line
		@@ -59,7 +59,7 @@ public class ValidateNotificationPeople implements NotificationSignalExtractor {
		// maps raw person handle to resolved person object
		private LruCache<String, LookupResult> mPeopleCache;

		private RankingFuture validatePeople(NotificationRecord record) {
		private RankingReconsideration validatePeople(final NotificationRecord record) {
		float affinity = NONE;
		Bundle extras = record.getNotification().extras;
		if (extras == null) {
		@@ -99,11 +99,11 @@ public class ValidateNotificationPeople implements NotificationSignalExtractor {
		}

		if (DEBUG) Slog.d(TAG, "Pending: future work scheduled for: " + record.sbn.getKey());
		return new RankingFuture(record) {
		return new RankingReconsideration(record.getKey()) {
		float mContactAffinity = NONE;
		@Override
		public void work() {
		if (INFO) Slog.i(TAG, "Executing: validation for: " + mRecord.sbn.getKey());
		float affinity = NONE;
		if (INFO) Slog.i(TAG, "Executing: validation for: " + record.getKey());
		for (final String handle: pendingLookups) {
		LookupResult lookupResult = null;
		final Uri uri = Uri.parse(handle);
		@@ -124,13 +124,16 @@ public class ValidateNotificationPeople implements NotificationSignalExtractor {
		synchronized (mPeopleCache) {
		mPeopleCache.put(handle, lookupResult);
		}
		affinity = Math.max(affinity, lookupResult.getAffinity());
		mContactAffinity = Math.max(mContactAffinity, lookupResult.getAffinity());
		}
		}
		float affinityBound = mRecord.getContactAffinity();
		affinity = Math.max(affinity, affinityBound);
		mRecord.setContactAffinity(affinity);
		if (INFO) Slog.i(TAG, "final affinity: " + affinity);
		}

		@Override
		public void applyChangesLocked(NotificationRecord operand) {
		float affinityBound = operand.getContactAffinity();
		operand.setContactAffinity(Math.max(mContactAffinity, affinityBound));
		if (INFO) Slog.i(TAG, "final affinity: " + operand.getContactAffinity());
		}
		};
		}
		@@ -229,7 +232,7 @@ public class ValidateNotificationPeople implements NotificationSignalExtractor {
		mContext.getContentResolver(), SETTING_ENABLE_PEOPLE_VALIDATOR, 1);
		}

		public RankingFuture process(NotificationManagerService.NotificationRecord record) {
		public RankingReconsideration process(NotificationRecord record) {
		if (!mEnabled) {
		if (INFO) Slog.i(TAG, "disabled");
		return null;