Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 674b0a9e authored by Rhed Jao's avatar Rhed Jao
Browse files

Fix FileOperationService starts foreground service failed. 

Service#startForeground cannot use zero notification id.
Changing NOTIFICATION_ID_PROGRESS to 1. And use null tag
for foreground job notification.

Change-Id: I74f7e6e30a49766b31e88e2f9253654e3a1d0336
Fixes: 68392600
Fixes: 79779288
Test: Manually check if service destroy while copying job
Test: atest DocumentsUITests:FileOperationServiceTest
parent ec40c5c0

src/com/android/documentsui/services/FileOperationService.java

+49 −35
Original line number Diff line number Diff line
@@ -37,13 +37,12 @@ import com.android.documentsui.base.Features; 
import java.lang.annotation.Retention;

import java.lang.annotation.RetentionPolicy;

import java.util.ArrayList;

import java.util.HashMap;

import java.util.LinkedHashMap;

import java.util.List;

import java.util.Map;

import java.util.concurrent.ExecutorService;

import java.util.concurrent.Executors;

import java.util.concurrent.Future;

import java.util.concurrent.atomic.AtomicReference;



import javax.annotation.concurrent.GuardedBy;
@@ -99,9 +98,9 @@ public class FileOperationService extends Service implements Job.Listener { 


    private static final int POOL_SIZE = 2;  // "pool size", not *max* "pool size".



    private static final int NOTIFICATION_ID_PROGRESS = 0;

    private static final int NOTIFICATION_ID_FAILURE = 1;

    private static final int NOTIFICATION_ID_WARNING = 2;

    @VisibleForTesting static final int NOTIFICATION_ID_PROGRESS = 1;

    private static final int NOTIFICATION_ID_FAILURE = 2;

    private static final int NOTIFICATION_ID_WARNING = 3;



    // The executor and job factory are visible for testing and non-final

    // so we'll have a way to inject test doubles from the test. It's
@@ -124,10 +123,11 @@ public class FileOperationService extends Service implements Job.Listener { 
    @VisibleForTesting Features features;



    @GuardedBy("mJobs")

    private final Map<String, JobRecord> mJobs = new HashMap<>();

    private final Map<String, JobRecord> mJobs = new LinkedHashMap<>();



    // The job whose notification is used to keep the service in foreground mode.

    private final AtomicReference<Job> mForegroundJob = new AtomicReference<>();

    @GuardedBy("mJobs")

    private Job mForegroundJob;



    private PowerManager mPowerManager;

    private PowerManager.WakeLock mWakeLock;  // the wake lock, if held.
@@ -270,6 +270,7 @@ public class FileOperationService extends Service implements Job.Listener { 
            JobRecord record = mJobs.get(jobId);

            if (record != null) {

                record.job.cancel();

                updateForegroundState(record.job);

            }

        }
@@ -343,18 +344,23 @@ public class FileOperationService extends Service implements Job.Listener { 


        Notification notification = job.getSetupNotification();

        // If there is no foreground job yet, set this job to foreground job.

        if (mForegroundJob.compareAndSet(null, job)) {

        synchronized (mJobs) {

            if (mForegroundJob == null) {

                if (DEBUG) Log.d(TAG, "Set foreground job to " + job.id);

                mForegroundJob = job;

                foregroundManager.startForeground(NOTIFICATION_ID_PROGRESS, notification);

        }



            } else {

                // Show start up notification

                if (DEBUG) Log.d(TAG, "Posting notification for " + job.id);

                notificationManager.notify(

                job.id, NOTIFICATION_ID_PROGRESS, notification);

                        mForegroundJob == job ? null : job.id,

                        NOTIFICATION_ID_PROGRESS,

                        notification);

            }

        }



        // Set up related monitor

        JobMonitor monitor = new JobMonitor(job, notificationManager, handler, mJobs);

        JobMonitor monitor = new JobMonitor(job);

        monitor.start();

    }
@@ -388,11 +394,12 @@ public class FileOperationService extends Service implements Job.Listener { 


    @GuardedBy("mJobs")

    private void updateForegroundState(Job job) {

        Job candidate = mJobs.isEmpty() ? null : mJobs.values().iterator().next().job;

        Job candidate = getCandidateForegroundJob();



        // If foreground job is retiring and there is still work to do, we need to set it to a new

        // job.

        if (mForegroundJob.compareAndSet(job, candidate)) {

        if (mForegroundJob == job) {

            mForegroundJob = candidate;

            if (candidate == null) {

                if (DEBUG) Log.d(TAG, "Stop foreground");

                // Remove the notification here just in case we're torn down before we have the
@@ -401,12 +408,11 @@ public class FileOperationService extends Service implements Job.Listener { 
            } else {

                if (DEBUG) Log.d(TAG, "Switch foreground job to " + candidate.id);



                notificationManager.cancel(candidate.id, NOTIFICATION_ID_PROGRESS);

                Notification notification = (candidate.getState() == Job.STATE_STARTED)

                        ? candidate.getSetupNotification()

                        : candidate.getProgressNotification();

                foregroundManager.startForeground(NOTIFICATION_ID_PROGRESS, notification);

                notificationManager.notify(candidate.id, NOTIFICATION_ID_PROGRESS,

                        notification);

                notificationManager.notify(NOTIFICATION_ID_PROGRESS, notification);

            }

        }

    }
@@ -435,6 +441,19 @@ public class FileOperationService extends Service implements Job.Listener { 
        }

    }



    @GuardedBy("mJobs")

    private Job getCandidateForegroundJob() {

        if (mJobs.isEmpty()) {

            return null;

        }

        for (JobRecord rec : mJobs.values()) {

            if (!rec.job.isFinished()) {

                return rec.job;

            }

        }

        return null;

    }



    private static final class JobRecord {

        private final Job job;

        private final Future<?> future;
@@ -452,29 +471,22 @@ public class FileOperationService extends Service implements Job.Listener { 
     * still need to update notifications if jobs hang, so instead of jobs pushing their states,

     * we poll states of jobs.

     */

    private static final class JobMonitor implements Runnable {

    private final class JobMonitor implements Runnable {

        private static final long PROGRESS_INTERVAL_MILLIS = 500L;



        private final Job mJob;

        private final NotificationManager mNotificationManager;

        private final Handler mHandler;

        private final Object mJobsLock;



        private JobMonitor(Job job, NotificationManager notificationManager, Handler handler,

                Object jobsLock) {

        private JobMonitor(Job job) {

            mJob = job;

            mNotificationManager = notificationManager;

            mHandler = handler;

            mJobsLock = jobsLock;

        }



        private void start() {

            mHandler.post(this);

            handler.post(this);

        }



        @Override

        public void run() {

            synchronized (mJobsLock) {

            synchronized (mJobs) {

                if (mJob.isFinished()) {

                    // Finish notification is already shown. Progress notification is removed.

                    // Just finish itself.
@@ -483,11 +495,13 @@ public class FileOperationService extends Service implements Job.Listener { 


                // Only job in set up state has progress bar

                if (mJob.getState() == Job.STATE_SET_UP) {

                    mNotificationManager.notify(

                            mJob.id, NOTIFICATION_ID_PROGRESS, mJob.getProgressNotification());

                    notificationManager.notify(

                            mForegroundJob == mJob ? null : mJob.id,

                            NOTIFICATION_ID_PROGRESS,

                            mJob.getProgressNotification());

                }



                mHandler.postDelayed(this, PROGRESS_INTERVAL_MILLIS);

                handler.postDelayed(this, PROGRESS_INTERVAL_MILLIS);

            }

        }

    }

tests/common/com/android/documentsui/services/TestForegroundManager.java

+8 −8
Original line number Diff line number Diff line
@@ -23,17 +23,25 @@ import android.app.Notification; 


class TestForegroundManager implements FileOperationService.ForegroundManager {



    private final TestNotificationManager mNotificationManager;

    private int mForegroundId = -1;

    private Notification mForegroundNotification;



    TestForegroundManager(TestNotificationManager notificationManager) {

        assert(notificationManager != null);

        mNotificationManager = notificationManager;

    }



    @Override

    public void startForeground(int id, Notification notification) {

        mForegroundId = id;

        mForegroundNotification = notification;

        mNotificationManager.notify(null, mForegroundId, mForegroundNotification);

    }



    @Override

    public void stopForeground(boolean cancelNotification) {

        mNotificationManager.cancel(null, mForegroundId);

        mForegroundId = -1;

        mForegroundNotification = null;

    }
@@ -45,12 +53,4 @@ class TestForegroundManager implements FileOperationService.ForegroundManager { 
    void assertInBackground() {

        assertNull(mForegroundNotification);

    }



    int getForegroundId() {

        return mForegroundId;

    }



    Notification getForegroundNotification() {

        return mForegroundNotification;

    }

}

tests/common/com/android/documentsui/services/TestNotificationManager.java

+20 −18
Original line number Diff line number Diff line
@@ -17,6 +17,7 @@ 
package com.android.documentsui.services;



import static junit.framework.Assert.assertEquals;

import static junit.framework.Assert.assertFalse;

import static junit.framework.Assert.assertNotNull;

import static junit.framework.Assert.assertTrue;
@@ -32,21 +33,10 @@ import java.util.HashMap; 


class TestNotificationManager {



    private final TestForegroundManager mForegroundManager;

    private final SparseArray<HashMap<String, Notification>> mNotifications = new SparseArray<>();

    private final Answer<Void> mAnswer = this::invoke;



    TestNotificationManager(TestForegroundManager foregroundManager) {

        assert(foregroundManager != null);

        mForegroundManager = foregroundManager;

    }



    private void notify(String tag, int id, Notification notification) {

        if (notification == mForegroundManager.getForegroundNotification()

                && id != mForegroundManager.getForegroundId()) {

            throw new IllegalStateException("Mismatching ID and notification.");

        }



    void notify(String tag, int id, Notification notification) {

        if (mNotifications.get(id) == null) {

            mNotifications.put(id, new HashMap<>());

        }
@@ -54,16 +44,12 @@ class TestNotificationManager { 
        mNotifications.get(id).put(tag, notification);

    }



    private void cancel(String tag, int id) {

    void cancel(String tag, int id) {

        final HashMap<String, Notification> idMap = mNotifications.get(id);

        if (idMap != null && idMap.containsKey(tag)) {

            final Notification notification = idMap.get(tag);

            // Only cancel non-foreground notification

            if (mForegroundManager.getForegroundNotification() != notification) {

            idMap.remove(tag);

        }

    }

    }



    private Void invoke(InvocationOnMock invocation) {

        Object[] args = invocation.getArguments();
@@ -88,6 +74,14 @@ class TestNotificationManager { 
        return null;

    }



    private boolean hasNotification(int id, String jobId) {

        if (mNotifications.get(id) == null) {

            return false;

        }

        Notification notification = mNotifications.get(id).get(jobId);

        return notification != null;

    }



    NotificationManager createNotificationManager() {

        return Mockito.mock(NotificationManager.class, mAnswer);

    }
@@ -100,4 +94,12 @@ class TestNotificationManager { 


        assertEquals(expect, count);

    }



    void assertHasNotification(int id, String jobid) {

        assertTrue(hasNotification(id, jobid));

    }



    void assertNoNotification(int id, String jobid) {

        assertFalse(hasNotification(id, jobid));

    }

}

tests/unit/com/android/documentsui/services/FileOperationServiceTest.java

+28 −2
Original line number Diff line number Diff line
@@ -78,8 +78,8 @@ public class FileOperationServiceTest extends ServiceTestCase<FileOperationServi 
        mExecutor = new TestScheduledExecutorService();

        mDeletionExecutor = new TestScheduledExecutorService();

        mHandler = new TestHandler();

        mForegroundManager = new TestForegroundManager();

        mTestNotificationManager = new TestNotificationManager(mForegroundManager);

        mTestNotificationManager = new TestNotificationManager();

        mForegroundManager = new TestForegroundManager(mTestNotificationManager);

        TestFeatures features = new TestFeatures();

        features.notificationChannel = InstrumentationRegistry.getTargetContext()

                .getResources().getBoolean(R.bool.feature_notification_channel);
@@ -294,6 +294,32 @@ public class FileOperationServiceTest extends ServiceTestCase<FileOperationServi 
        mTestNotificationManager.assertNumberOfNotifications(0);

    }



    public void testNotificationUpdateAfterForegroundJobSwitch() throws Exception {

        startService(createCopyIntent(newArrayList(ALPHA_DOC), BETA_DOC));

        startService(createCopyIntent(newArrayList(GAMMA_DOC), DELTA_DOC));

        Job job1 = mCopyJobs.get(0);

        Job job2 = mCopyJobs.get(1);



        mService.onStart(job1);

        mTestNotificationManager.assertHasNotification(

                FileOperationService.NOTIFICATION_ID_PROGRESS, null);



        mService.onStart(job2);

        mTestNotificationManager.assertHasNotification(

                FileOperationService.NOTIFICATION_ID_PROGRESS, job2.id);



        job1.cancel();

        mService.onFinished(job1);

        mTestNotificationManager.assertHasNotification(

                FileOperationService.NOTIFICATION_ID_PROGRESS, null);

        mTestNotificationManager.assertNoNotification(

                FileOperationService.NOTIFICATION_ID_PROGRESS, job2.id);



        job2.cancel();

        mService.onFinished(job2);

        mTestNotificationManager.assertNumberOfNotifications(0);

    }



    private Intent createCopyIntent(ArrayList<DocumentInfo> files, DocumentInfo dest)

            throws Exception {

        DocumentStack stack = new DocumentStack();