libxposed API 102 support: hot reload, detach and atomic hook replacement#757
libxposed API 102 support: hot reload, detach and atomic hook replacement#757HSSkyBoy wants to merge 13 commits into
Conversation
|
重新提交的 #743 |
|
API 102 contains far more changes than just module hot reload. |
|
大概还要多久正式构建？ |
你在這問也沒用啊，看JingMatrix什麼時候處理該PR |
|
Would this apply to all modules or just ones that make use of this API? Sorry, I'm kind of a newbie. I'd find this really useful if so as I'm trying to write my own Xposed module but it's hard having to reboot every time |
Thanks for the work on this — the hot reload machinery genuinely works, and I want to say that up front because most of what follows is criticism. I merged this onto current master (clean), built it, flashed it to a Pixel 6 on Android 17, and drove it with a purpose-built API 102 module across five generations plus a separate hook target app. I then went through the libxposed 102.0.0 javadoc and AIDL against the implementation. Everything below quotes the specification verbatim; where the spec is silent I say so rather than dressing a design opinion up as a conformance problem. The harness is on the The vendored What conformsVerified on device:
Contradicts the documentation1. system_server is hooked by the module but never becomes a hot reload target.
With the test module scoped to and 2. Hot reload of a frozen process is reported as a module refusal.
Freezing the target's cgroup and requesting a reload gives This is the common case, not a corner: a module's targets are usually cached background processes, and Android freezes those. The The same contract is broken more generally: 3.
4.
5.
6. A module-thrown
7. Hook replacement is documented atomic and is not.
and 8. In-flight calls are supposed to be snapshot-based. Same javadoc:
That skip path has a second defect: Documented but not implemented9.
and 10. The 102 behaviour change for legacy APIs is not enforced.
11. The Not covered by the specificationDesign and security concerns. The docs say nothing either way, so treat these as my opinion rather than as breaches. 12. 13. 14. 15. Old code can still register hooks during 16. The daemon's 17. Reporting 18. 19. Pre-existing, not this PRFound while checking the chain against the spec. Worth separate issues rather than scope creep here.
Nits
RecommendationTo be direct rather than leave it hanging: this PR will most likely be closed. I intend to do the API 102 and hot reload work myself later, and I will work from the checklist above. The reason is that the branch reads as mostly AI-generated, and this particular feature does not survive that level of attention. The review above is largely evidence of it:
Bundling an unrelated privilege change on top of that settles it — the full read-write I have flagged at the top that this review is machine-assisted as well, so to be fair about the distinction: the difference I care about is not the tool, it is that every claim above was checked against the specification text and then run on a device. Thanks for raising the topic regardless. API 102 hot reload is worth having, and I would not have prioritised it this soon otherwise. Please leave the branch up as a reference. |
Bumps both libxposed submodules to 102.0.0 and implements the members the new API makes abstract. attachFramework() now takes the detach implementation, so detach() is wired to VectorLifecycleManager. activeModules is the framework's only strong reference to entry instances, which is what the javadoc requires detach() to drop; any dispatch added later must iterate it rather than keep an entry list of its own. HookHandle.replaceHook() and HookBuilder.setId() are implemented without touching the native layer. Installing the new record before uninstalling the old one would leave both in the callback multimap, so a call that snapshots in that window would run both hookers. Instead the record stays registered and its hooker is swapped through a volatile write: the native side indexes records by object identity and keeps priority in the map key, and replaceHook is specified to preserve executable, priority, exception mode and id, so nothing native-visible changes. Handle validity is tracked with a per-record epoch, and same-id intercept() reuses the installed record through a per-(module, executable, id) registry. Because the hooker is now mutable, VectorChain freezes the hooker list once when the root chain is built instead of reading it per node, so a mid-call replacement cannot leak into an in-flight call as the chain is documented to be snapshot based. libxposed-annotation is consumed as the published Maven artifact, which is what both submodules' own build files use, rather than being reimplemented in-tree.
The id path read the registry and then installed, so two threads racing on one id both saw no entry and both installed a native record. That is the duplicate-record window the in-place replacement design exists to avoid, and the later registry write also orphaned the first record, leaving it hooked with no way to reach it. Claiming the key with putIfAbsent makes the winner the only installer; everyone else takes the volatile-write path.
XposedInterface.API_102 states the behaviour change plainly: "Libxposed modules can not call legacy de.robv.android.xposed APIs." targetApiVersion only ever selected MODERN over LEGACY loading, so nothing stopped a module declaring 102 from using the legacy bridge. The module classloader is where this has to be enforced. Its parent is the framework's own loader, which carries de.robv, so refusing to resolve that package covers reflective lookups against the loader as well as direct references. targetApiVersion is carried on PreLoadedApk to reach it. module.prop parsing follows JingMatrix#794: Java Properties instead of a hand-rolled split("="), tolerating a malformed file rather than making the APK unloadable, and leading-digit integers matching the manager's extractIntPart. That PR is still open and owns this parsing plus an exceptionPassthrough field on the same parcelable, so expect both to conflict here and resolve in its favour. Note that :daemon does not compile yet: bumping libxposed to 102 added getRunningTargets() and hotReloadModule() to IXposedService, which the hot reload work still has to implement.
IXposedService gained getRunningTargets() and hotReloadModule() in API 102. Targets are built from the heartbeat registry ApplicationService already keeps, not from a registration call made by the injected process. That ordering matters: in system_server the module is loaded long before the daemon's config cache is populated, so a module-initiated registration cannot be answered and the process would silently never become a target - the one process where not rebooting is worth the most. getRunningTargets() reports every hooked process, including modules with zero or several Java entry classes, because the AIDL documents it as running processes hooked by this module rather than hot-reloadable ones. Those targets answer UNSUPPORTED instead of being hidden. hotReloadModule() raises SecurityException only for the two conditions the AIDL reserves it for, an unknown target id or one belonging to another module, so a SecurityException arriving from module code can still be reported as a result. Claiming a target is a compareAndSet rather than a read followed by a write, since reloads are serialized per target. The reload itself is not implemented yet and is reported as UNSUPPORTED, which package-info permits when the framework cannot provide a valid new module generation. Reporting SUCCEEDED would be untrue and FAILED would tell the module app that its own code refused. Module carries versionCode explicitly: ApplicationInfo.longVersionCode is hidden, and the system_server path builds its modules before PMS exists, so it reads the version through PackageParser to avoid reporting 0 there.
The daemon reaches the hooked process through a new IHotReloadTarget, which each process registers while the framework bootstraps rather than when its modules load. Registration therefore does not depend on the daemon's module cache, which is what previously kept system_server from ever becoming a target. Ordering follows the callback contract. Old code is frozen before onHotReloading runs, not after, so hook registrations attempted from inside the callback fail while the unhook and replace paths the same javadoc tells modules to use keep working; the freeze gates the single registration entry point and keys on the generation rather than on the hooker's classloader. The old hook handle list is captured after that. The new generation becomes the committed one only after onHotReloaded returns. If it throws, the hooks the new generation installed are removed, the old entries are made live again and the old generation stays committed. Committing first would make that rollback unreachable, which is what the previous attempt did while advertising it. Result encoding keeps the two failures distinguishable, since a null message is reserved for onHotReloading returning false: every other failure carries a diagnostic, synthesized from the throwable's class when it has no message of its own. A process whose entries have all detached reports UNSUPPORTED rather than borrowing the refusal encoding for a decision no module code made. A frozen target is thawed for the transaction, because a cached process's binder is alive but unreachable and the failure would otherwise be indistinguishable from a refusal. detach() is honoured for both callbacks: dispatch resolves entries through VectorLifecycleManager.activeModules, so a detached entry is skipped and the framework holds no entry list of its own. The old generation stays strongly reachable only for the duration of the cycle. Hot reload also drops the module's hook id registrations, which strongly reference hook records and through them the retired classloader. Not verified on a device yet. The frozen-target path, the freeze, and the rollback are the parts that only surface at runtime.
HSSkyBoy
changed the title
HSSkyBoy
changed the title
Measured on device: getOldHookHandles() returned two handles on the first reload of a process and none on every reload after it, while the hooks themselves stayed installed and serving. The reload path dropped the module's whole tracking set once the old handles had been captured. That is wrong for the replacement design: replaceHook swaps the hooker inside a record that is already installed, so forgetting the record leaves a live hook the framework can no longer hand back. The damage is worse than a missing list, because the default onHotReloaded unhooks exactly the handles it is given - an empty list means the retired hookers, and the classloader behind them, are never released. Tracking and the id registry now survive a reload, keyed by package name as they already were, and the rollback undoes only what the incoming generation added on top of the set it inherited. Verified on device: four consecutive reloads now report two old handles each.
ILSPApplicationService leaves transaction ids implicit, so a method added at the top renumbers every method below it. Everything ships in one zip, so it worked, but it is the mistake the interface's own layout invites and the reason upstream pins ids explicitly elsewhere.
The module list only used minVersion and targetVersion to raise a warning when they were out of range, so the API a module was built against - which decides how the framework loads it - was never visible next to the module itself. It now sits on the version line, as a range when min and target differ, and is omitted entirely when a module declares no metadata rather than rendering "API 0". The CLI reported the framework version but had no way to show the API version the manager already reads over binder. zh-rTW was missing menu_select, menu_select_all, menu_select_none and menu_auto_include. That file is localised for Taiwan rather than converted from zh-rCN - it uses 程式, 儲存 and 停用 - so the additions follow the same conventions instead of copying zh-rHK.
Three gaps around the state a module app polls for. HookedProcess.TARGET_STATE_STALE was never reported. A target's state only moved when a reload finished, so a process running an older generation kept answering UP_TO_DATE and the documented "notice STALE, then request a reload" flow had nothing to notice. The reported state now compares the generation a target loaded against the installed one; RELOADING and FAILED still describe the last attempt and outrank it. A successful reload moves the target onto the new version, which is what clears STALE. autoHotReload was documented in package-info as an API 102 module.prop key and appeared nowhere in the tree, leaving half the documented trigger set missing. It is now read alongside minApiVersion - which package-info lists as required and the daemon also ignored - and an app update reloads that module's stale targets. The module keeps the final say: this drives the same cycle as a service request, so returning false from onHotReloading still cancels it. The trigger runs after the cache swap, so the generation targets are asked to load is the one that was just installed. Targets were only forgotten when their process died, so disabling or uninstalling a module left its targets listed.
Measured: system_server's target reported loadedVersionCode=0 and, once targets started comparing versions, therefore reported STALE permanently - no reload could ever satisfy it. That path builds its modules before PMS exists, so it parses the APK directly, and PackageParser.Package does not expose a usable version on current releases. ApplicationInfo carries one but the field is hidden, so it is read reflectively rather than by adding a stub that would shadow the real class for the whole daemon. A zero is now treated as an unknown generation rather than an old one, in both the reported state and the auto-reload selection, so a device where the lookup fails degrades to "no opinion" instead of a target stuck at STALE.
The previous attempt read the version off the ApplicationInfo that PackageParser produces. Measured on device: it stays zero, because a raw parse does not populate it, so system_server's target still had no comparable generation. It is taken from the first cache update that knows the version instead. Nothing can update a module between system_server loading it and that point without also rebuilding the process, so the value is the one that target is actually running. Zero keeps meaning "unknown" rather than "old", which is what stops a target that never gets a version from reporting STALE forever - but it no longer stays unknown, so system_server is now visible to stale reporting and to autoHotReload like any other target.
Rationale that belongs in commit messages had been left in the source as KDoc blocks on private helpers and fields, which buries the code it describes. Kept only the lines that explain an ordering or a constraint that is not visible from the code itself.
|
The branch has been replaced with the rewrite. It is 11 commits on top of current Since the objection to the first attempt was that its claims had not been checked against a running device, everything below was measured. Two caveats about the measurements, stated up front:
Measured against the review
Two more, not in the review but in the same area: What running it found that reading the diff did notYour point about this stands, so it is worth being specific about where it applied here.
The same applied to the cgroup layout. This device exposes Not verified
Left alone
I am not asking for this to be merged as-is; the reasonable next step is whichever commits you want in whatever order suits the work you had planned. Splitting them so each stands alone was the point. |
Consuming io.github.libxposed:annotation as compileOnly is right - the annotations are metadata and nothing reads them at runtime - but it leaves the vendored API and service sources referencing classes that are not packaged, and R8 fails the release build over it. Release was never built locally while this was developed, so the failure only showed up in CI. The rule rides on xposed's consumer rules for :zygisk, and is repeated in :daemon, which minifies the service sources through a module that publishes no consumer rules of its own.
4 participants
Status
This PR is being redone from scratch rather than patched, in response to the review.
To be unambiguous about what is on this branch right now: the diff shown above is still the original attempt. The rewrite is on a separate branch and is not finished. It will replace this branch only once it has been built and run on a device against the
api102-harnesscontract, not before.Why a rewrite instead of fixes
The review checked the branch against the libxposed
102.0.0javadoc and AIDL and measured it on a real device. Several findings were structural rather than local, and patching them individually would not have converged on the same code as building it correctly:registerHotReloadTargetwas initiated by the injected process and raced the daemon's module cache. In system_server the modules load long before that cache exists, so the registration always threw and was swallowed — system_server could never become a hot reload target at all.newStateCommittedwas set beforeonHotReloadedran, so the rollback this PR's original description promised could never execute.ILSPInjectedModuleServicehad been widened into a read-write control surface with no caller check, and anXposedServicewas injected into every hooked process. Neither is needed for hot reload.openRemoteFilecollapsed two methods with deliberately different documented semantics onto one, making the module's data directory writable from hooked apps.shared/libxposed-annotationhand-reimplementedio.github.libxposed:annotation:1.0.0, an artifact already on Maven Central that both submodules' own build files consume.What the rewrite is doing differently
Rebased onto current upstream
masterrather than the older base this branch was built on, and split so each piece is reviewable on its own.getRunningTargets()reports every hooked process, including modules with zero or several Java entry classes, matching the AIDL wording. Those answerUNSUPPORTEDrather than being hidden.hotReloadModule()validates and enqueues, then reports through the callback, instead of blocking the caller for the whole reload.FAILEDwith a null message — the encoding the API reserves exclusively for a module refusal. Where thawing is not possible the failure now carries a diagnostic instead of masquerading as a refusal.SecurityExceptionis raised only for the two conditions the AIDL reserves it for, so aSecurityExceptionthrown by module code still reaches the module app as a result.replaceHook()/setId()swap the hooker inside the single registered record rather than installing a second one and removing the first, which left both in the native callback multimap. The chain also freezes its hooker list when a call starts, so a replacement cannot leak into an in-flight call.detach()drops the framework's reference to the entry and stops every lifecycle callback includingonHotReloadingandonHotReloaded.de.robv.android.xposed, enforced in the module classloader so reflection is covered too.libxposed-annotationis consumed as the published Maven artifact, which also removes the duplicate-class workaround this branch needed.Where the original content went
Nothing was deleted. The branch as it stood is preserved unchanged at
HSSkyBoy/Vector@api102-old(b186e336), which keeps it available as the reference the review asked for. This PR'smasterbranch will be replaced by the rewrite;api102-oldis the permanent record of what it used to contain.