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

Commit 7529b207 authored by Jeff Sharkey's avatar Jeff Sharkey
Browse files

More robust updating of "runnable" list. 

Whenever BroadcastProcessQueue.getRunnableAt() changes, we need to
check its position in the "runnable" list to ensure it stays sorted.

We've been returning boolean values to indicate when a caller should
invoke updateRunnableList(), so we expand this to more methods that
can have side-effects, and use the "@CheckResult" annotation to
ensure callers don't forget their duty.

It's okay to err on the side of calling updateRunnableList() when
unsure, since it has a fast-path that returns quickly if a queue is
already sorted correctly.

Bug: 274681945
Test: atest FrameworksMockingServicesTests:BroadcastQueueTest
Test: atest FrameworksMockingServicesTests:BroadcastQueueModernImplTest
Test: atest FrameworksMockingServicesTests:BroadcastRecordTest
Change-Id: Ief7d5f8540e5cbe3adc1f49c7703d41ae19003b8
parent 4b5ce492

services/core/java/com/android/server/am/BroadcastProcessQueue.java

+68 −15
Original line number Diff line number Diff line
@@ -21,6 +21,7 @@ import static com.android.server.am.BroadcastRecord.deliveryStateToString; 
import static com.android.server.am.BroadcastRecord.isDeliveryStateTerminal;

import static com.android.server.am.BroadcastRecord.isReceiverEquals;



import android.annotation.CheckResult;

import android.annotation.IntDef;

import android.annotation.NonNull;

import android.annotation.Nullable;
@@ -341,7 +342,12 @@ class BroadcastProcessQueue { 
     * Predicates that choose to remove a broadcast <em>must</em> finish

     * delivery of the matched broadcast, to ensure that situations like ordered

     * broadcasts are handled consistently.

     *

     * @return if this operation may have changed internal state, indicating

     *         that the caller is responsible for invoking

     *         {@link BroadcastQueueModernImpl#updateRunnableList}

     */

    @CheckResult

    public boolean forEachMatchingBroadcast(@NonNull BroadcastPredicate predicate,

            @NonNull BroadcastConsumer consumer, boolean andRemove) {

        boolean didSomething = false;
@@ -354,6 +360,7 @@ class BroadcastProcessQueue { 
        return didSomething;

    }



    @CheckResult

    private boolean forEachMatchingBroadcastInQueue(@NonNull ArrayDeque<SomeArgs> queue,

            @NonNull BroadcastPredicate predicate, @NonNull BroadcastConsumer consumer,

            boolean andRemove) {
@@ -370,6 +377,10 @@ class BroadcastProcessQueue { 
                    args.recycle();

                    it.remove();

                    onBroadcastDequeued(record, recordIndex, recordWouldBeSkipped);

                } else {

                    // Even if we're leaving broadcast in queue, it may have

                    // been mutated in such a way to change our runnable time

                    invalidateRunnableAt();

                }

                didSomething = true;

            }
@@ -381,30 +392,43 @@ class BroadcastProcessQueue { 


    /**

     * Update the actively running "warm" process for this process.

     *

     * @return if this operation may have changed internal state, indicating

     *         that the caller is responsible for invoking

     *         {@link BroadcastQueueModernImpl#updateRunnableList}

     */

    public void setProcess(@Nullable ProcessRecord app) {

    @CheckResult

    public boolean setProcessAndUidFrozen(@Nullable ProcessRecord app, boolean uidFrozen) {

        this.app = app;

        if (app != null) {

            setProcessInstrumented(app.getActiveInstrumentation() != null);

            setProcessPersistent(app.isPersistent());

        } else {

            setProcessInstrumented(false);

            setProcessPersistent(false);

        }



        // Since we may have just changed our PID, invalidate cached strings

        mCachedToString = null;

        mCachedToShortString = null;



        boolean didSomething = false;

        if (app != null) {

            didSomething |= setProcessInstrumented(app.getActiveInstrumentation() != null);

            didSomething |= setProcessPersistent(app.isPersistent());

            didSomething |= setUidFrozen(uidFrozen);

        } else {

            didSomething |= setProcessInstrumented(false);

            didSomething |= setProcessPersistent(false);

            didSomething |= setUidFrozen(uidFrozen);

        }

        return didSomething;

    }



    /**

     * Update if this UID is in the "frozen" state, typically signaling that

     * broadcast dispatch should be paused or delayed.

     */

    public void setUidFrozen(boolean frozen) {

    private boolean setUidFrozen(boolean frozen) {

        if (mUidFrozen != frozen) {

            mUidFrozen = frozen;

            invalidateRunnableAt();

            return true;

        } else {

            return false;

        }

    }
@@ -413,10 +437,13 @@ class BroadcastProcessQueue { 
     * signaling that broadcast dispatch should bypass all pauses or delays, to

     * avoid holding up test suites.

     */

    public void setProcessInstrumented(boolean instrumented) {

    private boolean setProcessInstrumented(boolean instrumented) {

        if (mProcessInstrumented != instrumented) {

            mProcessInstrumented = instrumented;

            invalidateRunnableAt();

            return true;

        } else {

            return false;

        }

    }
@@ -424,10 +451,13 @@ class BroadcastProcessQueue { 
     * Update if this process is in the "persistent" state, which signals broadcast dispatch should

     * bypass all pauses or delays to prevent the system from becoming out of sync with itself.

     */

    public void setProcessPersistent(boolean persistent) {

    private boolean setProcessPersistent(boolean persistent) {

        if (mProcessPersistent != persistent) {

            mProcessPersistent = persistent;

            invalidateRunnableAt();

            return true;

        } else {

            return false;

        }

    }
@@ -647,8 +677,20 @@ class BroadcastProcessQueue { 
        return mActive != null;

    }



    void forceDelayBroadcastDelivery(long delayedDurationMs) {

    /**

     * @return if this operation may have changed internal state, indicating

     *         that the caller is responsible for invoking

     *         {@link BroadcastQueueModernImpl#updateRunnableList}

     */

    @CheckResult

    boolean forceDelayBroadcastDelivery(long delayedDurationMs) {

        if (mForcedDelayedDurationMs != delayedDurationMs) {

            mForcedDelayedDurationMs = delayedDurationMs;

            invalidateRunnableAt();

            return true;

        } else {

            return false;

        }

    }



    /**
@@ -720,10 +762,21 @@ class BroadcastProcessQueue { 
     * broadcasts would be prioritized for dispatching, even if there are urgent broadcasts

     * waiting. This is typically used in case there are callers waiting for "barrier" to be

     * reached.

     *

     * @return if this operation may have changed internal state, indicating

     *         that the caller is responsible for invoking

     *         {@link BroadcastQueueModernImpl#updateRunnableList}

     */

    @CheckResult

    @VisibleForTesting

    void setPrioritizeEarliest(boolean prioritizeEarliest) {

    boolean setPrioritizeEarliest(boolean prioritizeEarliest) {

        if (mPrioritizeEarliest != prioritizeEarliest) {

            mPrioritizeEarliest = prioritizeEarliest;

            invalidateRunnableAt();

            return true;

        } else {

            return false;

        }

    }



    /**

services/core/java/com/android/server/am/BroadcastQueueModernImpl.java

+37 −17
Original line number Diff line number Diff line
@@ -360,8 +360,6 @@ class BroadcastQueueModernImpl extends BroadcastQueue { 
        // If app isn't running, and there's nothing in the queue, clean up

        if (queue.isEmpty() && !queue.isActive() && !queue.isProcessWarm()) {

            removeProcessQueue(queue.processName, queue.uid);

        } else {

            updateQueueDeferred(queue);

        }

    }
@@ -496,8 +494,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { 
        // relevant per-process queue

        final BroadcastProcessQueue queue = getProcessQueue(app);

        if (queue != null) {

            queue.setProcess(app);

            queue.setUidFrozen(mUidFrozen.get(queue.uid, false));

            setQueueProcess(queue, app);

        }



        boolean didSomething = false;
@@ -538,8 +535,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { 
        // relevant per-process queue

        final BroadcastProcessQueue queue = getProcessQueue(app);

        if (queue != null) {

            queue.setProcess(null);

            queue.setUidFrozen(mUidFrozen.get(queue.uid, false));

            setQueueProcess(queue, null);

        }



        if ((mRunningColdStart != null) && (mRunningColdStart == queue)) {
@@ -566,6 +562,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { 
                return (r.receivers.get(i) instanceof BroadcastFilter);

            }, mBroadcastConsumerSkip, true);

            if (didSomething || queue.isEmpty()) {

                updateQueueDeferred(queue);

                updateRunnableList(queue);

                enqueueUpdateRunningList();

            }
@@ -631,6 +628,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { 
                setDeliveryState(queue, null, r, i, receiver, BroadcastRecord.DELIVERY_DEFERRED,

                        "deferred at enqueue time");

            }

            updateQueueDeferred(queue);

            updateRunnableList(queue);

            enqueueUpdateRunningList();

        }
@@ -1079,6 +1077,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { 


            final int queueIndex = getRunningIndexOf(queue);

            mRunning[queueIndex] = null;

            updateQueueDeferred(queue);

            updateRunnableList(queue);

            enqueueUpdateRunningList();
@@ -1161,6 +1160,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { 
                                getReceiverUid(otherReceiver));

                        if (otherQueue != null) {

                            otherQueue.invalidateRunnableAt();

                            updateQueueDeferred(otherQueue);

                            updateRunnableList(otherQueue);

                        }

                    }
@@ -1291,6 +1291,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { 
                if (queuePredicate.test(leaf)) {

                    if (leaf.forEachMatchingBroadcast(broadcastPredicate,

                            broadcastConsumer, andRemove)) {

                        updateQueueDeferred(leaf);

                        updateRunnableList(leaf);

                        didSomething = true;

                    }
@@ -1304,29 +1305,40 @@ class BroadcastQueueModernImpl extends BroadcastQueue { 
        return didSomething;

    }



    private void forEachMatchingQueue(

    private boolean forEachMatchingQueue(

            @NonNull Predicate<BroadcastProcessQueue> queuePredicate,

            @NonNull Consumer<BroadcastProcessQueue> queueConsumer) {

        boolean didSomething = false;

        for (int i = mProcessQueues.size() - 1; i >= 0; i--) {

            BroadcastProcessQueue leaf = mProcessQueues.valueAt(i);

            while (leaf != null) {

                if (queuePredicate.test(leaf)) {

                    queueConsumer.accept(leaf);

                    updateQueueDeferred(leaf);

                    updateRunnableList(leaf);

                    didSomething = true;

                }

                leaf = leaf.processNameNext;

            }

        }

        if (didSomething) {

            enqueueUpdateRunningList();

        }

        return didSomething;

    }



    private void updateQueueDeferred(

            @NonNull BroadcastProcessQueue leaf) {

    @SuppressWarnings("CheckResult")

    private void updateQueueDeferred(@NonNull BroadcastProcessQueue leaf) {

        if (leaf.isDeferredUntilActive()) {

            // We ignore the returned value here since callers are invoking us

            // just before updateRunnableList()

            leaf.forEachMatchingBroadcast((r, i) -> {

                return r.deferUntilActive && (r.getDeliveryState(i)

                        == BroadcastRecord.DELIVERY_PENDING);

            }, mBroadcastConsumerDefer, false);

        } else if (leaf.hasDeferredBroadcasts()) {

            // We ignore the returned value here since callers are invoking us

            // just before updateRunnableList()

            leaf.forEachMatchingBroadcast((r, i) -> {

                return r.deferUntilActive && (r.getDeliveryState(i)

                        == BroadcastRecord.DELIVERY_DEFERRED);
@@ -1356,10 +1368,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { 
                        while (leaf != null) {

                            // Update internal state by refreshing values previously

                            // read from any known running process

                            leaf.setProcess(leaf.app);

                            leaf.setUidFrozen(frozen);

                            updateQueueDeferred(leaf);

                            updateRunnableList(leaf);

                            setQueueProcess(leaf, leaf.app);

                            leaf = leaf.processNameNext;

                        }

                        enqueueUpdateRunningList();
@@ -1520,8 +1529,20 @@ class BroadcastQueueModernImpl extends BroadcastQueue { 


    private void updateWarmProcess(@NonNull BroadcastProcessQueue queue) {

        if (!queue.isProcessWarm()) {

            queue.setProcess(mService.getProcessRecordLocked(queue.processName, queue.uid));

            queue.setUidFrozen(mUidFrozen.get(queue.uid, false));

            setQueueProcess(queue, mService.getProcessRecordLocked(queue.processName, queue.uid));

        }

    }



    /**

     * Update the {@link ProcessRecord} associated with the given

     * {@link BroadcastProcessQueue}. Also updates any runnable status that

     * might have changed as a side-effect.

     */

    private void setQueueProcess(@NonNull BroadcastProcessQueue queue,

            @Nullable ProcessRecord app) {

        if (queue.setProcessAndUidFrozen(app, mUidFrozen.get(queue.uid, false))) {

            updateQueueDeferred(queue);

            updateRunnableList(queue);

        }

    }
@@ -1715,8 +1736,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue { 
        }



        BroadcastProcessQueue created = new BroadcastProcessQueue(mConstants, processName, uid);

        created.setProcess(mService.getProcessRecordLocked(processName, uid));

        created.setUidFrozen(mUidFrozen.get(uid, false));

        setQueueProcess(created, mService.getProcessRecordLocked(processName, uid));



        if (leaf == null) {

            mProcessQueues.put(uid, created);

services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueModernImplTest.java

+7 −7
Original line number Diff line number Diff line
@@ -371,9 +371,9 @@ public final class BroadcastQueueModernImplTest { 
                List.of(makeMockRegisteredReceiver()), false);

        queue.enqueueOrReplaceBroadcast(airplaneRecord, 0, false);



        queue.setUidFrozen(false);

        queue.setProcessAndUidFrozen(null, false);

        final long notCachedRunnableAt = queue.getRunnableAt();

        queue.setUidFrozen(true);

        queue.setProcessAndUidFrozen(null, true);

        final long cachedRunnableAt = queue.getRunnableAt();

        assertThat(cachedRunnableAt).isGreaterThan(notCachedRunnableAt);

        assertFalse(queue.isRunnable());
@@ -398,9 +398,9 @@ public final class BroadcastQueueModernImplTest { 
                List.of(makeMockRegisteredReceiver()), false);

        queue.enqueueOrReplaceBroadcast(airplaneRecord, 0, false);



        queue.setUidFrozen(false);

        queue.setProcessAndUidFrozen(null, false);

        final long notCachedRunnableAt = queue.getRunnableAt();

        queue.setUidFrozen(true);

        queue.setProcessAndUidFrozen(null, true);

        final long cachedRunnableAt = queue.getRunnableAt();

        assertThat(cachedRunnableAt).isGreaterThan(notCachedRunnableAt);

        assertTrue(queue.isRunnable());
@@ -430,13 +430,13 @@ public final class BroadcastQueueModernImplTest { 
        // verify that:

        // (a) the queue is immediately runnable by existence of a fg-priority broadcast

        // (b) the next one up is the fg-priority broadcast despite its later enqueue time

        queue.setUidFrozen(false);

        queue.setProcessAndUidFrozen(null, false);

        assertTrue(queue.isRunnable());

        assertThat(queue.getRunnableAt()).isAtMost(airplaneRecord.enqueueClockTime);

        assertEquals(ProcessList.SCHED_GROUP_DEFAULT, queue.getPreferredSchedulingGroupLocked());

        assertEquals(queue.peekNextBroadcastRecord(), airplaneRecord);



        queue.setUidFrozen(true);

        queue.setProcessAndUidFrozen(null, true);

        assertTrue(queue.isRunnable());

        assertThat(queue.getRunnableAt()).isAtMost(airplaneRecord.enqueueClockTime);

        assertEquals(ProcessList.SCHED_GROUP_DEFAULT, queue.getPreferredSchedulingGroupLocked());
@@ -499,7 +499,7 @@ public final class BroadcastQueueModernImplTest { 
    private void doRunnableAt_Frozen(BroadcastRecord testRecord, int testRunnableAtReason) {

        final BroadcastProcessQueue queue = new BroadcastProcessQueue(mConstants,

                PACKAGE_GREEN, getUidForPackage(PACKAGE_GREEN));

        queue.setUidFrozen(true);

        queue.setProcessAndUidFrozen(null, true);



        final BroadcastRecord lazyRecord = makeBroadcastRecord(

                new Intent(Intent.ACTION_AIRPLANE_MODE_CHANGED),