google / promises

Promises is a modern framework that provides a synchronization construct for Swift and Objective-C.
Apache License 2.0
3.79k stars 291 forks source link

Bug in iOS 16 DevBeta1 #185

Closed mlch911 closed 2 years ago

mlch911 commented 2 years ago

https://github.com/google/promises/blob/46c1e6b5ac09d8f82c991061c659f67e573d425d/Sources/Promises/Promise%2BAsync.swift#L28 String can convert to NSError in iOS 16. This most likely is a bug in iOS 16. Hope Apple could fix this soon.

momoraul commented 2 years ago

My app also worked strangely on iOS16, so I tried debugging it and the reason was the code above as well. However, this problem is not only a problem with iOS 16, but potentially also on iOS 15 and lower versions. Just add this code to your project and the problem will be reproduced regardless of iOS version.

extension String: Error {}

So I think that this problem is not only due to the Apple, but potentially to the Promises as well.

It is assumed that the problem occurred in iOS16 because the above code exists in 'SafariServices.framework' of iOS16.

I reported this issue to the Apple Developer forums. https://developer.apple.com/forums/thread/708025

dmace2 commented 2 years ago

@ykjchen Can we get someone to look at this? It's a pretty massive issue that breaks a ton of use cases for Promises

ykjchen commented 2 years ago

Thanks for reporting. It looks like this results in success being handled like an error, in the case where the promise type is String.

Is that the correct read on this?

dmace2 commented 2 years ago

Yes that is correct.

momoraul commented 2 years ago

Thanks for reporting. It looks like this results in success being handled like an error, in the case where the promise type is String.

Is that the correct read on this?

Yes that is correct. However, currently there is only a problem with the String type in iOS16, If it extends to Int: Error {}, etc., it may be a problem for other types as well. For details, refer to the information submitted to the forums. https://developer.apple.com/forums/thread/708025

ykjchen commented 2 years ago

Does changing the check from:

if let error = value as? NSError { 

to something like:

if type(of: error) is NSError.Type

seem like a possible solution?

(from https://stackoverflow.com/a/44698046)

dmace2 commented 2 years ago

I believe that does work yes.

Edit: this does work, every place that attempts casts value to NSError should be replaced with a check of type instead

dmace2 commented 2 years ago

I opened a PR for this here: https://github.com/google/promises/pull/188

dmace2 commented 2 years ago

Is there anything else I need to do in order to get this merged? We are holding off on pushing the next build of our app until this has been resolved

ykjchen commented 2 years ago

Apologies for the delay, thanks for your patience, just added a review.

dmace2 commented 2 years ago

No worries, just got those changes in so hopefully all is done now

dmace2 commented 2 years ago

Can you make a new release so we can target it in spm? @ykjchen

ykjchen commented 2 years ago

Sure, it's done now.