swiftlang / swift

The Swift Programming Language
https://swift.org
Apache License 2.0
67.58k stars 10.36k forks source link

[SR-11969] Public Structs crash iOS 12.x apps when their framework is built with BUILD_LIBRARY_FOR_DISTRIBUTION=YES #54394

Open swift-ci opened 4 years ago

swift-ci commented 4 years ago
Previous ID SR-11969
Radar rdar://problem/58455073
Original Reporter Lazer PDX (JIRA User)
Type Bug

Attachment: Download

Additional Detail from JIRA | | | |------------------|-----------------| |Votes | 8 | |Component/s | | |Labels | Bug | |Assignee | None | |Priority | Medium | md5: 4b83c34e8c58942163fc50af9318ceea

Issue Description:

After setting BUILD_LIBRARY_FOR_DISTRIBUTION to YES, structs in the built framework cause apps to crash when using the struct as a class property. I've created a repo with a project and framework that [reproduces the issue here|https://github.com/zach-babb-streem/build-library-for-distribution-bug-reproducer].

If instead of using the struct as a class property you use it as a variable in a method, the app does not crash. Additionally adding the `@frozen` attribute to the struct in the framework avoids the crash.

An Apple Developer Forum thread about this issue can be found here: https://forums.developer.apple.com/message/398162

beccadax commented 4 years ago

@swift-ci create

eeckstein commented 4 years ago

I could not reproduce this with a recent Xcode 11.3.
Which Xcode version did you use?

swift-ci commented 4 years ago

Comment by Mo Farajmandi (JIRA)

@eeckstein We are facing very similar problem, and it looks like a bug. I quickly tested attached repo, and I was able to reproduce. When testing, please ensure you are testing using iOS 12 simulator (not reproducible in iOS 13).

The same occurs for enum as well. Essentially it looks like a combination of the following steps causes the crash:
a. declaring a stored property (not computed properties) of
b. `unfrozen` struct or enum type
c. .. that is defined inside a binary framework built using modular stability (library evolution) turned on
d. inside a Class (e.g. ViewController) that is referenced in a Storyboard/IB

[looks like unexpected memory layout issue perhaps, if I were to speculate]

eeckstein commented 4 years ago

Thanks! I could reproduce it now

slavapestov commented 4 years ago

@objc open classes are not fully supported when a library built for distribution is run on iOS \< 13. I’m afraid there’s not much we can do except for document and/or warn about this, because class resilience requires new features in the Objective-C runtime.

swift-ci commented 4 years ago

Comment by Camilo Morales (JIRA)

@slavapestov We've been doing some testing and were able to avoid the crash by instantiating VCs from an UIViewController extension, I'm attaching a sample project that demonstrates both scenarios, call to instantiateViewController(withIdentifier: "DetailViewController"), which results in a vanilla UIViewController - and a crash - and via an UIViewController extension which make use of the same API but works fine.

Also setting DetailViewController as the initial view controller causes a crash.

Note this only affects Interface Builder, when declaring all the UI programmatically in your UIViewController sub-class it also works fine and no crashes are observed.

UPDATE

If you first instantiate and push a VC via the provided extension by tapping "Push VC (No Crash)", go back to the main screen and try instantiate via instantiateViewController(withIdentifier: "DetailViewController") by tapping "Push VC (Crash)" crash is avoided.

SR-11969.zip

swift-ci commented 4 years ago

Comment by Stan Stadelman (JIRA)

@slavapestov we're seeing this for the `unfrozen` struct case in Storyboards, and this is a critical issue for our SDK product, which must support iOS12. We're quite worried that existing customer applications would experience unexpected runtime crashes, since the issue isn't detected until the `loadViewFromNibNamed:` selector is invoked.

Question: would it be appropriate for a SDK framework developer to universally apply @frozen attributes to all structs in their module(s)? It appears this would at least match the current behavior.

It sounds like the `frozen` feature set would primarily be relevant to ‘system-style’ frameworks, in which the system will dynamically substitute a newer copy of the binary when the OS upgrades. For a SDK of binary frameworks, any application developer is going to recompile their app against newer versions of the SDK, if for no other reason than to re-embed them in the app bundle. The only `frozen`-relevant case could be if the app developer also has frameworks embedded in the app which are also compiled against a lower versions of the SDK framework, which for whatever reason the developer hopes to avoid recompiling.

If that's the case... the universal addition of @frozen attribute would seem to successfully workaround.

Repro:

// TestFmwk with BUILD_LIBRARY_FOR_DISTRIBUTION=YES
public struct MyStruct: Codable {

    public let x: Int

    public init(x: Int) {
        self.x = x
    }
}
import UIKit
import TestFmwk

/// Will crash at runtime due to `label` IBOutlet not found.
/// Workaround is to set `MyStruct` to `@frozen` and rebuild.
class ViewController: UIViewController {

    public var aStruct: MyStruct?

    override func viewDidLoad() {
        super.viewDidLoad()

        self.aStruct = MyStruct(x: 0)
    }

    @IBOutlet weak var label: UILabel!
}

bumped Feedback with dup:

FB7550087

swift-ci commented 4 years ago

Comment by Stan Stadelman (JIRA)

Additional Repro of unfrozen struct: TestClient.zip

swift-ci commented 4 years ago

Comment by Milos Pesic (JIRA)

I've also noticed this with un-frozen enums that are declared as a member of class, after setting BUILD_LIBRARY_FOR_DISTRIBUTION to YES.

Is the only possible solution at the moment if we want to keep the forward compatibility to manually go and declare all enums/structs with @frozen?

Regards,
Milos

swift-ci commented 4 years ago

Comment by Dylan Bruschi (JIRA)

Our team ran into the same issue when developing a UI SDK built as a static framework with Build Library for Distribution enabled. We were seeing the same crash in iOS 12 when we tried to instantiate client subclasses of SDK view controllers via storyboard. Conspicuously these subclasses were missing from the client Objective-C compatibility header. Our workaround was to create an instance of a given view controller programmatically before initing with storyboard:

_ = overrideViewControllerType.init()
storyboard.instantiate...

This seems to prevent the crash. We also needed to do the same thing for custom UIViews we subclassed from SDK superclasses. Hope this helps someone.