Repository navigation
[in_app_purchase_android] Defer reconnect off the platform-message upcall to fix an uncatchable SIGABRT - #12639
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the in_app_purchase_android package to version 0.5.3. It defers the billing client reconnection logic to a macrotask using Future.delayed(Duration.zero) inside BillingClientManager to prevent a synchronous outbound platform message from within the inbound onBillingServiceDisconnected callback. Additionally, the package's tests are updated to handle this asynchronous delay by using pumpEventQueue, and a regression test is added to verify the deferred reconnection behavior. There are no review comments, so no additional feedback is provided.
bd45782 to
e251d29
Compare
e251d29 to
dc21f9d
Compare
…cate listener responses (#12640) The Play Billing library can invoke the response listener of a single async call more than once (observed in production via its internal watchdog/timeout path, `zzaf.run → zzan.run`, with the real response also arriving). A pigeon reply may only be submitted once; the second completion throws `IllegalStateException: Reply already submitted` on the main thread and kills the process. `startConnection` already guards against a double `onBillingSetupFinished` with its `alreadyFinished` flag; this PR extends the same protection to the other 9 async methods via a `replyOnce(methodName, callback)` wrapper — an `AtomicBoolean.getAndSet(true)` single-shot around the pigeon callback. The first completion wins; duplicates are dropped with a `Log.d`. Call sites bind the pigeon parameter as `rawCallback` and rebind `callback` to the wrapper, so the method bodies are otherwise unchanged and every completion path (null-client error, null-activity error, listener response, RuntimeException catch) is single-shot. Listener side effects (`updateCachedProducts`) still run on dropped duplicates, so if an error response lands first and the real one second, the product cache still receives real data for a later `launchBillingFlow`. Two JUnit regression tests fire a captured listener twice with distinct results and assert exactly one pigeon completion holding the first result. *Developed with AI assistance; tested and verified by the submitter.* Fixes flutter/flutter#191832 Related: flutter/flutter#103432, flutter/flutter#134967 ## Pre-Review Checklist Note: this PR and #12639 both bump the package to 0.5.3; whichever lands second will be rebased and re-bumped. [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
…call to fix an uncatchable SIGABRT BillingClientManager._connect issued startConnection synchronously from inside the onBillingServiceDisconnected callback, which runs on an inbound platform-message JNI upcall; a pending Java exception at that moment trips FML_CHECK in FlutterViewHandlePlatformMessage and aborts the process, uncatchably from Dart. Yield one macrotask before the platform send so it always lands on a fresh task off the upcall. Claude-Session: https://claude.ai/code/session_012juAFafaPDBSvR7pcqTLMd
dc21f9d to
4dd2767
Compare
|
Rebased onto @gmackall @camsim99 — you reviewed the sibling #12640 (the duplicate-reply guard, released in 0.5.4); could one of you take a look at this one too, and re-apply the |
gmackall
left a comment
gmackall
left a comment
There was a problem hiding this comment.
I'm a bit skeptical that deferring _connect by Duration.zero will prevent this SIGABRT. While Dart does invoke FlutterViewHandlePlatformMessage synchronously inside onBillingServiceDisconnected, the Java side of startConnection isn't actually executed synchronously there: DartMessenger.handleMessageFromDart already defers the channel handler to a later Looper turn via PlatformTaskQueue.dispatch (handler.post). Since Java can't enter the inbound JNI call with an already-pending exception, running FlutterViewHandlePlatformMessage one macrotask later on the same thread seems likely to hit the same CheckException failure.
That said, the deferral and _isDisposed check themselves seem harmless, so I'm fine landing this if we either:
- Remove the
SIGABRTcode comments inbilling_client_manager.dart/billing_client_manager_test.dart(and update the CHANGELOG/PR description so it doesn't claim to fix theSIGABRT), or - Have reproduction/logcat evidence (e.g., the Java exception logged by
CheckExceptionright before the abort) showing how deferring off the upcall prevents theSIGABRT.
|
This pull request is not mergeable in its current state, likely because of a merge conflict. Pre-submit CI jobs were not triggered. Pushing a new commit to this branch that resolves the issue will result in pre-submit jobs being scheduled. |
tarrinneal
added
the
waiting for response
BillingClientManager._connectissuesstartConnectionsynchronously frominside the
onBillingServiceDisconnectedcallback. That callback runs insidean inbound platform-message JNI upcall, so the outbound
startConnectionsendhappens re-entrantly on the same stack. When the billing service is flapping,
the Java side has a pending exception when the engine runs
FML_CHECK(CheckException(env))inFlutterViewHandlePlatformMessage, andthe process aborts (
SIGABRT) — uncatchable from Dart(flutter/flutter#144954).
This PR yields one macrotask (
await Future<void>.delayed(Duration.zero))before the platform send, so
startConnectionalways runs on a fresh task offthe upcall. A microtask would not suffice: microtasks drain before control
returns to the engine, so they do not escape the JNI upcall.
_readyFutureisstill assigned synchronously, so
runWithClientcallers are unaffected asidefrom a one-turn deferral; disposal during the deferral is re-checked before
sending.
Existing tests that asserted a synchronous reconnect now pump the event queue;
a new regression test asserts the send is not issued synchronously on the
disconnect callback.
Developed with AI assistance; tested and verified by the submitter.
Fixes flutter/flutter#191831
Pre-Review Checklist
[shared_preferences]///).Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2