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

Travel back in time for failing patches #58

Open
oSumAtrIX opened this issue Jul 3, 2022 · 14 comments
Open

Travel back in time for failing patches #58

oSumAtrIX opened this issue Jul 3, 2022 · 14 comments
Assignees
Labels
Feature request Requesting a new feature that's not implemented yet

Comments

@oSumAtrIX
Copy link
Member

Problem

Currently, a failing patch can be destructive. This means if a patch fails it could cause problems on the patched application.

Solution

This can be solved by traveling back in time. This is possible since mutable instances of the modified classes are being created which can be discarded if the patch fails.

Feats

Resources are not being copied. This means any action on resources can still be destructive. For that, a temporal clone of the resource can be held in memory or in the cache for the duration of the patch. If it fails the patcher can revert the changes on the resources.

@oSumAtrIX oSumAtrIX added the Feature request Requesting a new feature that's not implemented yet label Jul 3, 2022
@Sculas
Copy link
Contributor

Sculas commented Jul 21, 2022

I have 2 methods of doing this:

  • snapshots.
  • keeping track of changes.

1 can be memory intensive but easy to develop, 2 is quite hard to develop and I don't know exactly how to pull that off right now.

@oSumAtrIX
Copy link
Member Author

Is this regarding the feat?

@Sculas
Copy link
Contributor

Sculas commented Jul 21, 2022

No, it was a response to the "Solution" paragraph/text.

@oSumAtrIX
Copy link
Member Author

In that case its as easy as it can get. All mutable classes pass are created in the proxy() method, which means you can store them in a list, if the patch succeeds, merge them as it currently is being done, or discard them, if the patch fails - for each patch.

@Sculas
Copy link
Contributor

Sculas commented Jul 21, 2022

This whole proposal is very memory expensive. Reminder, we're running on Android (on-device) here. I'm slowly starting to think this on-device goal was a bad idea x)

@TheJeterLP
Copy link

TheJeterLP commented Jul 21, 2022

This whole proposal is very memory expensive. Reminder, we're running on Android (on-device) here. I'm slowly starting to think this on-device goal was a bad idea x)

I agree on that. I'm not sure this feature would make sense on Android here. My phone has 12gb of ram so that wouldn't be a problem, but not many people have access to that.
But no reason to cancel the on-device goal. The manager is a must for revanced.

@oSumAtrIX
Copy link
Member Author

oSumAtrIX commented Jul 21, 2022

This whole proposal is very memory expensive. Reminder, we're running on Android (on-device) here. I'm slowly starting to think this on-device goal was a bad idea x)

The proposal at max does not add more than couple megabytes. So the patcher will only have to backup around lets say 100 classes at max, if they create that many that is.

@Sculas
Copy link
Contributor

Sculas commented Jul 21, 2022

The patcher doesn't know which classes will be changed. You have to make a snapshot of every class. Add another 500 MB or so.

@oSumAtrIX
Copy link
Member Author

No, the patcher knows which classes will be changed. This is due to the proxy() class I mentioned above. Each patch only modifies the classes it intendeds to. Hence the patcher knows which classes it needs to backup.

@oSumAtrIX
Copy link
Member Author

Fingerprints which do not access the mutable members also are considered in this process and will also work the same way. Keep in mind, we already have a cache for ALL proxied classes, I would even say this proposal does not add ANY additional memory.

@Sculas
Copy link
Contributor

Sculas commented Jul 21, 2022

I would even say this proposal does not add ANY additional memory.

This is not true, because we still need a snapshot of the class when it's proxied (multiple patches can access the same proxy, so you can't just "drop" the proxy). But feel free to PR the change and benchmark the impact of it.

@oSumAtrIX
Copy link
Member Author

No, the snapshot already exists. Its in the ProxyBackedClasses class, no additional copy is required -> no additional memory.

@oSumAtrIX
Copy link
Member Author

The only problem is the feat i mentioned above.

@Sculas
Copy link
Contributor

Sculas commented Jul 21, 2022

No, the snapshot already exists. Its in the ProxyBackedClasses class, no additional copy is required -> no additional memory.

Well, you'll find out yourself then x)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature request Requesting a new feature that's not implemented yet
Projects
None yet
Development

No branches or pull requests

3 participants