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

Commit a417c258 authored by Pablo Gamito's avatar Pablo Gamito
Browse files

Make accesses to the LogGroupsMap thread safe 

There was a change of concurrent read and accesses to mLogGroups which would have caused issues.

We are also moving all reads and writes of mLogGroups out of the background worker thread to avoid out of sync accesses to the log groups leading to potential duplicate group registrations.

Flag: EXEMPT minor race-condition fix
Test: repeated runs of com.android.internal.protolog.ProtoLogTest
Bug: 423195734
Change-Id: Ic1957f7e25ff9b66683340893a5f5a3a182c85c5
parent 50252565

core/java/com/android/internal/protolog/PerfettoProtoLogImpl.java

+58 −36
Original line number Diff line number Diff line
@@ -62,6 +62,7 @@ import android.util.Log; 
import android.util.Slog;

import android.util.proto.ProtoOutputStream;



import com.android.internal.annotations.GuardedBy;

import com.android.internal.annotations.VisibleForTesting;

import com.android.internal.protolog.IProtoLogConfigurationService.RegisterClientArgs;

import com.android.internal.protolog.ProtoLogDataSource.Instance.TracingFlushCallback;
@@ -102,9 +103,14 @@ public abstract class PerfettoProtoLogImpl extends IProtoLogClient.Stub implemen 
    @NonNull

    protected final ProtoLogDataSource mDataSource;

    @Nullable

    protected final IProtoLogConfigurationService mConfigurationService;

    private final IProtoLogConfigurationService mConfigurationService;



    @NonNull

    private final Object mLogGroupsLock = new Object();



    @GuardedBy("mLogGroupsLock")

    @NonNull

    protected final TreeMap<String, IProtoLogGroup> mLogGroups = new TreeMap<>();

    private final TreeMap<String, IProtoLogGroup> mLogGroups = new TreeMap<>();

    @NonNull

    private final ProtoLogCacheUpdater mCacheUpdater;
@@ -182,7 +188,13 @@ public abstract class PerfettoProtoLogImpl extends IProtoLogClient.Stub implemen 
        Producer.init(InitArguments.DEFAULTS);



        if (android.tracing.Flags.clientSideProtoLogging() && mConfigurationService != null) {

            connectToConfigurationServiceAsync();

            synchronized (mLogGroupsLock) {

                // Get the values on the main thread instead of the background worker thread because

                // if we register more groups in the future this might happen before the task

                // executes on the background thread leading to double registration of groups.

                final var groups = mLogGroups.values().toArray(new IProtoLogGroup[0]);

                connectToConfigurationServiceAsync(groups);

            }

        }



        mDataSource.registerOnStartCallback(this);
@@ -195,6 +207,8 @@ public abstract class PerfettoProtoLogImpl extends IProtoLogClient.Stub implemen 
        registerGroupsLocally(groups);



        if (mConfigurationService != null) {

            // Because this will execute with a slight delay it means that we might be in sync with

            // the main log to logcat configuration immediately, but will be eventually.

            registerGroupsWithConfigurationServiceAsync(groups);

        } else {

            Log.w(LOG_TAG, "Missing configuration service... Not registering groups with it.");
@@ -223,23 +237,23 @@ public abstract class PerfettoProtoLogImpl extends IProtoLogClient.Stub implemen 
        return null;

    }



    private void connectToConfigurationServiceAsync() {

    private void connectToConfigurationServiceAsync(@NonNull IProtoLogGroup... groups) {

        Objects.requireNonNull(mConfigurationService,

                "A null ProtoLog Configuration Service was provided!");



        mBackgroundHandler.post(() -> {

            try {

                var args = createConfigurationServiceRegisterClientArgs();

                args.groups = new String[groups.length];

                args.groupsDefaultLogcatStatus = new boolean[groups.length];



                args.groups = new String[mLogGroups.size()];

                args.groupsDefaultLogcatStatus = new boolean[mLogGroups.size()];



                var groups = mLogGroups.values().stream().toList();

                for (var i = 0; i < groups.size(); i++) {

                    var group = groups.get(i);

                synchronized (mLogGroupsLock) {

                    for (var i = 0; i < groups.length; i++) {

                        var group = groups[i];

                        args.groups[i] = group.name();

                        args.groupsDefaultLogcatStatus[i] = group.isLogToLogcat();

                    }

                }



                mConfigurationService.registerClient(this, args);

            } catch (RemoteException e) {
@@ -405,10 +419,13 @@ public abstract class PerfettoProtoLogImpl extends IProtoLogClient.Stub implemen 
    @Override

    @NonNull

    public List<IProtoLogGroup> getRegisteredGroups() {

        synchronized (mLogGroupsLock) {

            return List.copyOf(mLogGroups.values());

        }

    }



    private void registerGroupsLocally(@NonNull IProtoLogGroup[] protoLogGroups) {

        synchronized (mLogGroupsLock) {

            // Verify we don't have id collisions, if we do we want to know as soon as possible and

            // we might want to manually specify an id for the group with a collision

            IProtoLogGroup[] allGroups = Stream.concat(
@@ -421,6 +438,7 @@ public abstract class PerfettoProtoLogImpl extends IProtoLogClient.Stub implemen 
                mLogGroups.put(protoLogGroup.name(), protoLogGroup);

            }

        }

    }



    private void verifyNoCollisionsOrDuplicates(@NonNull IProtoLogGroup[] protoLogGroups) {

        final var groupId = new ArraySet<Integer>();
@@ -823,6 +841,7 @@ public abstract class PerfettoProtoLogImpl extends IProtoLogClient.Stub implemen 
    }



    protected boolean validateGroups(@NonNull ILogger logger, @NonNull String[] groups) {

        synchronized (mLogGroupsLock) {

            for (int i = 0; i < groups.length; i++) {

                String group = groups[i];

                IProtoLogGroup g = mLogGroups.get(group);
@@ -831,6 +850,7 @@ public abstract class PerfettoProtoLogImpl extends IProtoLogClient.Stub implemen 
                    return false;

                }

            }

        }

        return true;

    }
@@ -839,6 +859,7 @@ public abstract class PerfettoProtoLogImpl extends IProtoLogClient.Stub implemen 
            return -1;

        }



        synchronized (mLogGroupsLock) {

            for (int i = 0; i < groups.length; i++) {

                String group = groups[i];

                IProtoLogGroup g = mLogGroups.get(group);
@@ -848,6 +869,7 @@ public abstract class PerfettoProtoLogImpl extends IProtoLogClient.Stub implemen 
                    throw new RuntimeException("No IProtoLogGroup named " + group);

                }

            }

        }



        mCacheUpdater.update(this);

        return 0;