android: LibraryLoader: Annotate fields with @GuardedBy

Readability of this class suffers from (perhaps premature) optimizations
to eliminate locks in a few places where the class is used only on a
single thread. I believe avoiding these locks does not affect
correctness in practice, but explaining and maintaining the proper ways
to use these classes becomes more time consuming.

In this change I:

* Annotated the fields that are indeed guarded by the main lock.
* Expanded the explanation of the use of the secondary lock.
* Also added a few short-lived synchronized blocks, where an explanation
  of why it can be avoided is long and brittle. This adds more locking,
  but I believe it won't affect performance noticeably.
* Moved a few unguarded fields above the lock, so that the latter can
  guard everything 'below'.
* Made a few minor corrections in comments, now that I looked at this
  code a bit longer.

Bug: 1154224
Change-Id: I346d358a4b2a465cecdec1881908026e17fef7d6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2602285Reviewed-by: Benoit L <lizeb@chromium.org>
Commit-Queue: Egor Pasko <pasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840645}

base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java

@@ -60,9 +60,6 @@ import javax.annotation.concurrent.GuardedBy;
 public class LibraryLoader {
     private static final String TAG = "LibraryLoader";
 
-    // Location of extracted native libraries.
-    private static final String LIBRARY_DIR = "native_libraries";
-
     // Shared preferences key for the reached code profiler.
     private static final String DEPRECATED_REACHED_CODE_PROFILER_KEY =
             "reached_code_profiler_enabled";
@@ -77,53 +74,63 @@ public class LibraryLoader {
 
     // One-way switch becomes true when the libraries are initialized (by calling
     // LibraryLoaderJni.get().libraryLoaded, which forwards to LibraryLoaded(...) in
-    // library_loader_hooks.cc).  Note that this member should remain a one-way switch, since it
+    // library_loader_hooks.cc). Note that this member should remain a one-way switch, since it
     // accessed from multiple threads without a lock.
     private volatile boolean mInitialized;
 
     // State that only transitions one-way from 0->1->2. Volatile for the same reasons as
     // mInitialized.
+    @IntDef({LoadState.NOT_LOADED, LoadState.MAIN_DEX_LOADED, LoadState.LOADED})
+    @Retention(RetentionPolicy.SOURCE)
+    private @interface LoadState {
+        int NOT_LOADED = 0;
+        int MAIN_DEX_LOADED = 1;
+        int LOADED = 2;
+    }
     private volatile @LoadState int mLoadState;
 
-    // Guards all fields below.
-    private final Object mLock = new Object();
-
-    // Guards non-Main Dex initialization, which doesn't touch any fields guarded by mLock.
-    private final Object mNonMainDexLock = new Object();
-
-    private NativeLibraryPreloader mLibraryPreloader;
-    private boolean mLibraryPreloaderCalled;
-
-    // Whether to use the Chromium linker vs system linker.
+    // Whether to use the Chromium linker vs. the system linker.
+    // Avoids locking: should be initialized very early.
     private boolean mUseChromiumLinker;
 
-    // Whether to use ModernLinker, vs LegacyLinker.
+    // Whether to use ModernLinker vs. LegacyLinker.
+    // Avoids locking: should be initialized very early.
     private boolean mUseModernLinker;
 
-    // Whether the configuration has been set.
+    // Whether the |mUseChromiumLinker| and |mUseModernLinker| configuration has been set.
+    // Avoids locking: should be initialized very early.
     private boolean mConfigurationSet;
 
-    @IntDef({LoadState.NOT_LOADED, LoadState.MAIN_DEX_LOADED, LoadState.LOADED})
-    @Retention(RetentionPolicy.SOURCE)
-    private @interface LoadState {
-        int NOT_LOADED = 0;
-        int MAIN_DEX_LOADED = 1;
-        int LOADED = 2;
-    }
+    // The type of process the shared library is loaded in.
+    // Avoids locking: should be initialized very early.
+    private @LibraryProcessType int mLibraryProcessType;
 
-    // Similar to |mLoaded| but is limited case of being loaded in app zygote.
+    // Makes sure non-Main Dex initialization happens only once. Does not use any class members
+    // except the volatile |mLoadState|.
+    private final Object mNonMainDexLock = new Object();
+
+    // Guards all the fields below.
+    private final Object mLock = new Object();
+
+    @GuardedBy("mLock")
+    private NativeLibraryPreloader mLibraryPreloader;
+
+    @GuardedBy("mLock")
+    private boolean mLibraryPreloaderCalled;
+
+    // Similar to |mLoadState| but is limited case of being loaded in app zygote.
     // This is exposed to clients.
+    @GuardedBy("mLock")
     private boolean mLoadedByZygote;
 
     // One-way switch becomes true when the Java command line is switched to
     // native.
+    @GuardedBy("mLock")
     private boolean mCommandLineSwitched;
 
-    // The type of process the shared library is loaded in.
-    private @LibraryProcessType int mLibraryProcessType;
-
     // The number of milliseconds it took to load all the native libraries, which
     // will be reported via UMA. Set once when the libraries are done loading.
+    @GuardedBy("mLock")
     private long mLibraryLoadTimeMs;
 
     /**
@@ -165,15 +172,15 @@ public class LibraryLoader {
      * Set native library preloader, if set, the NativeLibraryPreloader.loadLibrary will be invoked
      * before calling System.loadLibrary, this only applies when not using the chromium linker.
      *
-     * Since this function is called extremely early on in startup, locking is not required.
-     *
      * @param loader the NativeLibraryPreloader, it shall only be set once and before the
-     *               native library loaded.
+     *               native library is loaded.
      */
     public void setNativeLibraryPreloader(NativeLibraryPreloader loader) {
-        assert mLibraryPreloader == null;
-        assert mLoadState == LoadState.NOT_LOADED;
-        mLibraryPreloader = loader;
+        synchronized (mLock) {
+            assert mLibraryPreloader == null;
+            assert mLoadState == LoadState.NOT_LOADED;
+            mLibraryPreloader = loader;
+        }
     }
 
     /**
@@ -200,7 +207,7 @@ public class LibraryLoader {
     private void setLinkerImplementationIfNeededAlreadyLocked() {
         if (mConfigurationSet) return;
 
-        // Cannot use initializers for the variables below, as this makes roboelectric tests fail,
+        // Cannot use initializers for the fields below, as this makes roboelectric tests fail,
         // since they don't have a NativeLibraries class.
         mUseChromiumLinker = NativeLibraries.sUseLinker;
         mUseModernLinker = NativeLibraries.sUseModernLinker;
@@ -256,7 +263,9 @@ public class LibraryLoader {
      * Return if library is already loaded successfully by the zygote.
      */
     public boolean isLoadedByZygote() {
-        return mLoadedByZygote;
+        synchronized (mLock) {
+            return mLoadedByZygote;
+        }
     }
 
     /**
@@ -641,9 +650,11 @@ public class LibraryLoader {
 
     // Called after all native initializations are complete.
     public void onBrowserNativeInitializationComplete() {
-        if (mUseChromiumLinker) {
-            RecordHistogram.recordTimesHistogram(
-                    "ChromiumAndroidLinker.BrowserLoadTime", mLibraryLoadTimeMs);
+        synchronized (mLock) {
+            if (mUseChromiumLinker) {
+                RecordHistogram.recordTimesHistogram(
+                        "ChromiumAndroidLinker.BrowserLoadTime", mLibraryLoadTimeMs);
+            }
         }
     }
 

base/android/java/src/org/chromium/base/library_loader/Linker.java

@@ -348,8 +348,7 @@ public abstract class Linker {
     }
 
     /**
-     * Retrieve the base load address of the shared RELRO section.  This also enforces the creation
-     * of the shared RELRO section, which can later be retrieved with getSharedRelros().
+     * Retrieve the base load address of the library. This also enforces initializing the linker.
      *
      * @return a common, random base load address, or 0 if RELRO sharing is
      * disabled.