wsot / openpdroid_support

A stub to allow github issues to be lodged against OpenPDroid
5 stars 0 forks source link

Requests for cell tower location are always blocked #1

Open wsot opened 11 years ago

wsot commented 11 years ago

"Issue with pd2.0 1.52.

It is preventing Llama from getting my cell location: 08:55:24.146 PrivacyTelephonyRegistry( 347) package: com.kebab.Llama blocked for CellLocation

This prevents llama from triggering events because it does not know which area i am in. I have NOT changed any permissions in pd2.0 for the llama app, everything is allowed. Llama stopped working as soon as I installed pd2.0."

It is likely this problem also exists in openpdroid

oneacl commented 11 years ago

I can confirm Llama is broken with OpenPDroid. CollegeDev advised that he fixed the problem in PD2.0 but he never made the changes public.

CollegeDev commented 11 years ago

Yes, I'm new to github and now learning how to handle with it :+1: As far as I know a good solution how to sync my sources I will offer new fixes and many other things...

mateor commented 11 years ago

I am not a professional, but I have gotten relatively good with git. Anything you need to know you can ask me, through pm or whatever else. I am going to update those headers with your preferred name today.

We can merge anything you push upwards and thereby preserve your authorship as well. But no matter how it gets to github, we will find a way to preserve your credits for all of your contributions. Let me know if you would like any help.

oneacl commented 11 years ago

Any chance this will get pushed into the autopatcher? I'm currently using Llama with android location and it's heavily eating the battery life, previously (with cell towers) it wasn't using any battery at all. Thanks.

mateor commented 11 years ago

It will, we just need to rebuild. The plan is to keep the master branches of all the OpenPDroid repos, the OpenPDroidPatches and the Auto-Patcher 'latest' smali patches in sync.

So we will be pushing these changes out, and hopefully pretty soon. There have been some upstream changes which are putting a little pressure on us as well. It just takes some time to put together, as we have to build several times over for each romtype everytime we update the smali patches.

edit: and anything specific that the PDroid2.0 application might need can be worked at as well, just with the same limitations in time and the need for all the builds increasing in constant time if PDroid2.0 is not compatible with OpenPdroid. Hopefully we can get to just one core patch and obviate the need for double build requirements.

wesnm commented 11 years ago

We're talking about pushing patches, but I don't think this bug has actually been fixed. Has it?

mateor commented 11 years ago

This is wsot, right?

How about this- recent commits to cm, aokp and pa have broken our build patches as well as the auto-patcher, starting with the SMSDispatcher and continuing to spread to several other classes. The build patches probably no longer apply cleanly (haven't checked recently but if broken it should be the telephony patch) and the auyipatcher patches are 100% broken. They are only still working because of my trickery, but I am getting four-seven questions about this a day.

So we need to rebuild one way or another. I thought that we may as well include the fixes that are already done and counter known issues that generate regular bug reports. If we want to wait a little longer we can. If you have an idea about the SMS bugs or cell tower bug that could be a valid reason, although I wouldn't wait long. Soon my workarounds will fail too. I am also talking with CollegeDev about his sources so a merge with his latest contributions may be soon. But that might be something we do more meticulously, as an official major update as opposed to what I am talking about, which is a bug fix.

Welcome to chasing live development in the upstream branches. They are impossible to predict and break without remorse.

wsot commented 11 years ago

Sorry all; busy days. I haven't fixed (read: looked at yet) this issue, because I was working on all the exception handling code, which is now 'done' pending bugs. I'll get onto this one, and specifically test using Llama.

mateor commented 11 years ago

I like what you have done with the recent commits, makes a lot of sense.

We should think about (and I don't really know how) migrating these issues over to the OpenPDroid repo. Maybe just go ahead and add the stub to the organization after all. It won't be completely out of place when/if I ever add the autopatcher stub, as a way to keep you informed of what we have going on at ground level.

phillipberndt commented 11 years ago

On my phone, this bug has worse implications than just Llama not working. I find it also very hard to get a GPS lock when no WIFIs are near by, and I guess this is because the requests by com.google.android.location / com.google.android.apps.maps are also blocked.

Can you estimate when you will find the time to look into this? I am a complete novice in Android core development, but I've tried to do some research nonetheless. If you won't have time to fix this in the foreseeable feature, I will try to setup a build environment and try to fix this on my own.

Here's what I've got so far: Logcat reports

I/PrivacyGSMPhone( 6899): Package: com.android.phone asked for getCellLocation()
[..]
E/PrivacySettingsManager( 2674): getSettings - PrivacySettingsManagerService is null
I/PrivacyTelephonyRegistry( 2674): package: com.google.android.location blocked for CellLocation

As far as I understood the patch, the explaination for the above message must be that in the PrivacyTelephonyRegistry class, the pSetMan member is NULL. Which means that for some reason in the constructor ServiceManager.getService("privacy") returns NULL.

mateor commented 11 years ago

There is the possibility that this has been fixed but not merged into the main branch. I am working on that update as I can. If you want to be of some assistance, since you reliably have the issue, you could build and test. The jb-mr1-release-pdroid2.0 branch would be the best place to start, and I may even have a second commit to apply to that should it still exist after the test.

phillipberndt commented 11 years ago

I tried to do that, but it seems that integrating other repositories into Android mods is beyond my current abilities. Sorry!

What I managed to do is to confirm that my above speculation is correct: I applied the current 4.2 patches from OpenPDroid/OpenPDroidPatches to the cm10.1 trunk and then redacted the patch such that in PrivacyTelephonyRegistry the PrivacySettingsManager member pSetMan is initialized in isPackageAllowed rather than in the constructor. (See https://gist.github.com/phillipberndt/5339943) This fixed the bug for me - apps I've authorized have access to CellLocation, others don't. So the problem indeed seems to be that at the time the constructor is called, the privacy service is not available yet. I hope that this information is still of some use to you.

CollegeDev commented 11 years ago

Hey phillip,

your thoughts are good, but not right at all. The only reason why your code is working is because you're always check for every data access if the pSetMan is null or not and if it is null you're going to connect to the service again. But, it is a "fix" and I'm happy to see it :-). The check for

if (ServiceManager.getService("privacy") != null)

is unnecessary, because the service will be added in system server on boot just before other applications or whatever can access these methods. Furthermore, all privacy classes are preloaded too.

It is an general "android" and PDroid problem with the services. They deconnect (local instance of the service == null) after a while if they're idling or whatever (I've found some hints on the inline documentation from the framework-devs). Furthermore: I've implemented a general fix for that few month ago, but no one took it over to OPD btw (like a lot of other fixes I made)

mateor commented 11 years ago

Thanks for the gist phillip, and for the review CollegeDev, I agree that all contributions are welcome, even an unfinished fix is better than an untouched problem!

You can see my current progress on the merge at the openpdroid-pdroid2.0-merge branch. If you see above, my first request was for him to build and see if the bug was still present in the pdroid2,0 branch, mostly because I haven't had any testers report on that yet but I remembered you telling me it was resolved.

Send me a line when you have time, okay?

/mateor

phillipberndt commented 11 years ago

I'm glad to hear that a solution to this bug is known. Thank you guys! I will keep trying to compile a version from that branch and check back on you if I succeed. Still, out of academic interest:

CollegeDev, I believe that your explanation for why my patch is working is not correct. The service reference returned by getService is passed to a newly initiated PrivacySettingsManager and is stored in its service member. It is this locally initiated PrivacySettingsManager which is then stored in pSetMan, not the reference to the service. So my check if pSetMan is null ensures that the member is only initialized once, but that should be about it. Also, If in the version without my patch pSetMan was null, the line

PrivacySettings settings = pSetMan.getSettings(packageName, Process.myUid());

in PrivacyTelephonyRegistry.isPackageAllowed should raise a NullPointerException, but instead logcat shows the error message dropped by pSetMan.getSettings,

if (service != null) {
  return service.getSettings(packageName);
} else {
  Log.e(TAG, "getSettings - PrivacySettingsManagerService is null");
  return null;
}

About that check for getService returning null.. this could be interesting. On my phone, getService does actually return null most times when called in the constructor and only occasionally gets it right. (I tested that by Log.d'ing getService's output, but only with apps which likely start early like Llama and com.google.android.location.) If I get you right, this should not be happening?!

mateor commented 11 years ago

I am currently going through the entirety of the patches pretty thoroughly as I merge the two forks. Your find seems like something fairly straight forward to check. When I get to that section I will look for your suspicions and see.

Thanks for taking the time, not only for the original gist, but also to explain your thinking. I will report back here later this week.

CollegeDev commented 11 years ago

Hell yeah, I should have gone to bed instead of writing thi comment... I'm sorry. I will explain you what I mean: In this case the pSetMan is a private globale class variable and only init one time. The constructor of the pSetMan is following:

 PrivacySettingsManager(Context context, IPrivacySettingsMangerService service)

Inside the pSetMan we hold an reference to the service with an private globale class variable of IPrivacySettingsManagerService. The problem of all that is (based on experience in ICS) that the reference get lost after a while (thats the pretty logcat message getSettings - PrivacySettingsManagerService is null). For this "problem" I implemented a central reconnector which fixes this bug in a more general way. As I can read in some inline comments inside the framework classses, the framework devs sometimes have the same problem.

In this case, your "bugfix" should work some hours, but after a while it doesn't work anymore until you will restart your phone. If you have time, can you test if I'm right or if android changed something to that in JB?

phillipberndt commented 11 years ago

Sorry it took me a while, but I wanted to wait a few days to be sure. My SGS2 has been up for 4 days now with my patch in place and the bug did not return: Llama still reacts on cell location changes and also freshly started apps can still get the CellLocation. I can't tell if this is due to a change in JB or solely due to my patch though.

I wonder if those are actually two different bugs? Anyway, even if they are, from the sounds of it your solution will likely resolve the problem I described above as well.

mateor commented 11 years ago

Thanks phillip. I am going to give your fix a full test myself this week.

kimboinatl commented 11 years ago

I am also noticing this problem with Tasker (AOKP MR1 build 6 on my VZW Gnex). I did a logcat and I am seeing the following:

04-18 10:50:35.794 V/PDroidAlternative(2224): NotificationHandler: Notification for: com.google.android.location:locationNetwork:0 04-18 10:50:36.223 V/PDroidAlternative(2224): NotificationHandler: Notification for: net.dinglisch.android.taskerm:locationNetwork:0 04-18 10:50:41.270 I/PrivacyCDMALTEPhone(638): Package: com.android.phone asked for getCellLocation() 04-18 10:50:41.270 I/PrivacyTelephonyRegistry(381): package: com.google.android.location blocked for CellLocation

I granted all permissions to both Tasker and Phone.

I guess at this point y'all know about the problem, but I wanted to add this in case it helps at all. I've just started playing around with PDroid so I'm not too familiar with its inner workings, but let me know if there is anything I can test out or do to help.

mateor commented 11 years ago

I think that this issue has exposed up to three bugs, all of which have fixes in the pipeline. We are working on the update now and are appreciative of anyone who takes the time to file a bug report, especially one with logs.

I will try and remember to post to this issue when I push the update so that you guys can test the fix if able.

kimboinatl commented 11 years ago

Good deal, I'd be happy to help test.

uniquePWD commented 11 years ago

Any news on getting this bug fixed? It also causes Foursquare to crash.

mateor commented 11 years ago

the fix is in scattered places around the project...it has not yet been merged into even the -devel branches yet. I would expect the upcoming release to have a fix for this issue.

uniquePWD commented 11 years ago

Awesome, thanks for the update. Is there a workaround that could be done on the end-user end in the meantime? I understand that new versions don't roll out overnight and it would be cool if there was less pressure on you to fix this bug and enable you to have as much time as you require to get the next version ready.

phillipberndt commented 11 years ago

mateor, towards that workaround: Is PrivacyTelephonyRegistry.smali the same for all mods? (I believe it should, because the class is introduced by the openpdroid patch, but I might misunderstand the inner workings of Dalvik and/or auto_patcher)

If it is, I could use your diff_tools to create an auto_patcher patch to be applied after the openpdroid patch (i.e. I'd diff a mod with openpdroid against the same mod with openpdrod + my patch) which would work for all users, regardless of their mod (version), only depending on them using the same set of openpdroid patches.

Am I right? I'd provide that patch here..

mateor commented 11 years ago

Yes it is the same.

I have been looking at that fix and debating this for awhile. My main reluctance to simply shipping it was twofold...CollegeDev's reluctance and then the fact that I wanted any updates to be making progress on supporting PDroid2.0 as well as PDroid Manager. But even though I am getting close to finishing the merge, upon second thought those reasons don't really hold water.

Let's add it...the autopatcher can do this pretty easily I expect. I was about to suggest exactly what you outlined before I went back and read your exact proposal. I will just add that small diff to every shipment.

Thanks for suggesting this, I think it is the right thing to do and probably should have been done earlier.

Also, if anyone is interested, the merge's staging area is here

phillipberndt commented 11 years ago

sabret00the, if you'd like to test: Have a look at my fork of auto_patcher. (If you haven't used the git version of auto_patcher before note that you'll have to run ./batch.sh and use the auto_patcher from within the archive created by that script.)

I have so far tested the patch with today's nightly of cyanogenmod on my i9100 (sgs 2). The best way to see if it's working IMHO is Llama's history panel: If the patch works, it should show cell numbers. If it did not, you'll see 1-2 correct cell id's and then lots of -1 : -1 : -1 there)

mateor, here's the smali patch: https://github.com/phillipberndt/auto-patcher/commit/db33df82065ea87a6a790215fd1499fb827ad957. Worked like a charm. I did not find a mechanism for adding a second patch against the same jar file, so I added one in https://github.com/phillipberndt/auto-patcher/commit/89dc51b6ac4da9079020ef9989352a7a5fceaba2. I also fixed what I think were two typos in the diff_tools.

mateor commented 11 years ago

Thanks a lot...I will not be able to look at this until tonight, but I will put some time in then.

I have been planning on adding an "incremental" patch set/support to the autopatcher and will probably do so with your patches. When the next OpenPdroid version goes out, I anticipate several waves of bug fixes and updates...hopefully I can use that to avoid rebuilding for every romtype. Your bugfix patch is a good opportunity to soft launch that. I will pull your changes over to a test branch and see what works the best in the short/long term.

Oh, and yeah, the diff_tools are simply not quite up to snuff. The are remnants from ICS, and are out of date in several ways. I spent maybe 15 mins trying to bring them forward for some collaborators, but the collaborators moved on and so i never finished. They are a valuable idea and tool, I appreciate the time you spent looking at them and this patch in general. I will respond again when I have given it a look.

uniquePWD commented 11 years ago

@phillipberndt there was no AutoPatcher/APG.exe created after running the batch.sh from your branch

phillipberndt commented 11 years ago

@sabret00the AFAIK, APG is a frontend to auto_patcher and not part of it. Its XDA thread mentions that it's able to use the latest git version of auto_patcher though. Since mateor has already merged my changes into his repo, simply rebuilding might work?!

If you want to try using auto_patcher without APG, try to run the auto_patcher script (within the zip file created by batch.sh) through bash, for example as

bash ./auto_patcher cm-daily.zip openpdroid cm
lukash-master commented 11 years ago

I've tried to apply patch via APG to my Motorola Milestone 2 CM10.1 rom, but experienced some problems. Please find log here: http://codepad.org/Eq2OdcRJ Could you please share some information if I am doing anything wrong and what to do to apply this patch to my rom?

JayXon commented 11 years ago

I tried autopatcher 2.9.7 with pa_maguro-3.56-28MAY2013-135145.zip, but still got fc in foursquare.

mateor commented 11 years ago

Thanks for the report...I have had the foursquare problems reported. It looks like the GPS problems are maybe half fixed. I have another set of GPS fixes in the queue, I hope to get them out in the auto-patcher soon.

lukash-master commented 11 years ago

Any news regarding the cell tower location issue? I would love to upgrade my CM rom to the newest version (which includes PDroid) but the cell tower location is crucial for me :).

phillipberndt commented 11 years ago

With one of the latest versions (I did the last update around end of july, so I can't tell exactly) the cell tower problem resurfaced on my SGS2 for cm-10.1.3-rc2. From a brief look into auto_patcher I think the problem is that while you define $INCREMENTAL, you then do not ever actually use it to apply the patches. Instead, there only is a work-in-progress comment:

# Incremental--use patch_matcher? (how, b/c dry-run...should INCREMENTAL be optional or fail-possible?

I added the incremental patching by copying the Actual application of patches code above the comment and replacing $PATCH_QUEUE by $INCREMENTAL. After building a new patch and installing it onto my phone, the problems were gone again.

One interesting observation is that when I created my patch half a year ago, Llama actually noticed cell changes, but got -1:-1:-1 as the cell id instead of the correct one. With current builds, it does not show those ids in its log anymore. I can't tell whether this is due to a change in Llamas front end or due to changes in Openpdroid though..

(On another note, in the current git version of auto_patcher you execute git log HEAD^1..HEAD >> "$LOG" after everything is finished and abort when this command fails. It actually does fail for shallow repositories cloned with --depth 1, because there is no HEAD^1 there...)

Edit: See https://github.com/phillipberndt/auto-patcher/commit/eb841e8ea0f22d350a2514b8081f857e67e21df6 and https://github.com/phillipberndt/auto-patcher/commit/a190aba880e42d32b091c15844e3cd7411da579c

mateor commented 11 years ago

Thanks for the debugging...there is a lot about the new implementation that is basically temporary...figuring out how to fit it all together took most of the time. I have cherry-picked both your contributions, although as my comment you quoted above indicates, I haven't decided on a long-term strategy with the Incremental fixes. Thanks.

oneacl commented 10 years ago

It seems that Tasker is also affected (no cellid is being received) and also the networklocation package from NOGAPPS (http://forum.xda-developers.com/showthread.php?t=1715375). With wi-fi disabled the network location fails, when I enable wi-fi I get a location in a few seconds based on the wi-fi hotspots nearby.

Thanks.

flimflammachine commented 10 years ago

The lack of location by cell tower is a real problem for me. Is there anyway a noob like me can patch my ROM with one of these experimental patches using the autopatcher?

Massive credit to everybody involved in this project, but this has been going on for ages now, does anybody know when it will be fixed (even using a workaround)?

If I can help using a patch , then please point me in the right direction.

Ta.

mateor commented 10 years ago

I don't own a device that can test this feature, so the fix will not be coming from me. Pull requests are welcome.

oneacl commented 10 years ago

There seems to be a hit and miss in getting this to work by forcing phone.apk and your application by trusted. I've managed to get it to work on my phone (mako running cm10.2) but I had issues in the past with this method. Others have also reported various success by forcing phone.apk to be trusted (as opposed to "no settings").

Mateor - I thought this applies to all phones/roms?

Ajex commented 10 years ago

I have tryed last version of patch, problem is still alive ((( foursquare isn't working (crashing) after patching (cm 10.2 jb 4.3.1). Phone - SII 9100