Fix a crash in AppBannerInfoBarDelegateAndroid on native app installation.
https://crrev.com/c/915442 restructured the ownership of objects in app banner Java code. In particular, InstallerDelegate was moved from AppBannerInfoBarDelegateAndroid to AppBannerUiDelegate. For native apps, InstallerDelegate is given AppBannerInfoBarDelegateAndroid as an observer, and tells it about the progress of installation. Prior to https://crrev.com/c/915442, when AppBannerInfoBarDelegateAndroid was destroyed, it would also destroy the InstallerDelegate, ensuring that it could not be signalled after destruction. However, there is now the following potential for the following sequence of (racy) actions: 1. C++ AppBannerInfoBarDelegateAndroid is destroyed. In its destructor, destroy() is called on its Java counterpart, resetting the native pointer on the Java side. 2. Race: prior to the AppBannerUiDelegate owned by the C++ AppBannerInfoBarDelegateAndroid being destroyed, the InstallerDelegate owned by AppBannerUiDelegate calls onInstallFinished on the Java-side AppBannerInfoBarDelegateAndroid. This will eventually call OnInstallFinished. 3. Meanwhile, the C++ AppBannerInfoBarDelegateAndroid's destructor continues, clearing the |ui_delegate_| field. This destroys the AppBannerUiDelegate and its owned InstallerDelegate. 4. The C++ AppBannerInfoBarDelegateAndroid has OnInstallFinished called on it (from 2), which attempts to access |ui_delegate_| and crashes with a segmentation fault. This CL fixes the bug by moving 3 (clearing the |ui_delegate_| field) to happen before 1 (calling destroy() on the Java-side AppBannerInfoBarDelegateAndroid). This ensures the InstallerDelegate is destroyed before the native pointer on the Java-side AppBannerInfoBarDelegateAndroid is reset, so that any further progress in installation is not signalled as destruction continues. This prevents accessing |ui_delegate_| after it is cleared, and prevents the crash. BUG=819434 Change-Id: Iab0f7cb9044da332b6d4c6ab0a6f1d41a72195ca Reviewed-on: https://chromium-review.googlesource.com/959409 Commit-Queue: Dominick Ng <dominickn@chromium.org> Reviewed-by:Ben Wells <benwells@chromium.org> Cr-Commit-Position: refs/heads/master@{#542688}
Showing
Please register or sign in to comment