Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2268233004:
Fix HandlerLeak lint warning (Closed)
|
Created:
4 years, 3 months ago by hush (inactive) Modified:
4 years, 3 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix HandlerLeak lint warning
Suppress it, because fixing it with static inner classes proved
to be troublesome and not worth the effort.
BUG=640400
Committed: https://crrev.com/f148e4abd2cf574fcc82419c4163309a7eb85e1a
Cr-Commit-Position: refs/heads/master@{#415480}
Patch Set 1 #
Total comments: 4
Patch Set 2 : SuppressLint warning HandlerLeak. #
Total comments: 2
Patch Set 3 : fix comments #Messages
Total messages: 22 (6 generated)
hush@chromium.org changed reviewers: + boliu@chromium.org
PTAL
https://codereview.chromium.org/2268233004/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java (right): https://codereview.chromium.org/2268233004/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:115: private final WeakReference<WebView> mWeakWebView; I... don't think this is safe. It's actually conceivable app isn't holding onto the webview that created the pop up anymore, then gc could actually release it. so the whole premise of that warning just doesn't hold I hate these warnings, either it should be applied universally and break build, or they should just not be turned on at all https://codereview.chromium.org/2268233004/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:125: if (mWeakWebView.get() == null) { this is not c++ WeakPtr, gc can run at any time, and you need to hold a strong ref here to make sure it doesn't go away in this method
torne@chromium.org changed reviewers: + torne@chromium.org
https://codereview.chromium.org/2268233004/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java (right): https://codereview.chromium.org/2268233004/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:126: return; Just returning here would mean that even though the app *did* create a new WebView and pass it to us from onCreateWindow, the popup will never do anything and effectively be dead (if the app has managed to drop all strong references to the parent WebView at that point). This doesn't seem like a desirable outcome. I think we could only do this safely if we also invented a way to finish opening the popup successfully with a null parent - I have no idea if that's really possible..
https://codereview.chromium.org/2268233004/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java (right): https://codereview.chromium.org/2268233004/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:126: return; On 2016/08/24 15:31:36, Torne wrote: > Just returning here would mean that even though the app *did* create a new > WebView and pass it to us from onCreateWindow, the popup will never do anything > and effectively be dead (if the app has managed to drop all strong references to > the parent WebView at that point). This doesn't seem like a desirable outcome. > > I think we could only do this safely if we also invented a way to finish opening > the popup successfully with a null parent - I have no idea if that's really > possible.. It is. Have the native AwContents of the pop up be owned by the message probably. That probably gets into a lot of weird corner cases that I'd rather not think about too much. imo current thing is fine I will support merging all of our handlers so there is only one. There is another one for handling webview callbacks in AwContentsClientCallbackHelper
On 2016/08/24 16:08:59, boliu wrote: > https://codereview.chromium.org/2268233004/diff/1/android_webview/glue/java/s... > File > android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java > (right): > > https://codereview.chromium.org/2268233004/diff/1/android_webview/glue/java/s... > android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:126: > return; > On 2016/08/24 15:31:36, Torne wrote: > > Just returning here would mean that even though the app *did* create a new > > WebView and pass it to us from onCreateWindow, the popup will never do > anything > > and effectively be dead (if the app has managed to drop all strong references > to > > the parent WebView at that point). This doesn't seem like a desirable outcome. > > > > I think we could only do this safely if we also invented a way to finish > opening > > the popup successfully with a null parent - I have no idea if that's really > > possible.. > > It is. Have the native AwContents of the pop up be owned by the message > probably. That probably gets into a lot of weird corner cases that I'd rather > not think about too much. imo current thing is fine > > I will support merging all of our handlers so there is only one. There is > another one for handling webview callbacks in AwContentsClientCallbackHelper By current thing do you mean "this CL" or "as the code is already?"
On 2016/08/24 16:11:04, Torne wrote: > On 2016/08/24 16:08:59, boliu wrote: > > > https://codereview.chromium.org/2268233004/diff/1/android_webview/glue/java/s... > > File > > > android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java > > (right): > > > > > https://codereview.chromium.org/2268233004/diff/1/android_webview/glue/java/s... > > > android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:126: > > return; > > On 2016/08/24 15:31:36, Torne wrote: > > > Just returning here would mean that even though the app *did* create a new > > > WebView and pass it to us from onCreateWindow, the popup will never do > > anything > > > and effectively be dead (if the app has managed to drop all strong > references > > to > > > the parent WebView at that point). This doesn't seem like a desirable > outcome. > > > > > > I think we could only do this safely if we also invented a way to finish > > opening > > > the popup successfully with a null parent - I have no idea if that's really > > > possible.. > > > > It is. Have the native AwContents of the pop up be owned by the message > > probably. That probably gets into a lot of weird corner cases that I'd rather > > not think about too much. imo current thing is fine > > > > I will support merging all of our handlers so there is only one. There is > > another one for handling webview callbacks in AwContentsClientCallbackHelper > > By current thing do you mean "this CL" or "as the code is already?" "current" thing as in trunk, not this CL.
On 2016/08/24 16:13:15, boliu wrote: > On 2016/08/24 16:11:04, Torne wrote: > > On 2016/08/24 16:08:59, boliu wrote: > > > > > > https://codereview.chromium.org/2268233004/diff/1/android_webview/glue/java/s... > > > File > > > > > > android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2268233004/diff/1/android_webview/glue/java/s... > > > > > > android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:126: > > > return; > > > On 2016/08/24 15:31:36, Torne wrote: > > > > Just returning here would mean that even though the app *did* create a new > > > > WebView and pass it to us from onCreateWindow, the popup will never do > > > anything > > > > and effectively be dead (if the app has managed to drop all strong > > references > > > to > > > > the parent WebView at that point). This doesn't seem like a desirable > > outcome. > > > > > > > > I think we could only do this safely if we also invented a way to finish > > > opening > > > > the popup successfully with a null parent - I have no idea if that's > really > > > > possible.. > > > > > > It is. Have the native AwContents of the pop up be owned by the message > > > probably. That probably gets into a lot of weird corner cases that I'd > rather > > > not think about too much. imo current thing is fine > > > > > > I will support merging all of our handlers so there is only one. There is > > > another one for handling webview callbacks in AwContentsClientCallbackHelper > > > > By current thing do you mean "this CL" or "as the code is already?" > > "current" thing as in trunk, not this CL. After considering your comments, I think the weak reference thing does more harm than good: it affects correctness in some cases. I will just make a change to suppress the lint warnings instead.
PTAL
https://codereview.chromium.org/2268233004/diff/20001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java (right): https://codereview.chromium.org/2268233004/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:123: @SuppressLint("HandlerLeak") I say don't bother with the comment. it's bound to get out of date next time someone changes this Also I think general pattern is annotation after javadoc?
Also the CL description still says you're making it a static inner class and keeping a weak reference :)
Description was changed from ========== Fix HandlerLeak lint warning To fix it, we need to make the handler a static inner class and keep weak reference to the outer object. BUG=640400 ========== to ========== Fix HandlerLeak lint warning Suppress it, because fixing it with static inner classes proved to be troublesome and not worth the effort. BUG=640400 ==========
Fixed CL description. https://codereview.chromium.org/2268233004/diff/20001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java (right): https://codereview.chromium.org/2268233004/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:123: @SuppressLint("HandlerLeak") On 2016/08/24 21:25:43, boliu wrote: > I say don't bother with the comment. it's bound to get out of date next time > someone changes this > > Also I think general pattern is annotation after javadoc? Done.
lgtm remember to merge downstream
On 2016/08/30 21:59:38, boliu wrote: > lgtm > > remember to merge downstream thanks for the reminder
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix HandlerLeak lint warning Suppress it, because fixing it with static inner classes proved to be troublesome and not worth the effort. BUG=640400 ========== to ========== Fix HandlerLeak lint warning Suppress it, because fixing it with static inner classes proved to be troublesome and not worth the effort. BUG=640400 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix HandlerLeak lint warning Suppress it, because fixing it with static inner classes proved to be troublesome and not worth the effort. BUG=640400 ========== to ========== Fix HandlerLeak lint warning Suppress it, because fixing it with static inner classes proved to be troublesome and not worth the effort. BUG=640400 Committed: https://crrev.com/f148e4abd2cf574fcc82419c4163309a7eb85e1a Cr-Commit-Position: refs/heads/master@{#415480} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f148e4abd2cf574fcc82419c4163309a7eb85e1a Cr-Commit-Position: refs/heads/master@{#415480} |
