Merging PD2.0 into OPD

[CollegeDev]

Hey guys,

I would like to start the work on merging PD2.0 features into OPD.
Before I will describe some things:

PD2.0 is completely ported by my self and not a fork of OPD

Therefore I think it is a good way to review and compare the whole code and merging the best solutions together. Who would like to help??!

I haven't had a "deep" look at OPD yet, but that is what I can say now:

• Port contains several "normal" and "heavy" porting failures, just like the one in the TelephonyRegistry (I believe it was the cellInfo method)
• We can fix all known bugs with the review, because I can tell you what to do and how to fix or which code you have to copy
• purgeSettings method was never intended to delete the whole database. Please insert a new method to the interface just like "deleteDatabase" and change the code!
• Until now the database is never being closed the whole time (OPD), don't do that! You can fix it with AtomicInteger by increment/decrement for every dataccess and after that automatically close it
• The delete recursively method is more than deprecated and have to change
• ..... I haven't enough time to write down my mind about the framework :-D, I will update this post from time to time.

Hopefully there is someone out there who would like to start with merging.
Furthermore: I will talk with matoe about how to handle it on github.

Cheers

[wsot]

I've been looking over some of the changes in PDroid 1.54 (in the context of the comments above).
I'll break this up into three parts. First feedback on code, second some answering of the above questions (and that will partially be covered in feedback), and third discussion about merging. Note that all of my comments regarding the OpenPDroid code will relate to the 'devel' code, not the release code.

First, with the PrivacyPersistenceAdapter: You and I have taken a somewhat different approach as to where an error is handled, and indeed how it is handled. I have taken the approach of passing Exceptions up and then handling them in the original caller, whereas you have trapped the error and returned an object based on that in the PrivacyPersistenceAdapter itself. i.e. if an SQLiteException happens when doing a 'getSettings' in OpenPDroid, then an exception is thrown (and indeed throw upward by the service, out to the PrivacySettingsManager). The implication is that in extending the current OpenPDroid code, the 'default' action would tend to be handled in the PrivacySettingsManager object or even the object calling it, rather than in the PrivacySettingsManager. The use of a 'flag' package name in the returned settings is PDroid 2.0 1.54 is good in terms of allowing some identification of the cause - whether or not it is a 'valid' response or not.
There are a couple of questions here: first, should the error be handled this early? (I don't think it should; this is what Exception handling is for). Because in the end I would like the 'action on error' to be configurable, I think the Exception-to-PrivacySettings conversion needs to be done pre-service: in the PrivacySettingsManager object. I am also wondering if the 'failover' (and indeed 'default') objects should be a subclass of the PrivacySettings object. Either that, or there should be an additional flag in the PrivacySettings object to identify which type it is: user configured, or autogenerated (although you've somewhat done that with the 'isDefaultDenyObject' and 'isFailSafeObject').
One quick note: the unlocks for the ReentrantReadWriteLock should always be in 'finally' statements - otherwise you might get a long-lasting lock through some unexpected occurrence (e.g. in getSettings, I think it would be better to put a big try/finally block around absolutely everything after the 'mLock.readLock().lock();' right to the end of the function, with a finally to unlock, rather than doing an unlock before the return statement. Not everyone would agree, and in this specific case the only risk you have is if the c.close() near the end of the function throws an exception).
We need to think about whether we actually want to use the 'allowedContacts'. If not, we should scrap it, since it adds an extra database call for each settings retrieval, deletion, etc.
I like the 'Watchdog'. I haven't looked over the implementation in detail, but it makes sense.

In terms of the caching you and I have taken quite different approaches. I've used the Google-provided LRUCache (basically just because they recommend that, and I was aiming for a small cache including the most-used items, rather than caching everything as it is used). Both have their pros and cons. Originally, I wanted to use a 'cache-everything-until-memory-becomes-a-problem' using SoftReferences, but apparently Android doesn't hold on to SoftReferences at all - it just scraps them, so that wouldn't work as a cache. Using an LRUCache was basically my way of being conservative with holding stuff in memory. I'm not sure one method is especially better than another here. Using the LRUCache means the hashmap is smaller so lookups are faster (although there is the overhead of tracking the LRU), but if there is a lot of switching between more apps than the size of the cache then there are more database calls. I suspect the performance difference between a reasonably-sized (e.g. 20-30 item) LRUCache and your custom hashmap-based implementation is marginal at best.
Relating to that, there is a good reason why the database is never being closed: opening the database carries a (large) perfomance hit. The entirety of the performance problems with the GPS, Compass, Camera etc in OpenPDroid 1.0.0 was due to the database being closed when there were no threads using it. Given the size of the overhead of opening the database, I'm not sure whether it is worth closing it when it is idle. It didn't close in the original PDroid (although it was supposed to).

I like the PrivacyDebugger. It is nice to have the level of debugging switchable, rather than what I've currently got in OpenPDroid which is some compiled-in flags for level of debug logging. I'm not exactly sure why you are using that thread in the constructor to get the debugger state.

With the ResolveHelper: It may be possible to get the specific package by PID rather than UID (where you have the problem of multiple packages with the same UID). I have some code I was testing about somewhere and I'll try to track it down. Mind you, it would be ideal to implement both so it falls back from one to the other.

Likewise, the PrivacyConstants is excellent. I would also quite like to have validation functions that management apps could check against.

With the PrivacySettingsManager object, one thing we need to be very wary of is the IPrivacySettingsManager interface. Having the 'service' variable as an IPrivacySettingsManager means reflection can be used to inject a custom subclass in there rather than a valid service. I've tried to address that in OpenPDroid (devel) by actually using getClass to check the class against known-good class-names. If an invalid class name is detected, then the 'service' object is dropped and reconnection is attempted.
Again, going back to exception handling - I prefer errors in the service to be passed up via exceptions all the way to the PrivacySettingsManager class, and then to convert them to the 'deny-on-error' or 'allow-on-error' objects at that point. That way, all of the generation of error-triggered objects is handled in the one place, whether it is because there is an error in the database or because the service can't be reached.

And there is more, but that's all I'm going to dig through tonight.
In terms of merging, there will be several parts to it. There are things which are done in both OpenPDroid devel and PDroid 2.0 1.54, but in slightly different ways. We need to decide which way to run with.
With the 'deny/allow on error', if we want to implement user configurability for that (and I would like to do so) then we need to decide how/where the configuration will be stored. This is an interesting question, because for an app running as root this could be a very attractive target.
There are also some new things we need to look at. For example, is the user's own contact record protected? I haven't checked. Is the move from 'Contact' to 'ContactsContract' something we need to be paying attention to?

There is plenty to be done.

[mateor]

Okay, I have (perhaps) settled on a working format. In order to not demolish wsot's or CollegeDev's work (since this is still the only place either of their current work is hosted or available), I have started yet another merge branch. If things continue like they are now, once the merge is done I intend to remove 80% of our branches.

Anyway, the new working merge branches are going to hopefully be uniform and be called openpdroid-pdroid2-merge. I am going to try and actually add features in a sane manner, something that has eluded us since the beginning of the project. That may end up being ambitious, especially because I am likely to go file by file and miss some things. But I will attempt to rebase some of the inevitable mistakes before I merge the opendpdroid-pdroid2-merge branch into the -devel branch.

See the ContextImpl merge for an example, and as always, anyone who wants to jump in can do so or contact me for the ability to do so.