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

Temporary whitelist support for Doze #732

Merged
merged 12 commits into from Apr 21, 2019

Conversation

voidstarstar
Copy link
Contributor

This contains commits by @ccaapton @ale5000-git and myself.

Tested was done in a single user and multi-user setting. Further testing is welcome.

This will likely fix issues many users are having involving delayed push notifications. Some users might have even reported that their push notifications were not working at all when in fact Doze was really the issue.

This fixes #338.

@Vavun
Copy link
Contributor

Vavun commented Mar 18, 2019

Hey @Nanolx , could you please make a test build of your fork with this PR?

Any way, it's time to bump version to 15.1.80

@Nanolx
Copy link
Contributor

Nanolx commented Mar 18, 2019

I'll roll-out a new custom test build later today.

@Nanolx
Copy link
Contributor

Nanolx commented Mar 18, 2019

@Vavun uploaded custom microG build 0.2.6.15180-dirty-192 alongside with patched Play Store 14.0.33 to my F-Droid repo (this build of Play Store does not contain the anti-update patch as Google has changed around stuff and it's late here).

@voidstarstar
Copy link
Contributor Author

@Nanolx Thanks. Managed profiles/work profiles remain untested. Any feedback, especially if someone happens to use managed profiles, is appreciated.

@Nanolx
Copy link
Contributor

Nanolx commented Mar 21, 2019

@voidstarstar I'll leave a message over at XDA for people to test.

From my experience I've had some "problem apps" since some months now, which I had push notification issues with (namely XDA Labs, Bandcamp and Nintendo's Games (Fire Emblem: Heroes, Super Mario Run, Animal Crossing: Pocket Camp); though XDA Labs is the only one which push messages would be nice to have, I don't need games to ping me), I have not yet unininstalled and re-installed them, will report later (and also check the logs).

For the others I can confirm that push messages reliably appear in time when device is in Doze, as before there's been kind of delay for some.

@ale5000-git
Copy link
Member

@Nanolx: Uninstall isn't needed, you can use this code to force apps to refresh the GCM registration.

@chris42
Copy link

chris42 commented Mar 21, 2019

@ale5000-git Couldn't that be included in Microg itself? So many people ask on how to reset or mess it up after an upgrade?

@ale5000-git
Copy link
Member

@chris42
Well, this code delete a part of app data from all apps, it works fine from recovery but not from a normal app (microG is an app after all).

Also this is needed just one time.

@voidstarstar
Copy link
Contributor Author

Would it be possible for an app running as root to do that? It would be nice if this was done on first launch of microG or even manually via a GUI button within the microG settings.

@Nanolx
Copy link
Contributor

Nanolx commented Mar 21, 2019

An app running as root would technically be possible to do so, yes, but it's questionable whether something like this would be included.

@ale5000-git
Copy link
Member

ale5000-git commented Mar 21, 2019

If something like this is included, it may led antivirus apps to flag it as virus, if it is really needed I think a separate app is better.

@Nanolx
Copy link
Contributor

Nanolx commented Mar 21, 2019

I've added it to my nanodroid-util script, see https://gitlab.com/Nanolx/NanoDroid/commit/445ce81d066ea45e4303f327063704d1eb15b220

Providing it as a script is better I suppose.

@Vavun
Copy link
Contributor

Vavun commented Apr 3, 2019

I think this commit can be improved with this
https://developer.android.com/training/articles/direct-boot?hl=EN

@Vavun
Copy link
Contributor

Vavun commented Apr 8, 2019

Hello again. One user from russian 4pda community reported problems with push messages on nanolx microg fork starting 192 build and newer. (167 works fine)

the only difference in the way messages work is this pull request (I did not miss anything?)
And to be honest, it does not seem that the problem is in these changes.
But other changes are either not related to the push notifications at all or just cosmetic fixex.

Build 167, push notification tester run
https://www.youtube.com/watch?v=cfm7-0pyVKo

Build 192/227/241, push notification tester run
https://www.youtube.com/watch?v=J0fAcPiaykA

logcat_167.txt
logcat_241.txt

I tried to figure out something but I found nothing useful except
I/O error: java.net.SocketException: socket is closed

PS

@Nanolx it's time to enable issues on your fork page =)
It would be better if this issue was placed there.

@voidstarstar
Copy link
Contributor Author

voidstarstar commented Apr 8, 2019

@Vavun Are you able to reliably reproduce this?

Did you reinstall the Push Notification Tester app after installing the new version of GmsCore?

I see a SERVICE_NOT_AVAILABLE error. I think the issue is probably related to this.

I'm also wondering if the device was not registered yet. Did Google Cloud Messaging say it was connected before you opened the Push Notification Tester app?

Is the Google Cloud Messaging -> Advanced -> Confirm new apps setting enabled or disabled?

I could be wrong, but I don't think this is caused by this PR.

Edit: To clarify, I think this SERVICE_NOT_AVAILABLE error is being created here. I'm guessing either the device hasn't done a checkin yet or the "Confirm new apps" setting is enabled (which is a known issue).

@Nanolx
Copy link
Contributor

Nanolx commented Apr 8, 2019 via email

@Nanolx
Copy link
Contributor

Nanolx commented Apr 8, 2019

@Vavun I've enabled 'Issues' over at my fork, but as stated earlier: I don't experience any issue with 0.2.6.15180-dirty-241

@Vavun
Copy link
Contributor

Vavun commented Apr 8, 2019

Are you able to reliably reproduce this?

I can't reproduce this either. It works perfect for me with build 241

Thank you very much, I also can't reproduce this problem.
But that's wierd that he has problem with latest builds and has no problem with 167 build.

I'm also wondering if the device was not registered yet. Did Google Cloud Messaging say it was connected before you opened the Push Notification Tester app?

Is the Google Cloud Messaging -> Advanced -> Confirm new apps setting enabled or disabled?

GCM enabled and Confirm new apps disabled in both cases.

@voidstarstar @Nanolx I will give your responses to user with this issue. Thanks.

@julienschmidt
Copy link
Contributor

Ping @mar-v-in. This PR seems ready to merge, too.
If so, could we also have a new release? This, #624 and #635 should fix a lot of things for many users.

@voidstarstar
Copy link
Contributor Author

@mar-v-in Thanks for the feedback. I made changes based on the review above. Please let me know if I missed anything or if these should be squashed.

Copy link
Member

@mar-v-in mar-v-in left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small comment.

I'll do the squashing directly during the merge, don't worry about it.

@mar-v-in mar-v-in merged commit 6002388 into microg:master Apr 21, 2019
@ale5000-git
Copy link
Member

@voidstarstar
@mar-v-in

Looking up a class via reflection is, by magnitude, more expensive.

https://stackoverflow.com/a/435568/490687

This is why I have placed some code in onCreate(), the latest code do everything in handleAppMessage().
What do you think?

@voidstarstar
Copy link
Contributor Author

It's definitely much more expensive on the Oracle JVM and I can only assume it would be at least as much of a performance hit on Dalvik/ART.

I think calling Method.invoke() will still need to be done in handleAppMessage() in order to find the userId and to actually call the addPowerSaveTempWhitelistApp() method, but the rest can probably be done once in onCreate() by reusing the Method Objects.

@voidstarstar
Copy link
Contributor Author

I created a new commit voidstarstar@627d8ab that I think will move as much reflection as possible to onCreate(). There are 3 fields initialized in onCreate() using reflection and just 2 calls to invoke() in handleAppMessage().

If this looks good, should I open a new PR for this?

@mar-v-in
Copy link
Member

I don't think the performance penalty of using a bit more reflection is relevant here. But prefetching certainly doesn't hurt either.

Just open a new PR (as this one is merged already and cannot be reused) with a single commit with your changes.

@Tentos
Copy link

Tentos commented Dec 26, 2019

I have successfully installed microg with temporary whitelisting. Great work!

I would like to suggest two documentation changes on the installation wiki page:

  • It is necessary to install microg as a system app. This is quite obvious because Android does not grant the right for temporary whitelisting otherwise. (Still, I had to look at the logcat and see the SecurityExceptions to realize this.)
  • Additionally, I had to add an XML with the following content to /system/etc/permissions/ (named e.g. microg.xml):
<?xml version="1.0" encoding="utf-8"?>  
<permissions>
  <allow-in-power-save package="com.google.android.gms" />
</permissions>

The last bullet point was described by @ccaapton here.
When the XML was missing, I again got a SecurityException:

12-22 23:16:37.699 26505 26542 W GmsGcmMcsSvc: java.lang.reflect.InvocationTargetException
12-22 23:16:37.699 26505 26542 W GmsGcmMcsSvc: 	at java.lang.reflect.Method.invoke(Native Method)
...
12-22 23:16:37.699 26505 26542 W GmsGcmMcsSvc: Caused by: java.lang.SecurityException: Calling app u0a73 is not on whitelist
12-22 23:16:37.699 26505 26542 W GmsGcmMcsSvc: 	at android.os.Parcel.readException(Parcel.java:1692)

10073 is indeed the ID of my microg installation. I had manually disabled battery optimization for microg before, but it was necessary to whitelist it on a system level by the XML to not get the SecurityException, i.e., to get a working temporary whitelisting.

I am obviously open to do the necessary changes in the wiki, I just wanted to ask whether my suggestions are correct (and welcome :-) )

@drrossum
Copy link

drrossum commented Mar 17, 2021 via email

@HolisticDeveloper
Copy link

I'm not that familiar with Doze as I'm a neophyte when it comes to Android development. Would the changes in this PR have any impact for an app that is always running in the foreground on an always-on device (no battery)? We have a driver assistance application running on custom hardware, so we don't have to worry about the device "sleeping" or different apps being in focus.

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

Successfully merging this pull request may close these issues.

Put app into temporary whitelist when high priority gcm received [$5]