diff --git a/android-plugin/app/src/trace/AndroidManifest.xml b/android-plugin/app/src/trace/AndroidManifest.xml index f20e2313..a160bab1 100644 --- a/android-plugin/app/src/trace/AndroidManifest.xml +++ b/android-plugin/app/src/trace/AndroidManifest.xml @@ -52,8 +52,11 @@ android:name="pojavEnv" tools:node="remove" /> + diff --git a/android-plugin/app/src/trace/cpp/trace_replay_jni.cpp b/android-plugin/app/src/trace/cpp/trace_replay_jni.cpp index 4dc91ca8..08320ad4 100644 --- a/android-plugin/app/src/trace/cpp/trace_replay_jni.cpp +++ b/android-plugin/app/src/trace/cpp/trace_replay_jni.cpp @@ -2,6 +2,7 @@ #include "spawn_spike.hpp" #include "trace_env_overrides.hpp" +#include "trace_replay_lease.hpp" #include #include @@ -119,6 +120,18 @@ Java_top_mobilegl_plugin_trace_TraceReplayActivity_nativeRunTraceReplay(JNIEnv* jboolean benchmarkFinish, jstring benchmarkResultPath, jstring envOverrides) { + // Claim before changing GLWS globals, environment, logs or output files. The lease + // outlives replayState, so another invocation cannot race native-window cleanup. + mobilegl_trace::TraceReplayLease lease; + if (!lease) { + mobilegl_trace::Result rejected; + rejected.statusCode = mobilegl_trace::STATUS_RETRACE_FAILED; + rejected.message = "another trace replay is already running in this process"; + // No paths: a duplicate Activity can share outputDir with the live invocation. + // Publishing this failure there would overwrite that invocation's real result. + return MakeResult(env, rejected); + } + mobilegl_trace::Request request; request.tracePath = ToString(env, tracePath); request.goldenPath = ToString(env, goldenPath); @@ -157,12 +170,20 @@ Java_top_mobilegl_plugin_trace_TraceReplayActivity_nativeRunTraceReplay(JNIEnv* request.benchmarkResultPath = ToString(env, benchmarkResultPath); request.envOverrides = SplitSemicolonList(ToString(env, envOverrides)); - ScopedTraceReplayState replayState; - mobilegl_trace_set_requested_size(request.width, request.height); const bool needsNativeWindow = request.backend == "DirectVulkan" || (request.backend == "DirectGLES" && !request.usePbuffer); ANativeWindow *window = needsNativeWindow && surface != nullptr ? ANativeWindow_fromSurface(env, surface) : nullptr; + if (needsNativeWindow && window == nullptr) { + auto result = MakeFailureResult(request, mobilegl_trace::STATUS_RETRACE_FAILED, + "render surface was destroyed before native replay started"); + if (!result.resultPath.empty()) { + mobilegl_trace::WriteResultJson(request, result); + } + return MakeResult(env, result); + } + ScopedTraceReplayState replayState; + mobilegl_trace_set_requested_size(request.width, request.height); mobilegl_trace_set_native_window(window); if (window != nullptr) { ANativeWindow_release(window); diff --git a/android-plugin/app/src/trace/cpp/trace_replay_lease.hpp b/android-plugin/app/src/trace/cpp/trace_replay_lease.hpp new file mode 100644 index 00000000..b0c86ee7 --- /dev/null +++ b/android-plugin/app/src/trace/cpp/trace_replay_lease.hpp @@ -0,0 +1,28 @@ +#pragma once + +#include + +namespace mobilegl_trace { + +// apitrace, its GLWS adapter and the environment are process-global. --singlethread +// controls trace dispatch only; it does not make two JNI invocations independent. +class TraceReplayLease { +public: + TraceReplayLease() : acquired_(!running_.test_and_set(std::memory_order_acquire)) {} + ~TraceReplayLease() { + if (acquired_) { + running_.clear(std::memory_order_release); + } + } + + TraceReplayLease(const TraceReplayLease&) = delete; + TraceReplayLease& operator=(const TraceReplayLease&) = delete; + + explicit operator bool() const { return acquired_; } + +private: + inline static std::atomic_flag running_ = ATOMIC_FLAG_INIT; + const bool acquired_; +}; + +} // namespace mobilegl_trace diff --git a/android-plugin/app/src/trace/java/top/mobilegl/plugin/trace/TraceReplayActivity.java b/android-plugin/app/src/trace/java/top/mobilegl/plugin/trace/TraceReplayActivity.java index a7ef470c..a3038f2c 100644 --- a/android-plugin/app/src/trace/java/top/mobilegl/plugin/trace/TraceReplayActivity.java +++ b/android-plugin/app/src/trace/java/top/mobilegl/plugin/trace/TraceReplayActivity.java @@ -25,7 +25,8 @@ public final class TraceReplayActivity extends Activity { private TextView statusView; private TraceReplayRequest request; - private boolean started; + private TraceReplaySession replaySession; + private final TraceReplaySession.Listener replayListener = this::onReplayComplete; private final Handler mainHandler = new Handler(Looper.getMainLooper()); @Override @@ -62,6 +63,18 @@ public final class TraceReplayActivity extends Activity { return; } + @SuppressWarnings("unchecked") + TraceReplaySession retained = + (TraceReplaySession) getLastNonConfigurationInstance(); + replaySession = retained != null ? retained : new TraceReplaySession<>( + command -> new Thread(command, "MobileGLTraceReplay").start(), + command -> new Handler(Looper.getMainLooper()).post(command)); + if (replaySession.hasStarted()) { + Log.i(TAG, "Reattaching to retained trace replay: " + request.outputDir); + statusView.setText("Running trace replay\n" + request.outputDir); + } + replaySession.attach(replayListener); + SurfaceHolder holder = surfaceView.getHolder(); if (request.width > 0 && request.height > 0) { holder.setFixedSize(request.width, request.height); @@ -87,20 +100,43 @@ public final class TraceReplayActivity extends Activity { mainHandler.postDelayed(() -> startReplay(holder), 250); } + @Override + public Object onRetainNonConfigurationInstance() { + return replaySession; + } + + @Override + protected void onDestroy() { + // A callback queued by an old Surface must not start after that Activity is gone. + mainHandler.removeCallbacksAndMessages(null); + if (replaySession != null) { + replaySession.detach(replayListener); + } + super.onDestroy(); + } + private void startReplay(SurfaceHolder holder) { - if (started) { + if (isFinishing() || isDestroyed() || replaySession.hasStarted()) { return; } Surface surface = holder.getSurface(); if (surface == null || !surface.isValid()) { return; } - started = true; statusView.setText("Running trace replay\n" + request.outputDir); - new Thread(() -> runRequest(request, surface), "MobileGLTraceReplay").start(); + TraceReplayRequest replayRequest = request; + // The worker retains the request and Surface, not the Activity. JNI also acquires + // its own ANativeWindow reference until the replay and native cleanup finish. + replaySession.start(() -> runRequest(replayRequest, surface)); } - private void runRequest(TraceReplayRequest request, Surface surface) { + private void onReplayComplete(TraceReplayResult result) { + statusView.setText(result.toString()); + finish(); + } + + private static TraceReplayResult runRequest(TraceReplayRequest request, Surface surface) { + Log.i(TAG, "Starting native trace replay: " + request.outputDir); TraceReplayResult result = nativeRunTraceReplay( surface, request.tracePath, @@ -135,11 +171,7 @@ public final class TraceReplayActivity extends Activity { request.envOverrides ); Log.i(TAG, result.toString()); - TraceReplayResult finalResult = result; - runOnUiThread(() -> { - statusView.setText(finalResult.toString()); - finish(); - }); + return result; } private static native TraceReplayResult nativeRunTraceReplay( @@ -202,9 +234,7 @@ public final class TraceReplayActivity extends Activity { } private void runSpawnSpike(String libraryName) { - // The surface callbacks fire regardless; this keeps them from starting a replay - // underneath the spike. - started = true; + // onCreate returns before installing replay surface callbacks in this mode. File outputDir = new File(request.outputDir); String serverPath = new File(getApplicationInfo().nativeLibraryDir, libraryName) .getAbsolutePath(); diff --git a/android-plugin/app/src/trace/java/top/mobilegl/plugin/trace/TraceReplaySession.java b/android-plugin/app/src/trace/java/top/mobilegl/plugin/trace/TraceReplaySession.java new file mode 100644 index 00000000..116ce40b --- /dev/null +++ b/android-plugin/app/src/trace/java/top/mobilegl/plugin/trace/TraceReplaySession.java @@ -0,0 +1,59 @@ +package top.mobilegl.plugin.trace; + +import java.util.concurrent.Executor; +import java.util.function.Supplier; + +/** One native invocation, retained when Android replaces the Activity that displays it. */ +final class TraceReplaySession { + interface Listener { + void onComplete(T result); + } + + private final Executor worker; + private final Executor completion; + private boolean started; + private boolean completed; + private T result; + private Listener listener; + + TraceReplaySession(Executor worker, Executor completion) { + this.worker = worker; + this.completion = completion; + } + + // All session state is accessed on the main thread. Only replay.get() runs on the worker. + boolean hasStarted() { + return started; + } + + boolean start(Supplier replay) { + if (started) { + return false; + } + started = true; + worker.execute(() -> { + T replayResult = replay.get(); + completion.execute(() -> { + result = replayResult; + completed = true; + if (listener != null) { + listener.onComplete(result); + } + }); + }); + return true; + } + + void attach(Listener newListener) { + listener = newListener; + if (completed) { + listener.onComplete(result); + } + } + + void detach(Listener oldListener) { + if (listener == oldListener) { + listener = null; + } + } +} diff --git a/android-plugin/app/src/trace/res/values/config.xml b/android-plugin/app/src/trace/res/values/config.xml new file mode 100644 index 00000000..b65885dc --- /dev/null +++ b/android-plugin/app/src/trace/res/values/config.xml @@ -0,0 +1,11 @@ + + + + 0x80000f80 + diff --git a/tools/trace_replay/CMakeLists.txt b/tools/trace_replay/CMakeLists.txt index 4534e037..81229cb2 100644 --- a/tools/trace_replay/CMakeLists.txt +++ b/tools/trace_replay/CMakeLists.txt @@ -298,6 +298,17 @@ if(NOT CMAKE_CROSSCOMPILING) COMMAND mobilegl_trace_env_overrides_test COMMENT "Checking the retrace env passthrough parser") add_dependencies(mobilegl_trace_replay mobilegl_trace_env_overrides_test) + + add_executable(mobilegl_trace_replay_lease_test + "${MOBILEGL_TRACE_ROOT}/trace_replay_lease_test.cpp") + target_compile_features(mobilegl_trace_replay_lease_test PRIVATE cxx_std_17) + target_include_directories(mobilegl_trace_replay_lease_test PRIVATE + "${MOBILEGL_TRACE_SHARED_CPP_DIR}") + target_link_libraries(mobilegl_trace_replay_lease_test PRIVATE Threads::Threads) + add_custom_command(TARGET mobilegl_trace_replay_lease_test POST_BUILD + COMMAND mobilegl_trace_replay_lease_test + COMMENT "Checking process ownership of native trace replay") + add_dependencies(mobilegl_trace_replay mobilegl_trace_replay_lease_test) endif() if(MOBILEGL_TRACE_REPLAY_MOBILEGL_LIBRARY) @@ -464,4 +475,4 @@ execute_process( --fixture-root "${MOBILEGL_TRACE_ROOT}/fixtures" OUTPUT_FILE "${MOBILEGL_TRACE_CASES_CMAKE}" COMMAND_ERROR_IS_FATAL ANY) -include("${MOBILEGL_TRACE_CASES_CMAKE}") \ No newline at end of file +include("${MOBILEGL_TRACE_CASES_CMAKE}") diff --git a/tools/trace_replay/TraceReplaySessionTest.java b/tools/trace_replay/TraceReplaySessionTest.java new file mode 100644 index 00000000..2813217e --- /dev/null +++ b/tools/trace_replay/TraceReplaySessionTest.java @@ -0,0 +1,119 @@ +package top.mobilegl.plugin.trace; + +import java.util.ArrayDeque; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.Executor; +import java.util.concurrent.TimeUnit; + +/** Deterministic Activity replacement while native work or its completion is pending. */ +public final class TraceReplaySessionTest { + private static final class Queue implements Executor { + final ArrayDeque commands = new ArrayDeque<>(); + + @Override + public void execute(Runnable command) { + commands.add(command); + } + + void drain() { + while (!commands.isEmpty()) { + commands.remove().run(); + } + } + } + + private static void require(boolean condition, String message) { + if (!condition) { + throw new AssertionError(message); + } + } + + public static void main(String[] args) throws InterruptedException { + Queue worker = new Queue(); + Queue main = new Queue(); + TraceReplaySession session = new TraceReplaySession<>(worker, main); + List oldResults = new ArrayList<>(); + List newResults = new ArrayList<>(); + TraceReplaySession.Listener oldActivity = oldResults::add; + TraceReplaySession.Listener newActivity = newResults::add; + int[] nativeCalls = {0}; + + // Recreation before a valid Surface: the replacement may start the first run. + session.attach(oldActivity); + session.detach(oldActivity); + session.attach(newActivity); + require(session.start(() -> { ++nativeCalls[0]; return "real native result"; }), + "replacement must be able to start a pending request"); + require(session.hasStarted(), "ownership must be published before worker dispatch"); + + // surfaceCreated and surfaceChanged can both queue callbacks, and a recreated + // Activity receives both again. None may dispatch another native invocation. + session.detach(newActivity); + session.attach(oldActivity); + require(!session.start(() -> { ++nativeCalls[0]; return "duplicate"; }), + "Activity recreation must retain the running invocation"); + worker.drain(); + require(nativeCalls[0] == 1, "only one worker may execute the native request"); + + // The native result can arrive between the old Activity's destruction and the + // replacement attaching. It must be kept, without calling a destroyed owner. + session.detach(oldActivity); + main.drain(); + require(oldResults.isEmpty() && newResults.isEmpty(), "destroyed owners must not be notified"); + session.attach(newActivity); + require(newResults.equals(List.of("real native result")), "replacement must receive original result"); + require(!session.start(() -> "replayed completed request"), "completed request must not replay"); + + // An obsolete owner must not detach the current owner, including while running. + TraceReplaySession next = new TraceReplaySession<>(worker, main); + next.attach(oldActivity); + next.attach(newActivity); + next.detach(oldActivity); + next.start(() -> "next native result"); + worker.drain(); + main.drain(); + require(oldResults.isEmpty(), "obsolete owner was notified"); + require(newResults.equals(List.of("real native result", "next native result")), + "late detach lost the current owner"); + + // Replace the observer while the worker is actually inside the native-call + // boundary, not just queued. Latches force the interleaving without sleeps. + CountDownLatch entered = new CountDownLatch(1); + CountDownLatch release = new CountDownLatch(1); + List threads = new ArrayList<>(); + TraceReplaySession running = new TraceReplaySession<>(command -> { + Thread thread = new Thread(command); + threads.add(thread); + thread.start(); + }, main); + running.attach(oldActivity); + running.start(() -> { + entered.countDown(); + try { + if (!release.await(5, TimeUnit.SECONDS)) { + throw new AssertionError("test did not release the native invocation"); + } + } catch (InterruptedException exception) { + throw new AssertionError(exception); + } + return "running native result"; + }); + require(entered.await(5, TimeUnit.SECONDS), "native worker did not enter"); + running.detach(oldActivity); + running.attach(newActivity); + boolean duplicate = running.start(() -> "duplicate while running"); + release.countDown(); + for (Thread thread : threads) { + thread.join(5000); + require(!thread.isAlive(), "native worker did not finish"); + } + require(!duplicate, "recreation entered native code while original invocation was active"); + main.drain(); + require(oldResults.isEmpty(), "running invocation notified destroyed Activity"); + require(newResults.equals(List.of("real native result", "next native result", "running native result")), + "running invocation lost its original result during Activity replacement"); + System.out.println("TraceReplaySession: pending, running and completed recreation passed"); + } +} diff --git a/tools/trace_replay/test_android_lifecycle.py b/tools/trace_replay/test_android_lifecycle.py new file mode 100644 index 00000000..ba205c95 --- /dev/null +++ b/tools/trace_replay/test_android_lifecycle.py @@ -0,0 +1,39 @@ +#!/usr/bin/env python3 +"""Run the Android replay ownership regression without an emulator or Gradle.""" + +import pathlib +import subprocess +import tempfile +import xml.etree.ElementTree as ET + + +def main(): + root = pathlib.Path(__file__).resolve().parents[2] + trace = root / "android-plugin/app/src/trace" + android = "{http://schemas.android.com/apk/res/android}" + manifest = ET.parse(trace / "AndroidManifest.xml") + activity = next(a for a in manifest.findall("application/activity") + if a.get(android + "name") == ".trace.TraceReplayActivity") + config_value = activity.get(android + "configChanges", "0") + if config_value == "@integer/trace_replay_config_changes": + resources = ET.parse(trace / "res/values/config.xml") + config_value = resources.find("integer[@name='trace_replay_config_changes']").text + changes = int(config_value, 0) + # The failing emulator added resource overlays after replay had already started. + # orientation/screenSize alone does not handle CONFIG_ASSETS_PATHS. + if changes & 0x80000000 == 0: + raise RuntimeError("TraceReplayActivity must handle CONFIG_ASSETS_PATHS") + if changes & 0xF80 != 0xF80: + raise RuntimeError("TraceReplayActivity must retain the fixed-size render surface on display changes") + with tempfile.TemporaryDirectory(prefix="mobilegl-replay-lifecycle-") as output: + subprocess.run([ + "javac", "--release", "11", "-d", output, + str(trace / "java/top/mobilegl/plugin/trace/TraceReplaySession.java"), + str(root / "tools/trace_replay/TraceReplaySessionTest.java"), + ], check=True) + subprocess.run(["java", "-cp", output, "top.mobilegl.plugin.trace.TraceReplaySessionTest"], check=True) + print("Android replay manifest and lifecycle checks passed") + + +if __name__ == "__main__": + main() diff --git a/tools/trace_replay/trace_replay_lease_test.cpp b/tools/trace_replay/trace_replay_lease_test.cpp new file mode 100644 index 00000000..8fd85ec7 --- /dev/null +++ b/tools/trace_replay/trace_replay_lease_test.cpp @@ -0,0 +1,46 @@ +#include "trace_replay_lease.hpp" + +#include +#include +#include + +int main() { + using mobilegl_trace::TraceReplayLease; + std::atomic rejected{0}; + { + TraceReplayLease original; + if (!original) { + std::cerr << "initial replay could not claim process state\n"; + return 1; + } + std::vector contenders; + for (int i = 0; i < 16; ++i) { + contenders.emplace_back([&] { + TraceReplayLease duplicate; + if (!duplicate) { + ++rejected; + } + }); + } + for (auto& thread : contenders) { + thread.join(); + } + if (rejected != 16) { + std::cerr << "a duplicate replay entered the live process state\n"; + return 1; + } + // A rejected caller's destructor must not release the real owner's claim. + TraceReplayLease stillDuplicate; + if (stillDuplicate) { + std::cerr << "rejected caller released the original replay's claim\n"; + return 1; + } + } + TraceReplayLease next; + if (!next) { + std::cerr << "completed replay did not release process state\n"; + return 1; + } + std::cout << "TraceReplayLease: concurrent invocations rejected; owner cleanup retained\n"; + return 0; +}