Skip to content
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.

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

fixed #17594: [android] fix crash and memory leaks if launching game multiple times on Android platform with PhysX module enabled. #17602

Merged
merged 9 commits into from
Sep 4, 2024
7 changes: 7 additions & 0 deletions native/cocos/application/BaseGame.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,13 @@
extern "C" void cc_load_all_plugins(); // NOLINT

namespace cc {

BaseGame::~BaseGame() { // NOLINT
#if (CC_PLATFORM == CC_PLATFORM_ANDROID) && CC_SUPPORT_ADPF
ADPFManager::getInstance().destroy();
#endif
}

int BaseGame::init() {
cc::pipeline::GlobalDSManager::setDescriptorSetLayout();

Expand Down
1 change: 1 addition & 0 deletions native/cocos/application/BaseGame.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class BaseGame : public CocosApplication {
};

BaseGame() = default;
~BaseGame() override;
int init() override;

protected:
Expand Down
11 changes: 11 additions & 0 deletions native/cocos/physics/physx/PhysXSharedBody.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ PhysXSharedBody *PhysXSharedBody::getSharedBody(const Node *node, PhysXWorld *co
PhysXSharedBody *newSB;
if (iter != sharedBodesMap.end()) {
newSB = iter->second;
CC_ASSERT_EQ(newSB->_mWrappedWorld, world);
} else {
newSB = ccnew PhysXSharedBody(const_cast<Node *>(node), world, body);
newSB->_mFilterData.word0 = 1;
Expand All @@ -84,6 +85,16 @@ PhysXSharedBody *PhysXSharedBody::getSharedBody(const Node *node, PhysXWorld *co
return newSB;
}

void PhysXSharedBody::clearCache() {
// Move the map to avoid erase operation in the following for loop since 'delete' will trigger ~PhysxSharedBody.
// clearCache is invoked only in the destructor of PhysXWorld.
auto tmpMap = std::move(sharedBodesMap);
Comment on lines +89 to +91
Copy link

Choose a reason for hiding this comment

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

style: Consider using a unique_ptr for automatic memory management of PhysXSharedBody instances

Comment on lines +89 to +91
Copy link

Choose a reason for hiding this comment

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

style: Consider using a unique_ptr for automatic memory management of PhysXSharedBody instances

for (auto &e : tmpMap) {
delete e.second;
}
sharedBodesMap.clear();
}

PhysXSharedBody::~PhysXSharedBody() {
sharedBodesMap.erase(_mNode);
if (_mStaticActor != nullptr) PX_RELEASE(_mStaticActor);
Expand Down
1 change: 1 addition & 0 deletions native/cocos/physics/physx/PhysXSharedBody.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class PhysXRigidBody;
class PhysXSharedBody final {
public:
static PhysXSharedBody *getSharedBody(const Node *node, PhysXWorld *world, PhysXRigidBody *body);
static void clearCache();
PhysXSharedBody() = delete;
PhysXSharedBody(const PhysXSharedBody &other) = delete;
PhysXSharedBody(PhysXSharedBody &&other) = delete;
Expand Down
2 changes: 2 additions & 0 deletions native/cocos/physics/physx/PhysXWorld.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ PhysXWorld::~PhysXWorld() {
// clear material cache
materialMap.clear();
delete _mEventMgr;
PhysXSharedBody::clearCache();
PhysXJoint::releaseTempRigidActor();
PX_RELEASE(_mControllerManager);
PX_RELEASE(_mScene);
Expand All @@ -119,6 +120,7 @@ PhysXWorld::~PhysXWorld() {
PX_RELEASE(_mCooking);
PxCloseExtensions();
PX_RELEASE(_mFoundation);
instance = nullptr;
}

void PhysXWorld::step(float fixedTimeStep) {
Expand Down
2 changes: 1 addition & 1 deletion native/cocos/physics/physx/PhysXWorld.h
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ class PhysXWorld final : virtual public IPhysicsWorld {
physx::PxDefaultCpuDispatcher *_mDispatcher;
physx::PxScene *_mScene;
PhysXEventManager *_mEventMgr;
uint32_t _mCollisionMatrix[31];
uint32_t _mCollisionMatrix[31] = {0};
ccstd::vector<PhysXSharedBody *> _mSharedBodies;
ccstd::vector<PhysXCharacterController *> _mCCTs;

Expand Down
6 changes: 6 additions & 0 deletions native/cocos/platform/android/adpf_manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,12 @@ void ADPFManager::initialize() {
}
}

void ADPFManager::destroy() {
JNIEnv *env = cc::JniHelper::getEnv();
env->DeleteGlobalRef(obj_power_service_);
Copy link

Choose a reason for hiding this comment

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

style: Consider checking if obj_power_service_ is not null before deleting

Copy link

Choose a reason for hiding this comment

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

style: Consider checking if obj_power_service_ is not null before deleting

obj_power_service_ = nullptr;
}

// Initialize JNI calls for the powermanager.
bool ADPFManager::initializePowerManager() {
#if __ANDROID_API__ >= 31
Expand Down
5 changes: 2 additions & 3 deletions native/cocos/platform/android/adpf_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@
* limitations under the License.
*/

#ifndef ADPF_MANAGER_H_
#define ADPF_MANAGER_H_
#pragma once

#if CC_PLATFORM == CC_PLATFORM_ANDROID && __ANDROID_API__ >= 30
#include <android/api-level.h>
Expand Down Expand Up @@ -115,6 +114,7 @@ class ADPFManager {
AThermalManager *getThermalManager() { return thermal_manager_; }

void initialize();
void destroy();

private:
// Update thermal headroom each sec.
Expand Down Expand Up @@ -185,4 +185,3 @@ class ADPFManager {
#define CC_SUPPORT_ADPF 0 // NOLINT
#endif // ADPF_MANAGER_H_

#endif
Binary file modified native/cocos/platform/android/java/libs/game-sdk.jar
Binary file not shown.
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,22 @@ protected void onDestroy() {
CocosHelper.unregisterBatteryLevelReceiver(this);
CocosAudioFocusManager.unregisterAudioFocusListener(this);
CanvasRenderingContext2DImpl.destroy();
CocosHelper.destroy();
GlobalObject.destroy();
CocosWebViewHelper.resetStaticVariables();
CocosSensorHandler.resetStaticVariables();

mVideoHelper.destroy();
mSurfaceView.setOnTouchListener(null);
mSurfaceView.getHolder().removeCallback(this);

mRootLayout.removeAllViews();
mRootLayout = null;

mSensorHandler = null;
mWebViewHelper = null;
mVideoHelper = null;
mSurfaceView = null;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ public class CocosHelper {
// ===========================================================

private static Vibrator sVibrateService;
private static BatteryReceiver sBatteryReceiver = new BatteryReceiver();
private static BatteryReceiver sBatteryReceiver = null;

public static final int NETWORK_TYPE_NONE = 0;
public static final int NETWORK_TYPE_LAN = 1;
Expand All @@ -98,6 +98,13 @@ public void addTask(Runnable runnable) {
sTaskQ.add(runnable);
}
}

public void clearTasks() {
synchronized (readMtx) {
sTaskQ.clear();
}
}

public void runTasks(){
Queue<Runnable> tmp;
synchronized (readMtx) {
Expand Down Expand Up @@ -135,12 +142,18 @@ public void setBatteryLevelByIntent(Intent intent) {
}

static void registerBatteryLevelReceiver(Context context) {
if (sBatteryReceiver == null) {
sBatteryReceiver = new BatteryReceiver();
}
Intent intent = context.registerReceiver(sBatteryReceiver, new IntentFilter(Intent.ACTION_BATTERY_CHANGED));
sBatteryReceiver.setBatteryLevelByIntent(intent);
}

static void unregisterBatteryLevelReceiver(Context context) {
context.unregisterReceiver(sBatteryReceiver);
if (sBatteryReceiver != null) {
context.unregisterReceiver(sBatteryReceiver);
sBatteryReceiver = null;
}
}

//Run on game thread forever, no matter foreground or background
Expand Down Expand Up @@ -197,6 +210,13 @@ public static void init() {
}
}

public static void destroy() {
sVibrateService = null;
sInited = false;
sTaskQOnGameThread.clearTasks();
sForegroundTaskQOnGameThread.clearTasks();
}

public static float getBatteryLevel() {
return sBatteryReceiver.sBatteryLevel;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,10 @@ public static boolean init(String dbName, String tableName) {
}

public static void destroy() {
mDatabaseOpenHelper = null;
Copy link

Choose a reason for hiding this comment

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

style: Consider adding a null check before setting mDatabaseOpenHelper to null

if (mDatabase != null) {
mDatabase.close();
mDatabase = null;
}
Comment on lines 55 to 60
Copy link

Choose a reason for hiding this comment

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

style: Consider adding a null check before closing mDatabase to prevent potential NullPointerException

}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ public CocosSensorHandler(final Context context) {
mSensorHandler = this;
}

public static void resetStaticVariables() {
mSensorHandler = null;
mEnableSensor = false;
}
Comment on lines +61 to +64
Copy link

Choose a reason for hiding this comment

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

style: Consider adding synchronization to prevent potential race conditions when resetting static variables.


// ===========================================================
// Getter & Setter
// ===========================================================
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ public class CocosVideoHelper {
private Activity mActivity = null;
private static SparseArray<CocosVideoView> sVideoViews = null;
static VideoHandler mVideoHandler = null;
private static Handler sHandler = null;

CocosVideoHelper(Activity activity, FrameLayout layout)
{
Expand All @@ -57,7 +56,16 @@ public class CocosVideoHelper {

mVideoHandler = new VideoHandler(this);
sVideoViews = new SparseArray<CocosVideoView>();
sHandler = new Handler(Looper.myLooper());
}

public void destroy() {
if (mVideoHandler != null) {
mVideoHandler.removeCallbacksAndMessages(null);
mVideoHandler = null;
}
Comment on lines +62 to +65
Copy link

Choose a reason for hiding this comment

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

style: Consider using a more specific method like removeCallbacks() instead of removeCallbacksAndMessages(null) if possible

Comment on lines +62 to +65
Copy link

Choose a reason for hiding this comment

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

style: Consider using a more specific method like removeCallbacks() instead of removeCallbacksAndMessages(null) if possible

videoEventListener = null;
mLayout = null;
mActivity = null;
}

private static int videoTag = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,15 @@ public CocosWebViewHelper(FrameLayout layout) {
CocosWebViewHelper.webViews = new SparseArray<CocosWebView>();
}

public static void resetStaticVariables() {
sLayout = null;
if (sHandler != null) {
sHandler.removeCallbacksAndMessages(null);
sHandler = null;
Comment on lines +62 to +63
Copy link

Choose a reason for hiding this comment

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

style: Ensure all callbacks are removed before setting sHandler to null to prevent potential memory leaks

}
webViews = null;
}

private static native boolean shouldStartLoading(int index, String message);
private static native void didFinishLoading(int index, String message);
private static native void didFailLoading(int index, String message);
Expand Down
12 changes: 11 additions & 1 deletion native/cocos/scene/RenderWindow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,17 @@ RenderWindow::RenderWindow()
_colorName("Color" + std::to_string(_renderWindowId)),
_depthStencilName("DepthStencil" + std::to_string(_renderWindowId)) {}

RenderWindow::~RenderWindow() = default;
RenderWindow::~RenderWindow() {
// NOTE: destroy needs to be invoked in the destructor of RenderWindow to avoid wild pointer issues in gfx backend code.
// RenderWindow owns `_frameBuffer` and `_colorTextures`, `_frameBuffer` should be released before `_colorTextures`
// since gfx::Framebuffer keeps a weak pointer of `_colorTextures` and there is code :
// GLES3Device::getInstance()->framebufferHub()->disengage(colorTexture->gpuTexture(), _gpuFBO);
minggo marked this conversation as resolved.
Show resolved Hide resolved
// in the GLES3Framebuffer::doDestroy.
// Invoking `destroy` here will make sure that `_frameBuffer` is destructed before `_colorTextures`,
// otherwise, the `colorTexture` in `disengage(colorTexture->gpuTexture(), _gpuFBO);` will be a wild pointer and
// colorTexture->gpuTexture() will be an invalid memory read operation.
destroy();
}

bool RenderWindow::initialize(gfx::Device *device, IRenderWindowInfo &info) {
if (info.title.has_value() && !info.title.value().empty()) {
Expand Down
2 changes: 1 addition & 1 deletion native/external-config.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@
"type": "github",
"owner": "cocos-creator",
"name": "engine-native-external",
"checkout": "v3.8.4-5"
"checkout": "v3.8.5-1"
}
}
Loading