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

Fixed crash on async SK1 cancelled purchase #1869

Merged
merged 1 commit into from
Aug 29, 2022
Merged

Conversation

NachoSoto
Copy link
Contributor

@NachoSoto NachoSoto commented Aug 29, 2022

Fixes #1868.
See #1868 (comment) for a summary of the changes in #1841.

Before #1841:

  • SK1 / completion-block: no CustomerInfo + cancelled + ErrorCode.purchaseCancelledError
  • SK1 / async: thrown ErrorCode.purchaseCancelledError
  • SK2 / completion-block: CustomerInfo + cancelled + no error
  • SK2 / async: CustomerInfo + cancelled (not error)

After #1841:

  • SK1 / completion-block: no CustomerInfo + cancelled + ErrorCode.purchaseCancelledError
  • SK1 / async: no CustomerInfo + ignored error -> crash! 🚨
  • SK2 / completion-block: CustomerInfo + cancelled + ErrorCode.purchaseCancelledError
  • SK2 / async: CustomerInfo + cancelled (not error)

After this PR:

  • SK1 / completion-block: no CustomerInfo + cancelled + ErrorCode.purchaseCancelledError
  • SK1 / async: thrown ErrorCode.purchaseCancelledError ⚠️
  • SK2 / completion-block: CustomerInfo + cancelled + ErrorCode.purchaseCancelledError
  • SK2 / async: CustomerInfo + cancelled (not error)

The change in #1841 was slightly incorrect. The ignoreIfPurchaseCancelled didn't make a lot of sense because Result(value:error:) already ignored the error if the value wasn't nil.

This change still doesn't get us to a fully consistent behavior across SK1 and SK2 but it adds coverage for this crash and temporarily fixes it.

Fixes #1868.
See #1868 (comment) for a summary of the changes in #1841.

### Before #1841:
- SK1 / completion-block: no `CustomerInfo` + cancelled + `ErrorCode.purchaseCancelledError`
- SK1 / `async`: thrown `ErrorCode.purchaseCancelledError`
- SK2 / completion-block: `CustomerInfo` + cancelled + no error
- SK2 / `async`: `CustomerInfo` + cancelled (not error)

### After #1841:
- SK1 / completion-block:  no `CustomerInfo` + cancelled + `ErrorCode.purchaseCancelledError`
- SK1 / `async`: no `CustomerInfo` + ignored error -> crash! 🚨
- SK2 / completion-block: `CustomerInfo` + cancelled + `ErrorCode.purchaseCancelledError`
- SK2 / `async`: `CustomerInfo` + cancelled (not error)

### After this PR:
- SK1 / completion-block:  no `CustomerInfo` + cancelled + `ErrorCode.purchaseCancelledError`
- **SK1 / `async`: thrown `ErrorCode.purchaseCancelledError`** ⚠️
- SK2 / completion-block: `CustomerInfo` + cancelled + `ErrorCode.purchaseCancelledError`
- SK2 / `async`: `CustomerInfo` + cancelled (not error)

The change in #1841 was slightly incorrect. The `ignoreIfPurchaseCancelled` didn't make a lot of sense because `Result(value:error:)` already ignored the error if the value wasn't `nil`.

This change still doesn't get us to a fully consistent behavior across SK1 and SK2 but it adds coverage for this crash and temporarily fixes it.
@NachoSoto NachoSoto added the pr:fix A bug fix label Aug 29, 2022
@NachoSoto NachoSoto requested a review from a team August 29, 2022 01:31
@vegaro
Copy link
Contributor

vegaro commented Aug 29, 2022

If SK1 is going to throw now, will RevenueCat/purchases-flutter#403 resurface?

@NachoSoto
Copy link
Contributor Author

I believe not since this change is still there, but I'll test it to make sure.

@vegaro
Copy link
Contributor

vegaro commented Aug 29, 2022

I see. I think you're correct. The issue we had was that in SK2, there was no error, but this PR doesn't change that

@NachoSoto
Copy link
Contributor Author

Confirmed this still doesn't crash Flutter.

@NachoSoto NachoSoto merged commit a72b038 into main Aug 29, 2022
@NachoSoto NachoSoto deleted the purchase-cancel-crash branch August 29, 2022 23:51
kaushikpaperboat pushed a commit to Kiddopia/purchases-ios that referenced this pull request Sep 7, 2022
* rc-4.11.0: (964 commits)
  Update RevenueCat-Swift.h for version 4.11.0
  Version bump for 4.11.0
  skip if needed and adds automatic_release (RevenueCat#1870)
  Fixed crash on `async` SK1 cancelled purchase (RevenueCat#1869)
  Added `beginRefundRequest` overload with completion block (RevenueCat#1861)
  Release/4.10.3 (RevenueCat#1867)
  Update fastlane-plugin-revenuecat_internal and fix release-train job (RevenueCat#1866)
  fix typo in comment (RevenueCat#1863)
  Use Dangerfile repository (RevenueCat#1864)
  `CircleCI`: added job for building SDK with `SPM` (RevenueCat#1860)
  `TrialOrIntroPriceEligibilityChecker`: return `.noIntroOfferExists` if the product has no introductory offer (RevenueCat#1859)
  `Lock`: changed default implementation to use `NSLock` (RevenueCat#1819)
  `Offering: Sendable` conformance (RevenueCat#1826)
  `ReceiptParser: Sendable` conformance (RevenueCat#1825)
  `CustomerInfo: Sendable` conformance (RevenueCat#1824)
  Added `Collection.onlyElement` (RevenueCat#1857)
  README updates (RevenueCat#1856)
  `IntegrationTests`: actually fail test if tests aren't configured (RevenueCat#1855)
  `watchOS`: fixed crash on single-target apps (RevenueCat#1849)
  Prepare next version: 4.11.0-SNAPSHOT (RevenueCat#1854)
  ...

# Conflicts:
#	.circleci/config.yml
#	.jazzy.yaml
#	.version
#	CHANGELOG.latest.md
#	CHANGELOG.md
#	Gemfile.lock
#	Package.swift
#	Purchases/Attribution/RCAttributionFetcher.h
#	Purchases/Attribution/RCAttributionFetcher.m
#	Purchases/Attribution/RCAttributionPoster.m
#	Purchases/Attribution/RCAttributionTypeFactory.h
#	Purchases/Attribution/RCAttributionTypeFactory.m
#	Purchases/Caching/RCDeviceCache.m
#	Purchases/Misc/RCSystemInfo.m
#	Purchases/Networking/RCBackend.h
#	Purchases/Networking/RCBackend.m
#	Purchases/ProtectedExtensions/RCAttributionTypeFactory+Protected.h
#	Purchases/Public/RCPurchases.h
#	Purchases/Public/RCPurchases.m
#	Purchases/Public/RCPurchasesErrorUtils.h
#	Purchases/Public/RCPurchasesErrorUtils.m
#	Purchases/Purchasing/RCOfferingsFactory.m
#	Purchases/Purchasing/RCStoreKitRequestFetcher.h
#	Purchases/Purchasing/RCStoreKitRequestFetcher.m
#	Purchases/SubscriberAttributes/RCSubscriberAttributesManager.h
#	Purchases/SubscriberAttributes/RCSubscriberAttributesManager.m
#	PurchasesCoreSwift.podspec
#	PurchasesCoreSwift/Info.plist
#	PurchasesCoreSwift/Logging/Strings/AttributionStrings.swift
#	PurchasesCoreSwift/Logging/Strings/IdentityStrings.swift
#	PurchasesCoreSwift/Logging/Strings/OfferingStrings.swift
#	PurchasesCoreSwift/Misc/OperationDispatcher.swift
#	PurchasesCoreSwift/SubscriberAttributes/SpecialSubscriberAttributes.swift
#	PurchasesTests/Info.plist
#	PurchasesTests/Mocks/MockAttributionFetcher.swift
#	PurchasesTests/Mocks/MockAttributionTypeFactory.swift
#	PurchasesTests/Mocks/MockBackend.swift
#	PurchasesTests/Mocks/MockOfferingsFactory.swift
#	PurchasesTests/Mocks/MockRequestFetcher.swift
#	PurchasesTests/Networking/BackendTests.swift
#	PurchasesTests/Purchasing/OfferingsTests.swift
#	PurchasesTests/Purchasing/PurchasesTests.swift
#	PurchasesTests/SubscriberAttributes/PurchasesSubscriberAttributesTests.swift
#	RevenueCat.podspec
#	Sources/Info.plist
#	Tests/BackendIntegrationTestApp/Info.plist
#	Tests/BackendIntegrationTests/Info.plist
#	Tests/InstallationTests/CocoapodsInstallation/Gemfile.lock
#	Tests/InstallationTests/XcodeDirectInstallation/XcodeDirectInstallation.xcodeproj/project.pbxproj
#	Tests/UnitTests/Attribution/AttributionTypeFactoryTests.swift
#	Tests/UnitTests/Info.plist
#	Tests/UnitTests/Mocks/MockProductsRequest.swift
#	Tests/UnitTests/Mocks/MockSubscriberAttributesManager.swift
#	Tests/UnitTests/Purchasing/StoreKitRequestFetcherTests.swift
#	fastlane/Fastfile
NachoSoto added a commit that referenced this pull request Sep 16, 2022
…with StoreKit 2

Fixes [CSDK-464]. Follow up to #1869.

SK1 and SK2 had inconsistent behavior during cancelled purchases.

- SK1 / completion-block:  no `CustomerInfo` + cancelled + `ErrorCode.purchaseCancelledError`
- **SK1 / `async`: thrown `ErrorCode.purchaseCancelledError`** ⚠️
- SK2 / completion-block: `CustomerInfo` + cancelled + `ErrorCode.purchaseCancelledError`
- SK2 / `async`: `CustomerInfo` + cancelled (not error)

The output is now consistent across the board:

- SK1 / completion-block:  `CustomerInfo` + cancelled + `ErrorCode.purchaseCancelledError`
- SK1 / `async`: `CustomerInfo` + cancelled (not error)
- SK2 / completion-block: `CustomerInfo` + cancelled + `ErrorCode.purchaseCancelledError`
- SK2 / `async`: `CustomerInfo` + cancelled (not error)

This is of course a somewhat backwards incompatible change, but this output is now what makes the most sense based on the types. *This would only break existing code if users are relying on the existence of `CustomerInfo` alone without checking `error` or `userCancelled`.

- Completion-block API:

There's 2 ways of detecting cancellation, both valid and reasonable. Either checking that `cancelled` is `true`, or comparing `error` with `ErrorCode.purchaseCancelledError`

- `async` API:

A cancelled purchase no longer throws. This didn't make sense in SK1, because it meant that `userCancelled` was always `false` in the case that the purchase method returned.
NachoSoto added a commit that referenced this pull request Sep 22, 2022
…with `StoreKit 2` (#1910)

Fixes [CSDK-464]. Follow up to #1869.

SK1 and SK2 had inconsistent behavior during cancelled purchases.

### Before this PR:
- SK1 / completion-block: no `CustomerInfo` + cancelled +
`ErrorCode.purchaseCancelledError`
- **SK1 / `async`: thrown `ErrorCode.purchaseCancelledError`** ⚠️
- SK2 / completion-block: `CustomerInfo` + cancelled +
`ErrorCode.purchaseCancelledError`
- SK2 / `async`: `CustomerInfo` + cancelled (not error)

### After this PR:

The output is now consistent across the board:

- SK1 / completion-block: `CustomerInfo` + cancelled +
`ErrorCode.purchaseCancelledError`
- SK1 / `async`: `CustomerInfo` + cancelled (not error)
- SK2 / completion-block: `CustomerInfo` + cancelled +
`ErrorCode.purchaseCancelledError`
- SK2 / `async`: `CustomerInfo` + cancelled (not error)

## Notes

This is of course a somewhat backwards incompatible change, but this
output is now what makes the most sense based on the types. **This would
only break existing code if users are relying on the existence of
`CustomerInfo` alone without checking `error` or `userCancelled`.**

- Completion-block API:

There's 2 ways of detecting cancellation, both valid and reasonable.
Either checking that `cancelled` is `true`, or comparing `error` with
`ErrorCode.purchaseCancelledError`

- `async` API:

A cancelled purchase no longer throws. This didn't make sense in SK1,
because it meant that `userCancelled` was always `false` in the case
that the purchase method returned. This also matches `StoreKit`'s
behavior in `Product.purchase()`

[CSDK-464]:
https://revenuecats.atlassian.net/browse/CSDK-464?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:fix A bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash on failed purchase in async code on iOS 14
2 participants