Issue 29361556: Issue 4591 - Crash after dispose (Closed)
|
Created:
Nov. 2, 2016, 12:50 p.m. by anton Modified:
Dec. 2, 2016, 5:50 a.m. CC:
René Jeschke Visibility:
Public. |
DescriptionIssue 4591 - Crash after dispose
Patch Set 1 #
Total comments: 3
MessagesTotal messages: 8
https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (right): https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:156: return create(context, appInfo, basePath, elemhideEnabled, null, null, null, null); Is this callback cleanup related to the crash? If not, It might be better to do it in a separate issue
https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (right): https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:156: return create(context, appInfo, basePath, elemhideEnabled, null, null, null, null); On 2016/11/04 10:35:15, diegocarloslima wrote: > Is this callback cleanup related to the crash? If not, It might be better to do > it in a separate issue Yes, it's related as since we use smart pointers callback is fired after we no longer need it and it causes crash since vm is released (GetJavaVM() returns NULL (https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...) and cause NRE with sigsegv 11 (see issue log attachments)).
On 2016/11/07 07:35:14, anton wrote: > https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-andr... > File > libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java > (right): > > https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-andr... > libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:156: > return create(context, appInfo, basePath, elemhideEnabled, null, null, null, > null); > On 2016/11/04 10:35:15, diegocarloslima wrote: > > Is this callback cleanup related to the crash? If not, It might be better to > do > > it in a separate issue > > Yes, it's related as since we use smart pointers callback is fired after we no > longer need it and it causes crash since vm is released (GetJavaVM() returns > NULL > (https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...) > and cause NRE with sigsegv 11 (see issue log attachments)). LGTM
Looks good, one question. https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (right): https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:156: return create(context, appInfo, basePath, elemhideEnabled, null, null, null, null); On 2016/11/07 07:35:14, anton wrote: > On 2016/11/04 10:35:15, diegocarloslima wrote: > > Is this callback cleanup related to the crash? If not, It might be better to > do > > it in a separate issue > > Yes, it's related as since we use smart pointers callback is fired after we no > longer need it and it causes crash since vm is released (GetJavaVM() returns > NULL > (https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...) > and cause NRE with sigsegv 11 (see issue log attachments)). So what if the user passes these callback objects in? Wouldn't we then have the exact same problem as with our static versions? (Not that we seem to need those for anything but logging.)
On 2016/11/18 06:56:55, Felix Dahlke wrote: > Looks good, one question. > > https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-andr... > File > libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java > (right): > > https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-andr... > libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:156: > return create(context, appInfo, basePath, elemhideEnabled, null, null, null, > null); > On 2016/11/07 07:35:14, anton wrote: > > On 2016/11/04 10:35:15, diegocarloslima wrote: > > > Is this callback cleanup related to the crash? If not, It might be better to > > do > > > it in a separate issue > > > > Yes, it's related as since we use smart pointers callback is fired after we no > > longer need it and it causes crash since vm is released (GetJavaVM() returns > > NULL > > > (https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...) > > and cause NRE with sigsegv 11 (see issue log attachments)). > > So what if the user passes these callback objects in? Wouldn't we then have the > exact same problem as with our static versions? (Not that we seem to need those > for anything but logging.) Yes, i've been thinking about it and i removed static callbacks since they are pretty useless (do the logging only). However the user is still able to create his own callbacks and pass them. We're removing callbacks on dispose() and this is the point so it does not make sense if it's user's callbacks or our own static callbacks.
On 2016/11/21 07:41:56, anton wrote: > On 2016/11/18 06:56:55, Felix Dahlke wrote: > https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-andr... > > File > > > libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java > > (right): > > > > > https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-andr... > > > libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:156: > > return create(context, appInfo, basePath, elemhideEnabled, null, null, null, > > null); > > On 2016/11/07 07:35:14, anton wrote: > > > On 2016/11/04 10:35:15, diegocarloslima wrote: > > > > Is this callback cleanup related to the crash? If not, It might be better > to > > > do > > > > it in a separate issue > > > > > > Yes, it's related as since we use smart pointers callback is fired after we > no > > > longer need it and it causes crash since vm is released (GetJavaVM() returns > > > NULL > > > > > > (https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...) > > > and cause NRE with sigsegv 11 (see issue log attachments)). > > > > So what if the user passes these callback objects in? Wouldn't we then have > the > > exact same problem as with our static versions? (Not that we seem to need > those > > for anything but logging.) > > Yes, i've been thinking about it and i removed static callbacks since they are > pretty useless (do the logging only). However the user is still able to create > his own callbacks and pass them. > We're removing callbacks on dispose() and this is the point so it does not make > sense if it's user's callbacks or our own static callbacks. Ah, so removing the static callbacks isn't actually part of the fix, just a cleanup? Fair enough, LGTM.
On 2016/11/21 08:51:42, Felix Dahlke wrote: > On 2016/11/21 07:41:56, anton wrote: > > On 2016/11/18 06:56:55, Felix Dahlke wrote: > > > https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-andr... > > > File > > > > > > libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java > > > (right): > > > > > > > > > https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-andr... > > > > > > libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:156: > > > return create(context, appInfo, basePath, elemhideEnabled, null, null, null, > > > null); > > > On 2016/11/07 07:35:14, anton wrote: > > > > On 2016/11/04 10:35:15, diegocarloslima wrote: > > > > > Is this callback cleanup related to the crash? If not, It might be > better > > to > > > > do > > > > > it in a separate issue > > > > > > > > Yes, it's related as since we use smart pointers callback is fired after > we > > no > > > > longer need it and it causes crash since vm is released (GetJavaVM() > returns > > > > NULL > > > > > > > > > > (https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...) > > > > and cause NRE with sigsegv 11 (see issue log attachments)). > > > > > > So what if the user passes these callback objects in? Wouldn't we then have > > the > > > exact same problem as with our static versions? (Not that we seem to need > > those > > > for anything but logging.) > > > > Yes, i've been thinking about it and i removed static callbacks since they are > > pretty useless (do the logging only). However the user is still able to create > > his own callbacks and pass them. > > We're removing callbacks on dispose() and this is the point so it does not > make > > sense if it's user's callbacks or our own static callbacks. > > Ah, so removing the static callbacks isn't actually part of the fix, just a > cleanup? > > Fair enough, LGTM. Yes, i found them almost unuseful. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||
