Issue 29397615: Issue 5053 - Release JsValues explicitly (Closed)
|
Created:
March 29, 2017, 12:12 p.m. by anton Modified:
March 30, 2017, 7:28 p.m. CC:
René Jeschke Visibility:
Public. |
DescriptionIssue 5053 - Release JsValues explicitly
Patch Set 1 #
Total comments: 5
MessagesTotal messages: 12
LGTM. Is the number of users of JsEngine before disposing is usually (there are still race conditions) exactly one?
On 2017/03/29 12:30:56, sergei wrote: > LGTM. > > Is the number of users of JsEngine before disposing is usually (there are still > race conditions) exactly one? yes, i've checked for 35 allocations and 35 disposals and refs count on FilterEngine to be 1: D/libabp-android_JNI: Disposing FilterEngine 0xae4a08f8 (0xae439f80), refs=1
On 2017/03/29 12:58:01, anton wrote: > On 2017/03/29 12:30:56, sergei wrote: > > LGTM. > > > > Is the number of users of JsEngine before disposing is usually (there are > still > > race conditions) exactly one? > > yes, i've checked for 35 allocations and 35 disposals and refs count on > FilterEngine to be 1: > > D/libabp-android_JNI: Disposing FilterEngine 0xae4a08f8 (0xae439f80), refs=1 For FilterEngine it was 1 before, what is about JsEngine?
On 2017/03/29 12:59:00, sergei wrote: > On 2017/03/29 12:58:01, anton wrote: > > On 2017/03/29 12:30:56, sergei wrote: > > > LGTM. > > > > > > Is the number of users of JsEngine before disposing is usually (there are > > still > > > race conditions) exactly one? > > > > yes, i've checked for 35 allocations and 35 disposals and refs count on > > FilterEngine to be 1: > > > > D/libabp-android_JNI: Disposing FilterEngine 0xae4a08f8 (0xae439f80), refs=1 > > For FilterEngine it was 1 before, what is about JsEngine? Just checked it to be 1: Disposing JsEngine 0xae833780 (0xae9991d0, count=1) Full log is available at https://gist.github.com/4ntoine/464e3d27d88a6fd56a7a296517b0e7ed
On 2017/03/29 19:14:31, anton wrote: > On 2017/03/29 12:59:00, sergei wrote: > > On 2017/03/29 12:58:01, anton wrote: > > > On 2017/03/29 12:30:56, sergei wrote: > > > > LGTM. > > > > > > > > Is the number of users of JsEngine before disposing is usually (there are > > > still > > > > race conditions) exactly one? > > > > > > yes, i've checked for 35 allocations and 35 disposals and refs count on > > > FilterEngine to be 1: > > > > > > D/libabp-android_JNI: Disposing FilterEngine 0xae4a08f8 (0xae439f80), refs=1 > > > > For FilterEngine it was 1 before, what is about JsEngine? > > Just checked it to be 1: > Disposing JsEngine 0xae833780 (0xae9991d0, count=1) > > Full log is available at > https://gist.github.com/4ntoine/464e3d27d88a6fd56a7a296517b0e7ed And this log is with navigating to some URL (95 allocations and disposals, engine refs = 1) https://gist.github.com/4ntoine/fa69f1b7c57e4a15d09fed7d7b8ef8b7
https://codereview.adblockplus.org/29397615/diff/29397616/libadblockplus-andr... File libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java (right): https://codereview.adblockplus.org/29397615/diff/29397616/libadblockplus-andr... libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:996: if (debugMode) Isn't this check redundant here, since d() already checks for debugMode? https://codereview.adblockplus.org/29397615/diff/29397616/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (right): https://codereview.adblockplus.org/29397615/diff/29397616/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:236: JsValue jsTitle = jsSubscription.getProperty("title"); If JsValue implemented AutoCloseable, we could avoid some finally blocks, by using try-with-resources
https://codereview.adblockplus.org/29397615/diff/29397616/libadblockplus-andr... File libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java (right): https://codereview.adblockplus.org/29397615/diff/29397616/libadblockplus-andr... libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:996: if (debugMode) On 2017/03/30 14:19:29, diegocarloslima wrote: > Isn't this check redundant here, since d() already checks for debugMode? Acknowledged. Bu it's scope of scope of this task/review. Create separate issue for it if necessary. https://codereview.adblockplus.org/29397615/diff/29397616/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (right): https://codereview.adblockplus.org/29397615/diff/29397616/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:236: JsValue jsTitle = jsSubscription.getProperty("title"); On 2017/03/30 14:19:29, diegocarloslima wrote: > If JsValue implemented AutoCloseable, we could avoid some finally blocks, by > using try-with-resources i've been thinking about it and it makes no sense - the user can just forget to use `try-with-resources` just like forget to invoke `dispose()` is it correct?
https://codereview.adblockplus.org/29397615/diff/29397616/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (right): https://codereview.adblockplus.org/29397615/diff/29397616/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:236: JsValue jsTitle = jsSubscription.getProperty("title"); On 2017/03/30 14:34:15, anton wrote: > On 2017/03/30 14:19:29, diegocarloslima wrote: > > If JsValue implemented AutoCloseable, we could avoid some finally blocks, by > > using try-with-resources > > i've been thinking about it and it makes no sense - > the user can just forget to use `try-with-resources` just like forget to invoke > `dispose()` > > is it correct? Yeah, he can forget on both approaches. The advantage of using try-with-resources is that we won't need a boilerplate code 'finally { object.dispose()}' block every time we want to dispose a locally created variable, making the code cleaner. I would even suggest to make Disposable extend AutoCloseable, so every Disposable child can be used in a try-with-resources block. Since this change might be kinda unrelated to this issue, we can discuss further and do it in a separate issue.
On 2017/03/30 14:47:18, diegocarloslima wrote: > https://codereview.adblockplus.org/29397615/diff/29397616/libadblockplus-andr... > File > libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java > (right): > > https://codereview.adblockplus.org/29397615/diff/29397616/libadblockplus-andr... > libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:236: > JsValue jsTitle = jsSubscription.getProperty("title"); > On 2017/03/30 14:34:15, anton wrote: > > On 2017/03/30 14:19:29, diegocarloslima wrote: > > > If JsValue implemented AutoCloseable, we could avoid some finally blocks, by > > > using try-with-resources > > > > i've been thinking about it and it makes no sense - > > the user can just forget to use `try-with-resources` just like forget to > invoke > > `dispose()` > > > > is it correct? > > Yeah, he can forget on both approaches. yes, that was the key point i decided not use Closeable/Autocloseable for now. > The advantage of using > try-with-resources is that we won't need a boilerplate code 'finally { > object.dispose()}' block every time we want to dispose a locally created > variable, making the code cleaner. I would even suggest to make Disposable > extend AutoCloseable, so every Disposable child can be used in a > try-with-resources block. Since this change might be kinda unrelated to this > issue, we can discuss further and do it in a separate issue.
On 2017/03/30 14:47:18, diegocarloslima wrote: > https://codereview.adblockplus.org/29397615/diff/29397616/libadblockplus-andr... > File > libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java > (right): > > https://codereview.adblockplus.org/29397615/diff/29397616/libadblockplus-andr... > libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:236: > JsValue jsTitle = jsSubscription.getProperty("title"); > On 2017/03/30 14:34:15, anton wrote: > > On 2017/03/30 14:19:29, diegocarloslima wrote: > > > If JsValue implemented AutoCloseable, we could avoid some finally blocks, by > > > using try-with-resources > > > > i've been thinking about it and it makes no sense - > > the user can just forget to use `try-with-resources` just like forget to > invoke > > `dispose()` > > > > is it correct? > > Yeah, he can forget on both approaches. The advantage of using > try-with-resources is that we won't need a boilerplate code 'finally { > object.dispose()}' block every time we want to dispose a locally created > variable, making the code cleaner. I would even suggest to make Disposable > extend AutoCloseable, so every Disposable child can be used in a > try-with-resources block. Since this change might be kinda unrelated to this > issue, we can discuss further and do it in a separate issue. LGTM, since my considerations are most likely to be addressed (if ever) in a separate issue
LGTM - not pretty, but I don't see another way. I guess try-with-resources will make this pretty again. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
