Skip to content

Fixed crash on async SK1 cancelled purchase#1869

Merged
NachoSoto merged 1 commit into
mainfrom
purchase-cancel-crash
Aug 29, 2022
Merged

Fixed crash on async SK1 cancelled purchase#1869
NachoSoto merged 1 commit into
mainfrom
purchase-cancel-crash

Conversation

@NachoSoto

@NachoSoto NachoSoto commented Aug 29, 2022

Copy link
Copy Markdown
Contributor

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

vegaro commented Aug 29, 2022

Copy link
Copy Markdown
Member

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

@NachoSoto

Copy link
Copy Markdown
Contributor Author

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

@vegaro

vegaro commented Aug 29, 2022

Copy link
Copy Markdown
Member

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
Copy Markdown
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