libgdx / gdx-pay

A libGDX cross-platform API for InApp purchasing.
Apache License 2.0
224 stars 83 forks source link

You have forgot to add the BillingClient.endConnection() to the PurchaseManagerGoogleBilling.dispose() method #180

Closed ProZhar closed 5 years ago

ProZhar commented 6 years ago

https://github.com/libgdx/gdx-pay/blob/1dade184a96dae53a758988482f4d7397b27249a/gdx-pay-android-googlebilling/src/com/badlogic/gdx/pay/android/googlebilling/PurchaseManagerGoogleBilling.java#L170-L178

In javadocs of BillingClient.java it says:

When you are done with this object, don't forget to call endConnection() to ensure proper cleanup. This object holds a binding to the in-app billing service and the manager to handle broadcast events, which will leak unless you dispose it correctly. If you created the object inside the onCreate(Bundle) method, then the recommended place to dispose is the the onDestroy() method.

BillingClient.java

The TrivialDrive v2 sample app shows this has endConnection() too:

public void destroy() {
        Log.d(TAG, "Destroying the manager.");

        if (mBillingClient != null && mBillingClient.isReady()) {
            mBillingClient.endConnection();
            mBillingClient = null;
        }
    }

TrivialDrive v2 sample app BillingManager.java

I use PurchaseManager.dispose() on the Game.dispose()

In this case all good: start app-> start purchase-> cancel purchase-> home button-> start app-> start purchase-> cancel purchase

But I see crashes in this case: start app-> start purchase-> cancel purchase-> back button (this causes Gdx.app.exit() in my case and PurchaseManager.dispose()) -> start app-> start purchase-> cancel purchase-> -> CRASH REPORT

 com.prozhar.ua.spelling E/AndroidRuntime: FATAL EXCEPTION: main
    Process: com.prozhar.ua.spelling, PID: 24311
    java.lang.NullPointerException: Attempt to invoke interface method 'void com.badlogic.gdx.pay.PurchaseObserver.handlePurchaseCanceled()' on a null object reference
        at com.badlogic.gdx.pay.android.googlebilling.PurchaseManagerGoogleBilling.onPurchasesUpdated(PurchaseManagerGoogleBilling.java:211)
        at com.android.billingclient.api.BillingClientImpl$1.onReceive(BillingClientImpl.java:136)
        at com.android.billingclient.api.LocalBroadcastManager.executePendingBroadcasts(LocalBroadcastManager.java:303)
        at com.android.billingclient.api.LocalBroadcastManager.access$000(LocalBroadcastManager.java:44)
        at com.android.billingclient.api.LocalBroadcastManager$1.handleMessage(LocalBroadcastManager.java:114)
        at android.os.Handler.dispatchMessage(Handler.java:105)
        at android.os.Looper.loop(Looper.java:164)
        at android.app.ActivityThread.main(ActivityThread.java:6944)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.Zygote$MethodAndArgsCaller.run(Zygote.java:327)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1374)

Sorry for my english. Fix it faster plaese I use your perfect library in production. :)

ProZhar commented 6 years ago

@keesvandieren can you fix it, please I have many crashing in production, if people close the app, and then start again it creates multiple connections, early connections don't have observer because it is disposed

keesvandieren commented 6 years ago

@MrStahlfelge there seems to be an issue in GoogleBilling implementation. Can you have a look at it?

@ProZhar if you understand the code, you can submit a PR.

MrStahlfelge commented 6 years ago

These are two issues here.

  1. endConnection() is not called => correct, I will add this according to the documentation.
  2. Crash after calling dispose => the stack trace shows that calling the observer's purchaseCancelled() method fails. This is because the observer is set to null in dispose while a request is performed. I assume purchaseRestore() is called on game start and users immediately closing the game see the crash. This can be fixed by adding null checks to all observer calls from callbacks, but I recommend not to call purchaseRestore automatically.
ProZhar commented 6 years ago

@keesvandieren dieren and @MrStahlfelge thanks for answers

2. Crash after calling dispose => the stack trace shows that calling the observer's purchaseCancelled() method fails. This is because the observer is set to null in dispose while a request is performed. I assume purchaseRestore() is called on game start and users immediately closing the game see the crash. This can be fixed by adding null checks to all observer calls from callbacks, but I recommend not to call purchaseRestore automatically.

I think you are right. In my debuging process (see below) I see google keeps all connection where not called endConnection(). Google creates an array of connections and notify all of them. Debugging: -1st variant

  1. start 1st app-> created()->1st billing client (BC) created (1st observer (Obs) exist) ->
  2. close 1st app back button -> Gdx.app.exit()->purchase.dispose() called->1st Obs = null
  3. start 2nd app-> created()->2nd BC created (2nd Obs exist, 1st BC - exist, 1st Obs null -> 3a. handlePurchaseCanceled() -> CRASH, because 1st Obs = null, 2nd exist
  4. close 2nd app back button -> Gdx.app.exit()->purchase.dispose() called-> 2st Obs = null too In this case I see the described srach situation - Obs = null

2nd variant - is fixed :) It was because 1st start app from AS, then pressed home button, 2nd from Samsung Game Launcher - it creates a new app 2nd variant 1. start 1st app-> created()->1st (BC) created (1st (Obs) exist) -> 2. close 1st app via Launch apps menu (Clear all) -> purchase.dispose() not called ->1st Obs exits 3. start 2nd app-> created()->2nd BC created (2nd Obs exist, 1st BC - exist, 1st Obs EXIST -> 3a. handlePurchaseCanceled() called 2 times - because 1st Obs = exis, 2nd exist too 4. close 2nd app back button -> via Launch apps menu (Clear all) -> purchase.dispose() not called -> 1st BC - EXIST, 1st Obs EXIST and 2nd BC - EXIST, 2nd Obs EXIST TOO ~~In this case I see problem too - I see 2 times called observers methods for tht 1st and 2nd observers I tryed "Clear All" apps and reopen 5 times in a row - and I see in debug window 5 times called handlePurchaseCanceled() or other methods of the observer~~

ProZhar commented 6 years ago

@MrStahlfelge

2. Crash after calling dispose => the stack trace shows that calling the observer's purchaseCancelled() method fails. This is because the observer is set to null in dispose while a request is performed. I assume purchaseRestore() is called on game start and users immediately closing the game see the crash. This can be fixed by adding null checks to all observer calls from callbacks, but I recommend not to call purchaseRestore automatically.

In my case I don't call purchaseRestore() on Game.create(). The purchaseRestore() is called only the player press the RestoreButton. I check it, the observer.handleInstall() is always called before purchaseRestore().

ProZhar commented 6 years ago

@MrStahlfelge This is my app You can try this test:

  1. Launch app
  2. Click Shop (bottom-left corner)
  3. Click some purchase
  4. Cancel the purchase via by pressing anywhere outside of the purchase window or android back button
  5. Close app via android home button
  6. Launch apps menu and click Clear All (purchase.dispose() not called because Game.dispose() not called)
  7. Do 1-4 again and you will not see crash
  8. Press android back button and confirm exit from app (there is called purchase.dispose())
  9. Do 1-4 again and you will see crash
MrStahlfelge commented 6 years ago

I see. It will be fixed with the two steps mentioned. I'll add the null checks and call to endConnection next week.

MrStahlfelge commented 6 years ago

Please build the new version locally and test if this fixes your issues.

keesvandieren commented 6 years ago

@ProZhar do you have any results to share on this?

ProZhar commented 6 years ago

@keesvandieren @MrStahlfelge yes, thanks. All works fine. You can create new release.

I did not answer for so long, because I spent time solving the error "No toolchains found in the NDK toolchains folder for ABI with prefix: mips64el-linux-android" when I tried to create the gdx-pay locally build binaries. I found solutions there: there I removed NDK and remove path in File->Project Structure...->Android NDK location

Perhaps you need to add this to README in this place before using

the following command: ./gradlew assemble uploadArchives -PLOCAL

ProZhar commented 6 years ago

@keesvandieren will you release the new version or use a local build?

keesvandieren commented 6 years ago

@ProZhar the error is because gdx-pay was not yet updated to latest android-build-tools version. Had a quarter of time tonight, but still some errors to go: https://github.com/libgdx/gdx-pay/tree/android-build-tools-3-upgrade

Will continue when I have time, may take a while. If you have time to take it, would be appreciated :-)

ProZhar commented 6 years ago

@keesvandieren I would love to help, but I don’t know well gradle and its plugin :(

MrStahlfelge commented 5 years ago

This issue can be closed.