Merge "NativeBridge: Refactor for new initialization flow"

include/nativebridge/native_bridge.h

@@ -24,14 +24,24 @@ namespace android {
 
 struct NativeBridgeRuntimeCallbacks;
 
-// Initialize the native bridge, if any. Should be called by Runtime::Init().
+// Open the native bridge, if any. Should be called by Runtime::Init(). A null library filename
-// A null library filename signals that we do not want to load a native bridge.
+// signals that we do not want to load a native bridge.
-void SetupNativeBridge(const char* native_bridge_library_filename,
+bool LoadNativeBridge(const char* native_bridge_library_filename,
-                       const NativeBridgeRuntimeCallbacks* runtime_callbacks);
+                      const NativeBridgeRuntimeCallbacks* runtime_callbacks);
+
+// Initialize the native bridge, if any. Should be called by Runtime::DidForkFromZygote.
+bool InitializeNativeBridge();
+
+// Unload the native bridge, if any. Should be called by Runtime::DidForkFromZygote.
+void UnloadNativeBridge();
+
+// Check whether a native bridge is available (opened or initialized). Requires a prior call to
+// LoadNativeBridge.
+bool NativeBridgeAvailable();
 
 // Check whether a native bridge is available (initialized). Requires a prior call to
-// SetupNativeBridge to make sense.
+// LoadNativeBridge & InitializeNativeBridge.
-bool NativeBridgeAvailable();
+bool NativeBridgeInitialized();
 
 // Load a shared library that is supported by the native bridge.
 void* NativeBridgeLoadLibrary(const char* libpath, int flag);

libnativebridge/Android.mk

@@ -36,3 +36,5 @@ LOCAL_LDFLAGS := -ldl
 LOCAL_MULTILIB := both
 
 include $(BUILD_HOST_SHARED_LIBRARY)
+
+include $(LOCAL_PATH)/tests/Android.mk

libnativebridge/native_bridge.cc

@@ -19,27 +19,53 @@
 #include <cutils/log.h>
 #include <dlfcn.h>
 #include <stdio.h>
-#include "utils/Mutex.h"
 
 
 namespace android {
 
-static Mutex native_bridge_lock("native bridge lock");
-
 // The symbol name exposed by native-bridge with the type of NativeBridgeCallbacks.
 static constexpr const char* kNativeBridgeInterfaceSymbol = "NativeBridgeItf";
 
-// The filename of the library we are supposed to load.
+enum class NativeBridgeState {
-static const char* native_bridge_library_filename = nullptr;
+  kNotSetup,                        // Initial state.
+  kOpened,                          // After successful dlopen.
+  kInitialized,                     // After successful initialization.
+  kClosed                           // Closed or errors.
+};
+
+static const char* kNotSetupString = "kNotSetup";
+static const char* kOpenedString = "kOpened";
+static const char* kInitializedString = "kInitialized";
+static const char* kClosedString = "kClosed";
+
+static const char* GetNativeBridgeStateString(NativeBridgeState state) {
+  switch (state) {
+    case NativeBridgeState::kNotSetup:
+      return kNotSetupString;
+
+    case NativeBridgeState::kOpened:
+      return kOpenedString;
+
+    case NativeBridgeState::kInitialized:
+      return kInitializedString;
+
+    case NativeBridgeState::kClosed:
+      return kClosedString;
+  }
+}
+
+// Current state of the native bridge.
+static NativeBridgeState state = NativeBridgeState::kNotSetup;
 
-// Whether a native bridge is available (loaded and ready).
-static bool available = false;
-// Whether we have already initialized (or tried to).
-static bool initialized = false;
 // Whether we had an error at some point.
 static bool had_error = false;
 
+// Handle of the loaded library.
+static void* native_bridge_handle = nullptr;
+// Pointer to the callbacks. Available as soon as LoadNativeBridge succeeds, but only initialized
+// later.
 static NativeBridgeCallbacks* callbacks = nullptr;
+// Callbacks provided by the environment to the bridge. Passed to LoadNativeBridge.
 static const NativeBridgeRuntimeCallbacks* runtime_callbacks = nullptr;
 
 // Characters allowed in a native bridge filename. The first character must
@@ -83,81 +109,99 @@ bool NativeBridgeNameAcceptable(const char* nb_library_filename) {
   }
 }
 
-void SetupNativeBridge(const char* nb_library_filename,
+bool LoadNativeBridge(const char* nb_library_filename,
-                       const NativeBridgeRuntimeCallbacks* runtime_cbs) {
+                      const NativeBridgeRuntimeCallbacks* runtime_cbs) {
-  Mutex::Autolock auto_lock(native_bridge_lock);
+  // We expect only one place that calls LoadNativeBridge: Runtime::Init. At that point we are not
+  // multi-threaded, so we do not need locking here.
 
-  if (initialized || native_bridge_library_filename != nullptr) {
+  if (state != NativeBridgeState::kNotSetup) {
     // Setup has been called before. Ignore this call.
-    ALOGW("Called SetupNativeBridge for an already set up native bridge.");
+    ALOGW("Called LoadNativeBridge for an already set up native bridge. State is %s.",
+          GetNativeBridgeStateString(state));
     // Note: counts as an error, even though the bridge may be functional.
     had_error = true;
-    return;
-  }
-
-  runtime_callbacks = runtime_cbs;
-
-  if (nb_library_filename == nullptr) {
-    available = false;
-    initialized = true;
-  } else {
-    // Check whether it's an empty string.
-    if (*nb_library_filename == 0) {
-      available = false;
-      initialized = true;
-    } else if (!NativeBridgeNameAcceptable(nb_library_filename)) {
-      available = false;
-      initialized = true;
-      had_error = true;
-    }
-
-    if (!initialized) {
-      // Didn't find a name error or empty string, assign it.
-      native_bridge_library_filename = nb_library_filename;
-    }
-  }
-}
-
-static bool NativeBridgeInitialize() {
-  Mutex::Autolock auto_lock(native_bridge_lock);
-
-  if (initialized) {
-    // Somebody did it before.
-    return available;
-  }
-
-  available = false;
-
-  if (native_bridge_library_filename == nullptr) {
-    // Called initialize without setup. dlopen has special semantics for nullptr input.
-    // So just call it a day here. This counts as an error.
-    initialized = true;
-    had_error = true;
     return false;
   }
 
-  void* handle = dlopen(native_bridge_library_filename, RTLD_LAZY);
+  if (nb_library_filename == nullptr || *nb_library_filename == 0) {
-  if (handle != nullptr) {
+    state = NativeBridgeState::kClosed;
-    callbacks = reinterpret_cast<NativeBridgeCallbacks*>(dlsym(handle,
+    return true;
-                                                               kNativeBridgeInterfaceSymbol));
+  } else {
-
+    if (!NativeBridgeNameAcceptable(nb_library_filename)) {
-    if (callbacks != nullptr) {
+      state = NativeBridgeState::kClosed;
-      available = callbacks->initialize(runtime_callbacks);
-    }
-
-    if (!available) {
-      // If we fail initialization, this counts as an error.
       had_error = true;
-      dlclose(handle);
+    } else {
+      // Try to open the library.
+      void* handle = dlopen(nb_library_filename, RTLD_LAZY);
+      if (handle != nullptr) {
+        callbacks = reinterpret_cast<NativeBridgeCallbacks*>(dlsym(handle,
+                                                                   kNativeBridgeInterfaceSymbol));
+        if (callbacks != nullptr) {
+          // Store the handle for later.
+          native_bridge_handle = handle;
+        } else {
+          dlclose(handle);
+        }
+      }
+
+      // Two failure conditions: could not find library (dlopen failed), or could not find native
+      // bridge interface (dlsym failed). Both are an error and close the native bridge.
+      if (callbacks == nullptr) {
+        had_error = true;
+        state = NativeBridgeState::kClosed;
+      } else {
+        runtime_callbacks = runtime_cbs;
+        state = NativeBridgeState::kOpened;
+      }
+    }
+    return state == NativeBridgeState::kOpened;
+  }
+}
+
+bool InitializeNativeBridge() {
+  // We expect only one place that calls InitializeNativeBridge: Runtime::DidForkFromZygote. At that
+  // point we are not multi-threaded, so we do not need locking here.
+
+  if (state == NativeBridgeState::kOpened) {
+    // Try to initialize.
+    if (callbacks->initialize(runtime_callbacks)) {
+      state = NativeBridgeState::kInitialized;
+    } else {
+      // Unload the library.
+      dlclose(native_bridge_handle);
+      had_error = true;
+      state = NativeBridgeState::kClosed;
     }
   } else {
-    // Being unable to open the library counts as an error.
     had_error = true;
+    state = NativeBridgeState::kClosed;
   }
 
-  initialized = true;
+  return state == NativeBridgeState::kInitialized;
+}
 
-  return available;
+void UnloadNativeBridge() {
+  // We expect only one place that calls UnloadNativeBridge: Runtime::DidForkFromZygote. At that
+  // point we are not multi-threaded, so we do not need locking here.
+
+  switch(state) {
+    case NativeBridgeState::kOpened:
+    case NativeBridgeState::kInitialized:
+      // Unload.
+      dlclose(native_bridge_handle);
+      break;
+
+    case NativeBridgeState::kNotSetup:
+      // Not even set up. Error.
+      had_error = true;
+      break;
+
+    case NativeBridgeState::kClosed:
+      // Ignore.
+      break;
+  }
+
+  state = NativeBridgeState::kClosed;
 }
 
 bool NativeBridgeError() {
@@ -165,11 +209,17 @@ bool NativeBridgeError() {
 }
 
 bool NativeBridgeAvailable() {
-  return NativeBridgeInitialize();
+  return state == NativeBridgeState::kOpened || state == NativeBridgeState::kInitialized;
+}
+
+bool NativeBridgeInitialized() {
+  // Calls of this are supposed to happen in a state where the native bridge is stable, i.e., after
+  // Runtime::DidForkFromZygote. In that case we do not need a lock.
+  return state == NativeBridgeState::kInitialized;
 }
 
 void* NativeBridgeLoadLibrary(const char* libpath, int flag) {
-  if (NativeBridgeInitialize()) {
+  if (NativeBridgeInitialized()) {
     return callbacks->loadLibrary(libpath, flag);
   }
   return nullptr;
@@ -177,14 +227,14 @@ void* NativeBridgeLoadLibrary(const char* libpath, int flag) {
 
 void* NativeBridgeGetTrampoline(void* handle, const char* name, const char* shorty,
                                 uint32_t len) {
-  if (NativeBridgeInitialize()) {
+  if (NativeBridgeInitialized()) {
     return callbacks->getTrampoline(handle, name, shorty, len);
   }
   return nullptr;
 }
 
 bool NativeBridgeIsSupported(const char* libpath) {
-  if (NativeBridgeInitialize()) {
+  if (NativeBridgeInitialized()) {
     return callbacks->isSupported(libpath);
   }
   return false;

libnativebridge/tests/InvalidCharsNativeBridge_test.cpp

@@ -23,7 +23,7 @@ static const char* kTestName = "../librandom$@-bridge_not.existing.so";
 TEST_F(NativeBridgeTest, InvalidChars) {
     // Do one test actually calling setup.
     EXPECT_EQ(false, NativeBridgeError());
-    SetupNativeBridge(kTestName, nullptr);
+    LoadNativeBridge(kTestName, nullptr);
     // This should lead to an error for invalid characters.
     EXPECT_EQ(true, NativeBridgeError());
 

libnativebridge/tests/ReSetupNativeBridge_test.cpp

@@ -18,13 +18,11 @@
 
 namespace android {
 
-static const char* kTestName = "librandom-bridge_not.existing.so";
-
 TEST_F(NativeBridgeTest, ReSetup) {
     EXPECT_EQ(false, NativeBridgeError());
-    SetupNativeBridge(kTestName, nullptr);
+    LoadNativeBridge("", nullptr);
     EXPECT_EQ(false, NativeBridgeError());
-    SetupNativeBridge(kTestName, nullptr);
+    LoadNativeBridge("", nullptr);
     // This should lead to an error for trying to re-setup a native bridge.
     EXPECT_EQ(true, NativeBridgeError());
 }

libnativebridge/tests/UnavailableNativeBridge_test.cpp

@@ -20,9 +20,10 @@ namespace android {
 
 TEST_F(NativeBridgeTest, NoNativeBridge) {
     EXPECT_EQ(false, NativeBridgeAvailable());
-    // This should lead to an error for trying to initialize a not-setup
+    // Try to initialize. This should fail as we are not set up.
-    // native bridge.
+    EXPECT_EQ(false, InitializeNativeBridge());
     EXPECT_EQ(true, NativeBridgeError());
+    EXPECT_EQ(false, NativeBridgeAvailable());
 }
 
 }  // namespace android

libnativebridge/tests/ValidNameNativeBridge_test.cpp

@@ -21,13 +21,15 @@ namespace android {
 static const char* kTestName = "librandom-bridge_not.existing.so";
 
 TEST_F(NativeBridgeTest, ValidName) {
+    // Check that the name is acceptable.
+    EXPECT_EQ(true, NativeBridgeNameAcceptable(kTestName));
+
+    // Now check what happens on LoadNativeBridge.
     EXPECT_EQ(false, NativeBridgeError());
-    SetupNativeBridge(kTestName, nullptr);
+    LoadNativeBridge(kTestName, nullptr);
-    EXPECT_EQ(false, NativeBridgeError());
+    // This will lead to an error as the library doesn't exist.
-    EXPECT_EQ(false, NativeBridgeAvailable());
-    // This should lead to an error for trying to initialize a not-existing
-    // native bridge.
     EXPECT_EQ(true, NativeBridgeError());
+    EXPECT_EQ(false, NativeBridgeAvailable());
 }
 
 }  // namespace android