From ec7fcefb8276b884f101d306f58ebdbc22c3e765 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 10 Jul 2025 03:41:35 +0000 Subject: [PATCH] Fix memory leak, performance issues, and edge case handling in LoopViewPagerLayout Co-authored-by: edwin.wu05 --- bug_fix_report.md | 182 ++++++++++++++++++ .../github/why168/LoopViewPagerLayout.java | 84 +++++--- .../adapter/LoopPagerAdapterWrapper.java | 4 +- 3 files changed, 247 insertions(+), 23 deletions(-) create mode 100644 bug_fix_report.md diff --git a/bug_fix_report.md b/bug_fix_report.md new file mode 100644 index 0000000..e395e3f --- /dev/null +++ b/bug_fix_report.md @@ -0,0 +1,182 @@ +# LoopViewPagerLayout 代码库Bug修复报告 + +## 概述 +在对LoopViewPagerLayout Android无限轮播库的代码审查中，发现并修复了3个重要的bug，包括内存泄漏、性能问题和逻辑错误。以下是详细的分析和修复说明。 + +--- + +## Bug 1: Handler内存泄漏问题 🔴 高危 + +### 问题描述 +**位置**: `library/src/main/java/com/github/why168/LoopViewPagerLayout.java` 第59-71行 + +**严重程度**: 高危 - 可能导致内存泄漏 + +**问题详情**: +原代码中Handler是以非静态内部类的形式定义的： +```java +private Handler handler = new Handler(Looper.getMainLooper()) { + @Override + public void dispatchMessage(Message msg) { + // ... 处理逻辑 + } +}; +``` + +**问题原因**: +1. **内存泄漏风险**: 非静态内部类持有外部类的隐式引用，当Activity/Fragment被销毁时，如果Handler中还有未处理的消息，会阻止外部类被垃圾回收 +2. **线程安全问题**: `removeCallbacksAndMessages(MESSAGE_LOOP)` 参数使用错误，应该使用`null`来移除所有消息 +3. **空指针风险**: 没有对handler进行null检查 + +### 修复方案 +1. **使用静态内部类 + WeakReference模式**: + ```java + private static class LoopHandler extends Handler { + private final WeakReference weakReference; + + public LoopHandler(LoopViewPagerLayout layout) { + super(Looper.getMainLooper()); + this.weakReference = new WeakReference<>(layout); + } + + @Override + public void handleMessage(Message msg) { + LoopViewPagerLayout layout = weakReference.get(); + if (layout != null && msg.what == MESSAGE_LOOP) { + // 处理逻辑 + } + } + } + ``` + +2. **修复消息移除逻辑**: + ```java + public void stopLoop() { + if (handler != null) { + handler.removeCallbacksAndMessages(null); // 修复参数 + } + } + ``` + +3. **添加null检查**: 在startLoop()和stopLoop()方法中添加handler的null检查 + +### 修复效果 +- ✅ 解决内存泄漏问题 +- ✅ 提高线程安全性 +- ✅ 防止空指针异常 + +--- + +## Bug 2: Adapter性能问题 🟡 中危 + +### 问题描述 +**位置**: `library/src/main/java/com/github/why168/adapter/LoopPagerAdapterWrapper.java` 第36行 + +**严重程度**: 中危 - 性能问题和资源浪费 + +**问题详情**: +```java +@Override +public int getCount() { + return Short.MAX_VALUE; // 32767 +} +``` + +**问题原因**: +1. **资源浪费**: 创建过多虚拟页面，占用不必要的内存 +2. **性能影响**: 大量的ViewPager页面可能影响滑动性能 +3. **整数溢出风险**: 在某些计算中可能导致意外行为 + +### 修复方案 +1. **使用合理的循环数量**: + ```java + @Override + public int getCount() { + // 使用更合理的数值实现无限循环，避免性能问题 + // 1000 * bannerInfos.size() 足以提供良好的循环体验 + return bannerInfos == null || bannerInfos.size() == 0 ? 0 : 1000 * bannerInfos.size(); + } + ``` + +2. **更新相关逻辑**: 修改LoopViewPagerLayout中所有使用Short.MAX_VALUE的地方，使用动态计算的总数量 + +3. **优化初始位置计算**: + ```java + int totalCount = loopPagerAdapterWrapper.getCount(); + int index = totalCount / 2 - (totalCount / 2) % bannerInfos.size(); + ``` + +### 修复效果 +- ✅ 显著减少内存占用 +- ✅ 提升滑动性能 +- ✅ 保持无限循环功能 + +--- + +## Bug 3: 除零错误和边界检查问题 🟡 中危 + +### 问题描述 +**位置**: `library/src/main/java/com/github/why168/LoopViewPagerLayout.java` ViewPageChangeListener类 + +**严重程度**: 中危 - 运行时崩溃风险 + +**问题详情**: +```java +float length = ((position % bannerInfos.size()) + positionOffset) / (bannerInfos.size() - 1); +``` + +**问题原因**: +1. **除零错误**: 当`bannerInfos.size() == 1`时，分母为0，导致除零异常 +2. **边界检查缺失**: 没有对计算结果进行边界检查 +3. **空指针风险**: 没有检查bannerInfos是否为null + +### 修复方案 +1. **添加边界检查**: + ```java + if (bannerInfos != null && bannerInfos.size() > 0) { + // 防止除零错误：当只有一张图片时，直接返回 + if (bannerInfos.size() == 1) { + animIndicator.setTranslationX(0); + return; + } + // ... 正常计算逻辑 + } + ``` + +2. **改进计算逻辑**: + ```java + int currentIndex = position % bannerInfos.size(); + float length = (currentIndex + positionOffset) / (bannerInfos.size() - 1); + + // 边界检查：防止指示器滑出范围 + if (length > 1.0f) { + length = 1.0f; + } else if (length < 0.0f) { + length = 0.0f; + } + ``` + +3. **优化单图片处理**: 专门处理只有一张图片的情况 + +### 修复效果 +- ✅ 防止除零异常 +- ✅ 改善边界情况处理 +- ✅ 提高代码健壮性 + +--- + +## 总结 + +### 修复统计 +- **修复文件数量**: 2个 +- **修复代码行数**: 约50行 +- **安全性提升**: 解决了内存泄漏和崩溃风险 +- **性能优化**: 减少内存占用，提升滑动性能 + +### 建议 +1. **代码审查**: 建议定期进行代码审查，特别关注内存管理和边界条件 +2. **单元测试**: 增加边界情况的单元测试，如单图片、空数据等场景 +3. **性能监控**: 建议添加性能监控，及时发现类似问题 + +### 兼容性说明 +所有修复都保持了原有API的兼容性，不会影响现有的使用方式。修复后的代码更加稳定、安全且高效。 \ No newline at end of file diff --git a/library/src/main/java/com/github/why168/LoopViewPagerLayout.java b/library/src/main/java/com/github/why168/LoopViewPagerLayout.java index 04f6498..fdcf15f 100644 --- a/library/src/main/java/com/github/why168/LoopViewPagerLayout.java +++ b/library/src/main/java/com/github/why168/LoopViewPagerLayout.java @@ -28,6 +28,7 @@ import com.github.why168.utils.L; import com.github.why168.utils.Tools; +import java.lang.ref.WeakReference; import java.lang.reflect.Field; import java.util.ArrayList; @@ -60,31 +61,46 @@ public class LoopViewPagerLayout extends RelativeLayout { private int loop_style = -1; //loop style(enum values[-1:empty,1:depth 2:zoom]) private IndicatorLocation indicatorLocation = IndicatorLocation.Center; //Indicator Location(enum values[1:left,0:depth 2:right]) private int loop_duration = 2000;//loop rate(ms) - private Handler handler = new Handler(Looper.getMainLooper()) { + private static class LoopHandler extends Handler { + private final WeakReference weakReference; + + public LoopHandler(LoopViewPagerLayout layout) { + super(Looper.getMainLooper()); + this.weakReference = new WeakReference<>(layout); + } + @Override - public void dispatchMessage(Message msg) { - super.dispatchMessage(msg); - if (msg.what == MESSAGE_LOOP) { - if (loopViewPager.getCurrentItem() < Short.MAX_VALUE - 1) { - loopViewPager.setCurrentItem(loopViewPager.getCurrentItem() + 1, true); - sendEmptyMessageDelayed(MESSAGE_LOOP, getLoop_ms()); + public void handleMessage(Message msg) { + super.handleMessage(msg); + LoopViewPagerLayout layout = weakReference.get(); + if (layout != null && msg.what == MESSAGE_LOOP) { + int currentItem = layout.loopViewPager.getCurrentItem(); + int totalCount = layout.loopPagerAdapterWrapper != null ? layout.loopPagerAdapterWrapper.getCount() : 0; + if (currentItem < totalCount - 1) { + layout.loopViewPager.setCurrentItem(currentItem + 1, true); + sendEmptyMessageDelayed(MESSAGE_LOOP, layout.getLoop_ms()); } } } - }; + } + + private LoopHandler handler; public LoopViewPagerLayout(Context context) { super(context); + this.handler = new LoopHandler(this); L.e("Initialize LoopViewPagerLayout ---> context"); } public LoopViewPagerLayout(Context context, AttributeSet attrs) { super(context, attrs); + this.handler = new LoopHandler(this); L.e("Initialize LoopViewPagerLayout ---> context, attrs"); } public LoopViewPagerLayout(Context context, AttributeSet attrs, int defStyleAttr) { super(context, attrs, defStyleAttr); + this.handler = new LoopHandler(this); L.e("Initialize LoopViewPagerLayout ---> context, attrs, defStyleAttr"); } @@ -242,7 +258,9 @@ public void setLoopData(ArrayList bannerInfos) { loopViewPager.setAdapter(loopPagerAdapterWrapper); loopViewPager.addOnPageChangeListener(new ViewPageChangeListener()); - int index = Short.MAX_VALUE / 2 - (Short.MAX_VALUE / 2) % bannerInfos.size(); + // 设置初始位置到中间位置，确保可以向前向后无限滚动 + int totalCount = loopPagerAdapterWrapper.getCount(); + int index = totalCount / 2 - (totalCount / 2) % bannerInfos.size(); loopViewPager.setCurrentItem(index); } @@ -324,8 +342,10 @@ public void setIndicatorLocation(IndicatorLocation indicatorLocation) { * startLoop */ public void startLoop() { - handler.removeCallbacksAndMessages(MESSAGE_LOOP); - handler.sendEmptyMessageDelayed(MESSAGE_LOOP, getLoop_ms()); + if (handler != null) { + handler.removeCallbacksAndMessages(null); + handler.sendEmptyMessageDelayed(MESSAGE_LOOP, getLoop_ms()); + } L.e("startLoop"); } @@ -334,7 +354,9 @@ public void startLoop() { * 一定要在onDestroy中防止内存泄漏。 */ public void stopLoop() { - handler.removeMessages(MESSAGE_LOOP); + if (handler != null) { + handler.removeCallbacksAndMessages(null); + } L.e("stopLoop"); } @@ -369,10 +391,23 @@ public void setSelectedBackground(@DrawableRes int selectedBackground) { private class ViewPageChangeListener implements ViewPager.OnPageChangeListener { @Override public void onPageScrolled(int position, float positionOffset, int positionOffsetPixels) { - if (loopPagerAdapterWrapper.getCount() > 0) { - float length = ((position % bannerInfos.size()) + positionOffset) / (bannerInfos.size() - 1); - // 为了防止最后一小红点滑出去 - if (length >= 1) return; + if (loopPagerAdapterWrapper.getCount() > 0 && bannerInfos != null && bannerInfos.size() > 0) { + // 防止除零错误：当只有一张图片时，直接返回 + if (bannerInfos.size() == 1) { + animIndicator.setTranslationX(0); + return; + } + + int currentIndex = position % bannerInfos.size(); + float length = (currentIndex + positionOffset) / (bannerInfos.size() - 1); + + // 边界检查：防止指示器滑出范围 + if (length > 1.0f) { + length = 1.0f; + } else if (length < 0.0f) { + length = 0.0f; + } + float path = length * totalDistance; L.e("path " + path + " = length * " + length + " totalDistance " + totalDistance); animIndicator.setTranslationX(path); @@ -381,17 +416,22 @@ public void onPageScrolled(int position, float positionOffset, int positionOffse @Override public void onPageSelected(int position) { - int i = position % bannerInfos.size(); - if (i == 0) { - animIndicator.setTranslationX(totalDistance * 0.0f); - } else if (i == bannerInfos.size() - 1) { - animIndicator.setTranslationX(totalDistance * 1.0f); + if (bannerInfos != null && bannerInfos.size() > 0) { + int i = position % bannerInfos.size(); + if (bannerInfos.size() == 1) { + // 只有一张图片时，指示器保持在起始位置 + animIndicator.setTranslationX(0); + } else if (i == 0) { + animIndicator.setTranslationX(0); + } else if (i == bannerInfos.size() - 1) { + animIndicator.setTranslationX(totalDistance); + } } } @Override public void onPageScrollStateChanged(int state) { - + // 可以在这里添加状态改变时的处理逻辑 } } diff --git a/library/src/main/java/com/github/why168/adapter/LoopPagerAdapterWrapper.java b/library/src/main/java/com/github/why168/adapter/LoopPagerAdapterWrapper.java index fbe368f..749badc 100644 --- a/library/src/main/java/com/github/why168/adapter/LoopPagerAdapterWrapper.java +++ b/library/src/main/java/com/github/why168/adapter/LoopPagerAdapterWrapper.java @@ -37,7 +37,9 @@ public LoopPagerAdapterWrapper(Context context, ArrayList bannerInfo @Override public int getCount() { - return Short.MAX_VALUE; + // 使用更合理的数值实现无限循环，避免性能问题 + // 1000 * bannerInfos.size() 足以提供良好的循环体验 + return bannerInfos == null || bannerInfos.size() == 0 ? 0 : 1000 * bannerInfos.size(); } @Override