Skip to content

fix(android): Leaking listeners #118

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

fix(android): Leaking listeners #118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f61f02b
Fix condition in RNPerformance.removeListener
mateuuszzzzz Sep 30, 2025
83cef16
Fix ReactMarker listener leak
mateuuszzzzz Sep 30, 2025
cf3542d
Use invalidate instead of deprecated onCatalystInstanceDestroyed and …
mateuuszzzzz Sep 30, 2025
1afbb37
Prevent leaking listener when getPackages is called multiple times
mateuuszzzzz Oct 10, 2025
a12516d
Remove unused method
mateuuszzzzz Oct 10, 2025
b846df6
Remove comments
mateuuszzzzz Oct 10, 2025
762adbc
Remove override
mateuuszzzzz Oct 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 74 additions & 69 deletions ...t-native-performance/android/src/main/java/com/oblador/performance/PerformanceModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,64 +25,82 @@ public class PerformanceModule extends ReactContextBaseJavaModule implements Tur
private static final Queue<PerformanceEntry> markBuffer = new ConcurrentLinkedQueue<>();
private static boolean didEmit = false;

private static final ReactMarker.MarkerListener startupMarkerListener = (name, tag, instanceKey) -> {
switch (name) {
case RELOAD:
clearMarkBuffer();
addMark(new PerformanceMark(BRIDGE_SETUP_START, SystemClock.uptimeMillis()));
break;
case ATTACH_MEASURED_ROOT_VIEWS_END:
case ATTACH_MEASURED_ROOT_VIEWS_START:
case BUILD_NATIVE_MODULE_REGISTRY_END:
case BUILD_NATIVE_MODULE_REGISTRY_START:
case CONTENT_APPEARED:
case CREATE_CATALYST_INSTANCE_END:
case CREATE_CATALYST_INSTANCE_START:
case CREATE_REACT_CONTEXT_END:
case CREATE_REACT_CONTEXT_START:
case CREATE_UI_MANAGER_MODULE_CONSTANTS_END:
case CREATE_UI_MANAGER_MODULE_CONSTANTS_START:
case CREATE_UI_MANAGER_MODULE_END:
case CREATE_UI_MANAGER_MODULE_START:
case CREATE_VIEW_MANAGERS_END:
case CREATE_VIEW_MANAGERS_START:
case DOWNLOAD_END:
case DOWNLOAD_START:
case LOAD_REACT_NATIVE_SO_FILE_END:
case LOAD_REACT_NATIVE_SO_FILE_START:
case PRE_RUN_JS_BUNDLE_START:
case PRE_SETUP_REACT_CONTEXT_END:
case PRE_SETUP_REACT_CONTEXT_START:
case PROCESS_CORE_REACT_PACKAGE_END:
case PROCESS_CORE_REACT_PACKAGE_START:
case REACT_CONTEXT_THREAD_END:
case REACT_CONTEXT_THREAD_START:
case RUN_JS_BUNDLE_END:
case RUN_JS_BUNDLE_START:
case SETUP_REACT_CONTEXT_END:
case SETUP_REACT_CONTEXT_START:
case VM_INIT:
long startTime = SystemClock.uptimeMillis();
addMark(new PerformanceMark(getMarkName(name), startTime));
break;
}
};

private final ReactMarker.MarkerListener contentAppearedListener = (name, tag, instanceKey) -> {
switch (name) {
case CONTENT_APPEARED:
eventsBuffered = false;
emitNativeStartupTime();
emitBufferedMarks();
break;
case RELOAD:
eventsBuffered = true;
break;
}
};

public PerformanceModule(@NonNull final ReactApplicationContext reactContext) {
super(reactContext);
setupMarkerListener();
setupNativeMarkerListener();
}

private void setupMarkerListener() {
ReactMarker.addListener(
contentAppearedListener
);
}
Comment on lines +90 to +94
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need a separate method for it :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I wanted to make this look similar to setupNativeMarkerListener which was already in the codebase


private void setupNativeMarkerListener() {
RNPerformance.getInstance().addListener(this);
}

// Need to set up the marker listener before the react module is initialized
// to capture all events
public static void setupListener() {
ReactMarker.addListener(
(name, tag, instanceKey) -> {
switch (name) {
case RELOAD:
clearMarkBuffer();
addMark(new PerformanceMark(BRIDGE_SETUP_START, SystemClock.uptimeMillis()));
break;
case ATTACH_MEASURED_ROOT_VIEWS_END:
case ATTACH_MEASURED_ROOT_VIEWS_START:
case BUILD_NATIVE_MODULE_REGISTRY_END:
case BUILD_NATIVE_MODULE_REGISTRY_START:
case CONTENT_APPEARED:
case CREATE_CATALYST_INSTANCE_END:
case CREATE_CATALYST_INSTANCE_START:
case CREATE_REACT_CONTEXT_END:
case CREATE_REACT_CONTEXT_START:
case CREATE_UI_MANAGER_MODULE_CONSTANTS_END:
case CREATE_UI_MANAGER_MODULE_CONSTANTS_START:
case CREATE_UI_MANAGER_MODULE_END:
case CREATE_UI_MANAGER_MODULE_START:
case CREATE_VIEW_MANAGERS_END:
case CREATE_VIEW_MANAGERS_START:
case DOWNLOAD_END:
case DOWNLOAD_START:
case LOAD_REACT_NATIVE_SO_FILE_END:
case LOAD_REACT_NATIVE_SO_FILE_START:
case PRE_RUN_JS_BUNDLE_START:
case PRE_SETUP_REACT_CONTEXT_END:
case PRE_SETUP_REACT_CONTEXT_START:
case PROCESS_CORE_REACT_PACKAGE_END:
case PROCESS_CORE_REACT_PACKAGE_START:
case REACT_CONTEXT_THREAD_END:
case REACT_CONTEXT_THREAD_START:
case RUN_JS_BUNDLE_END:
case RUN_JS_BUNDLE_START:
case SETUP_REACT_CONTEXT_END:
case SETUP_REACT_CONTEXT_START:
case VM_INIT:
long startTime = SystemClock.uptimeMillis();
addMark(new PerformanceMark(getMarkName(name), startTime));
break;

}
}
);
ReactMarker.addListener(startupMarkerListener);
}

private static void clearMarkBuffer() {
Expand Down Expand Up @@ -118,28 +136,19 @@ public String getName() {
return PERFORMANCE_MODULE;
}

public void addListener(String eventName) {

}

public void removeListeners(double count) {

}

private void emitNativeStartupTime() {
safelyEmitMark(new PerformanceMark("nativeLaunchStart", StartTimeProvider.getStartTime()));
safelyEmitMark(new PerformanceMark("nativeLaunchEnd", StartTimeProvider.getEndTime()));
}

private void setupMarkerListener() {
ReactMarker.addListener(
(name, tag, instanceKey) -> {
switch (name) {
case CONTENT_APPEARED:
eventsBuffered = false;
emitNativeStartupTime();
emitBufferedMarks();
break;
case RELOAD:
eventsBuffered = true;
break;
}
}
);
}

private void safelyEmitMark(PerformanceEntry entry) {
if (eventsBuffered) {
addMark(entry);
Expand Down Expand Up @@ -215,13 +224,9 @@ public void logMarker(PerformanceEntry entry) {
}

@Override
public void onCatalystInstanceDestroy() {
super.onCatalystInstanceDestroy();
public void invalidate() {
super.invalidate();
RNPerformance.getInstance().removeListener(this);
}

// Fix new arch runtime error
public void addListener(String eventName) {

ReactMarker.removeListener(contentAppearedListener);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we remove the other listener too?

Copy link
Author

@mateuuszzzzz mateuuszzzzz Oct 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

startupMarkerListener is specific, because once we add it to ReactMarker (singleton) it can stay there through the whole app lifetime.

I.e.:

  1. We add it once (by storing it in a static constant, ReactMarker will know that this listener has already been added, so it won’t add it again, preventing duplication when PerformancePackage constructor is called multiple times).
  2. We don’t need to remove it in invalidate because this listener isn’t bound to the native module’s lifetime.

}
}
2 changes: 1 addition & 1 deletion ...react-native-performance/android/src/main/java/com/oblador/performance/RNPerformance.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ protected void addListener(MarkerListener listener) {

@DoNotStrip
protected void removeListener(MarkerListener listener) {
if (!sListeners.contains(listener)) {
if (sListeners.contains(listener)) {
sListeners.remove(listener);
}
}
Expand Down