Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(666)

Issue 29361556: Issue 4591 - Crash after dispose (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 7 months ago by anton
Modified:
3 years, 6 months ago
CC:
René Jeschke
Visibility:
Public.

Description

Issue 4591 - Crash after dispose

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+29 lines, -56 lines) Patch
M libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java View 4 chunks +29 lines, -56 lines 3 comments Download

Messages

Total messages: 8
anton
3 years, 7 months ago (2016-11-02 13:24:53 UTC) #1
diegocarloslima
https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (right): https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java#newcode156 libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:156: return create(context, appInfo, basePath, elemhideEnabled, null, null, null, null); ...
3 years, 7 months ago (2016-11-04 10:35:15 UTC) #2
anton
https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (right): https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java#newcode156 libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:156: return create(context, appInfo, basePath, elemhideEnabled, null, null, null, null); ...
3 years, 7 months ago (2016-11-07 07:35:14 UTC) #3
diegocarloslima
On 2016/11/07 07:35:14, anton wrote: > https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java > File > libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java > (right): > > ...
3 years, 7 months ago (2016-11-07 10:17:51 UTC) #4
Felix Dahlke
Looks good, one question. https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (right): https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java#newcode156 libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:156: return create(context, appInfo, basePath, elemhideEnabled, ...
3 years, 6 months ago (2016-11-18 06:56:55 UTC) #5
anton
On 2016/11/18 06:56:55, Felix Dahlke wrote: > Looks good, one question. > > https://codereview.adblockplus.org/29361556/diff/29361557/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java > ...
3 years, 6 months ago (2016-11-21 07:41:56 UTC) #6
Felix Dahlke
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-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java > ...
3 years, 6 months ago (2016-11-21 08:51:42 UTC) #7
anton
3 years, 6 months ago (2016-11-21 10:06:04 UTC) #8
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5