Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validator.registerAdapter() should not be static (or leaks are easy to follow) #97

Closed
dimsuz opened this issue Jul 24, 2015 · 15 comments
Closed
Assignees

Comments

@dimsuz
Copy link
Contributor

dimsuz commented Jul 24, 2015

I beleive that it would be better to make Validator.registerAdapter() non-static method.

I just had whole Activity context leaked because of this method is static.
If you do something like this in your activity:

void onCreate() {
  Validator.registerAdapter(MyCustomView.class, new ViewDataAdapter<MyCustomView, String>() {
    public String getData(MyCustomView view) {
       return view.getCustomText();
    }
   validator = new Validator();
}

then you automatically leak your Context, because it gets captured by anonymous class.
And using anonymous classes for this kind of things is very common.
So using static here makes it very easy to introduce a leak.

Besides by looking at the sources I don't see why this method needs to be global.
Validator class seems to be designed to be used in local scope, i.e. inside an activity lifetime, so it seems that registerAdapter should live in that scope too, i.e. be non static.

@ragunathjawahar ragunathjawahar self-assigned this Jul 24, 2015
@ragunathjawahar
Copy link
Owner

You're right. Feel free to send a PR.

@dimsuz
Copy link
Contributor Author

dimsuz commented Jul 24, 2015

Ok!

@ragunathjawahar
Copy link
Owner

Fixed by 350a42c

@ragunathjawahar
Copy link
Owner

@dimsuz Thank you 👍

@dimsuz
Copy link
Contributor Author

dimsuz commented Jul 28, 2015

you are welcome :)
do you plan to upload a new snapshot to sonatype in near future?

@ragunathjawahar
Copy link
Owner

Thanks for reminding. Just uploaded a new snapshot.

@dimsuz
Copy link
Contributor Author

dimsuz commented Jul 28, 2015

Great, thank you!

@dimsuz
Copy link
Contributor Author

dimsuz commented Jul 29, 2015

@ragunathjawahar somehow the current snapshot became .aar instead of .jar which it was in previous versions. Is this intentional or some issue has led to this? I unpacked .aar and its res/ folder is empty, so seems like a build misconfiguration?..

See https://oss.sonatype.org/#nexus-search;quick~saripaar

@ragunathjawahar
Copy link
Owner

@dimsuz I have to update my maven publishing script, both .aars and .jars are being uploaded. Did you mention @aar in your dependency?

@dimsuz
Copy link
Contributor Author

dimsuz commented Jul 29, 2015

No, I tried to download directly from the above link. Currently I have android-saripaar.jar placed directly in my libs/ folder (due to various reasons) - that's why I noticed that it changed from .jar to .aar...

@ragunathjawahar
Copy link
Owner

capture

@dimsuz
Copy link
Contributor Author

dimsuz commented Jul 29, 2015

oh, thank you. didn't know about this!

@dimsuz
Copy link
Contributor Author

dimsuz commented Jul 29, 2015

One more thing: somehow the change which was fixed by this issue is not present in the snapshot - Validator.registerAdapter is still static. Not sure why. I double checked by decompiling a Validator.class from the above .jar and it's indeed still static :)

@ragunathjawahar
Copy link
Owner

😃 I didn't pull before I uploaded the archives. Can you check now?

@dimsuz
Copy link
Contributor Author

dimsuz commented Jul 29, 2015

Everything is perfect now :) thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants