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 in Swift 1.2) Some methods (eg initWithNibName:bundle) vtabled in release mode #286

Closed
jdbunford opened this issue Dec 1, 2014 · 20 comments

Comments

@jdbunford
Copy link

Hi

Steps to reproduce:

  1. Download the example.
  2. Change the Build Configuartion scheme from 'Debug' to 'Release'
  3. Build and Run

Here is a copy of the exception being raised on startup.

2014-12-01 11:29:38.369 PocketForecast[38575:636356] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Method 'initWithNibName:bundle:' has 2 parameters, but 1 was injected. Inject with 'nil' if necessary'

The wrong parameters seem to be being injected into the initialiser...

Could this be a SWIFT closure problem? Strange it only happens in RELEASE builds.

FYI I am also seeing the same problem with my own SWIFT application using Typhoon both 2.3.0 & 2.3.1

@mowens
Copy link

mowens commented Dec 8, 2014

@jasperblues Interesting...

Appears to be an optimization issue (which is why it would happen on release and not debug). While debugging I noticed the useInitializer closure for the addCityViewController (which is the starting point of the issue) was actually calling the useInitializer closure from the cityDao! Weird! So I figured that maybe some funky stuff was going on with the addresses of these closures.

As a test, I moved the addCityViewController useInitializer controller to a local variable:

        var block: (initializer: TyphoonMethod!) -> Void = { initializer in
            initializer.injectParameterWith("AddCity")
            initializer.injectParameterWith(NSBundle.mainBundle())
        }

        return TyphoonDefinition.withClass(AddCityViewController.self) {
            (definition) in

            definition.useInitializer("initWithNibName:bundle:", parameters: block)

            definition.injectProperty("cityDao", with:self.coreComponents.cityDao())
            definition.injectProperty("weatherClient", with:self.coreComponents.weatherClient())
            definition.injectProperty("theme", with:self.themeAssembly.currentTheme())
            definition.injectProperty("rootViewController", with:self.rootViewController())
        }

Looking at the address space in the debugger (breakpoint at definition.useInitializer):

(lldb) frame variable
(initializer: TyphoonMethod!) -> () block = <no location, value may have been optimized out>

Notice how the block variable does not have a location. After stepping into the useInitializer function:

(lldb) frame variable
(__block_literal_generic *) parametersBlock = 0x00044b40 PocketForecast`reabstraction thunk helper from @callee_owned (@owned Swift.ImplicitlyUnwrappedOptional<ObjectiveC.TyphoonMethod>) -> (@unowned ()) to @callee_unowned @objc_block (@unowned Swift.ImplicitlyUnwrappedOptional<ObjectiveC.TyphoonMethod>) -> (@unowned ()) at CoreComponents.swift

Now the parametersBlock has an address space. This should be the same as the block closure.

A workaround, although something that should NOT be used as an end all be all solution would be to turn off compiler optimization. Set the release optimization level to -Onone.

EDIT trimmed out debugger console output to highlight, what I believe to be, the problem.

@jasperblues
Copy link
Member

@mowens Thanks for the analysis. I just tried the following:

    dynamic override init(nibName nibNameOrNil: String?, bundle nibBundleOrNil: NSBundle?) {
        super.init(nibName: nibNameOrNil, bundle: nibBundleOrNil)
    }

. . override the failing init with a dynamic one that calls super. It did not work :(

@jasperblues
Copy link
Member

Workaround:

It seems Swift has uncovered an issue with checkParametersCount. I commented this out and everything works ok.

As a short-term patch, I'll wrap this in an #if DEBUG

Edit: Actually we'll leave those checks as debug only too. They don't add much besides CPU cycles in release mode.

@jasperblues
Copy link
Member

Fixed in 2.3.2

Since the main issue is resolved, we'll close this now. Will still investigate why the argument checker doesn't like Swift in release mode though.

@jdbunford Sorry this took a while.

@mowens
Copy link

mowens commented Dec 8, 2014

@jasperblues I'm not entirely sure it is a problem with checkParametersCount. checkParametersCount does in fact retrieve the correct number of parameters for initWithNibName:bundle:, however, the parameterBlock that is used to populate the injectedParameters array for the TyphonMethod does not use the correct closure.

Breakpoint 1: Right before stepping into useInitializer:
screen shot 2014-12-08 at 7 10 50 am

Breakpoint 2: Right before stepping into parameterBlock where we would expect the closure for our add addCityViewController definition to be called (with 2 parameters)
screen shot 2014-12-08 at 7 11 01 am

Breakpoint 3: What the? Next execution is in the cityDao closure which only injects 1 parameter (e.g. which is why checkParameters fails 2 > 1 check:
screen shot 2014-12-08 at 7 11 08 am

Breakpoint 4: Stepping over the parameterBlock. Notice how what the? I am called? is printed
screen shot 2014-12-08 at 7 11 27 am

I think there is a deeper underlying issues with the Swift optimization here. If we skip thecheckParemeters call then this will cause on for seen effects

@jasperblues jasperblues reopened this Dec 8, 2014
@jasperblues
Copy link
Member

@mowens You're right. After reading your first analysis I incorrectly assumed that the sample app would either crash spectacularly or work. It appeared to work. So I imagined:

  • The cause of it all unwinding was a side-effect of this method check.
  • The initWithNibName:bundle method was magically vtabled in the first case, but later Swift correctly decided it did in fact need to use dynamic dispatch.

. . . seems a bit crazy in retrospect.

Of course it still crashes, but just for the controller that uses the optimized initWithNibName:bundle, which is not used at start-up. This at least suggests another (very unsatisfactory) workaround: use a custom initializer or property injection instead.

@jasperblues jasperblues changed the title The SWIFT example raises exception when compiled and run in RELEASE mode SWIFT : Some methods vtabled in release mode Dec 23, 2014
@jasperblues
Copy link
Member

@mowens It might be worth raising a Radr issue for this?

@jasperblues jasperblues changed the title SWIFT : Some methods vtabled in release mode SWIFT : Some methods (eg initWithNibName:bundle) vtabled in release mode Dec 23, 2014
@mowens
Copy link

mowens commented Dec 23, 2014

@jasperblues created #19337398 under Apple Bug Reporter. I will update once more information is available

@jasperblues
Copy link
Member

Thanks @mowens !

Sent from my iPhone

On Dec 24, 2014, at 3:22 AM, Mike Owens notifications@github.com wrote:

@jasperblues created #19337398 under Apple Bug Reporter. I will update once more information is available


Reply to this email directly or view it on GitHub.

@mowens
Copy link

mowens commented Feb 11, 2015

From Apple bug report:

Apple Developer Relations10-Feb-2015 12:50 PM

We believe this issue has been addressed in the latest Xcode 6.3 beta, including iOS 8.3 SDK with Swift 1.2.

Please test with this release. If you still have issues, please include any relevant logs or information that could help us investigate.

This is a pre-release version of the complete Xcode developer toolset for Mac, iPhone, iPad, and Apple Watch. This release requires OS X Yosemite.

Xcode 6.3 - Build 6D520o
https://developer.apple.com/xcode/downloads/

Sounds promising! I'll try and find some time to verify this on the latest beta build

@jasperblues
Copy link
Member

@mowens That's great news! Thanks so much for taking care of this. Hey would you like to be featured on the Typhoon website as a contributor? We add a picture and short-bio of folks who've made significant contributions.

@mowens
Copy link

mowens commented Feb 11, 2015

@jasperblues Sad news. I still received a crash on the release configuration. I wasn't able to debug too much into it last night but I will look into it in more detail shortly. Seems like there are some issues resolving symbols of release builds in Xcode Beta which was making it more difficult to determine if the crash point is the same as before or not. Anyways, I'll keep you posted.

As for being listed on the Typhoon website as a contributor, that sounds great :) I'd love to be featured. Thanks man!

@jasperblues
Copy link
Member

@mowens Damn, that's too bad. Hope its resolved soon. At least encouraging that its being given attention.

Can you send us a pic and brief bio?

@mowens
Copy link

mowens commented Feb 19, 2015

@jasperblues GREAT NEWS! Swift 1.2 has fixed the release mode optimizations 👍

I finally had time to really look into the Swift 1.2 code in regards to the Typhoon-Swift project. Turns out the original crash in release mode for 1.2 was cased by some bad Swift code due to the 1.2 upgrade.

I have created a branch with the updated code that now works great under 1.2 :)

https://github.com/appsquickly/Typhoon-Swift-Example/tree/feature/swift-1.2

For parties that are interested there are 2 commits on this branch. The first commit just updates the Pod dependencies. The second commit contains the Swift 1.2 code changes. I am going to keep this as a feature branch until 1.2 is out of beta.

@jasperblues
Copy link
Member

@mowens AWESOME! Thanks so much for taking care of this.

Swift + 💉💉💉💉💉💉 = 🎉 🎉 🎉 🎉 🎉 🎉

@jasperblues jasperblues changed the title SWIFT : Some methods (eg initWithNibName:bundle) vtabled in release mode (Fixed in Swift 1.2) Some methods (eg initWithNibName:bundle) vtabled in release mode Feb 19, 2015
@mowens
Copy link

mowens commented Feb 19, 2015

@jasperblues No problem 👍

I also sent you an email with my pic and bio from mike.owens11@gmail.com

@dereekb
Copy link

dereekb commented Feb 28, 2015

I've run into a problem that I assume is linked to this and also only occurs in releases. Basically the wrong values are being injected and coming from absolutely random (but consistent) locations. For example, an inline string value was being replaced with a value from a TyphoonConfig instance.

Thanks for posting the workaround, looks like my releases are working now! You wouldn't believe how long I spent looking through Typhoon and my code trying to figure out where it was going weird. 👍

@mowens
Copy link

mowens commented Feb 28, 2015

@dereekb could you post your code that is causing the issue?

@dereekb
Copy link

dereekb commented Mar 1, 2015

There is nothing particularly special about it, and the project is bigger than I'd like to post.

The injection that messed up wasn't anything in particular:

public class VISAppContextAssembly : TyphoonAssembly {

    public dynamic func appContext() -> AnyObject {
        return TyphoonDefinition.withClass(VISAppContext.self) { ... }
   }

    public dynamic func destinationKeyAccessor() -> AnyObject {
        return TyphoonDefinition.withFactory(self.appContext(), selector: "makeKeyAccessorWithType:", parameters: { $0.injectParameterWith("destination") })
    }

}
public class VISAppContext : NSObject { 
...
    @objc(makeKeyAccessorWithType:)
    public dynamic func makeKeyAccessor(keyType:String!) -> VISAppContextKeyAccessor {
        NSLog("Making accessor with type \(keyType)")
        return VISAppContextKeyAccessor(appContext:self, keyType:keyType)
    }
}

I restructured this Typhoon definition multiple times, and the injected value was first consistently a string that was defined in a config, and then a CGRect that was defined inline in another injection for UIWindow, so I am pretty certain it was just Swift optimization bugs.

I'll try and reproduce it in another smaller app and post that here if you'd like to see it. I also have a lot of debug outputs that show the values being injected because I was going to bring up the issue here.

This was the only definition that messed up in the whole project though, and messed up in a separate assembly and this assembly, which was why I thought it might have been Typhoon related at first.

@jasperblues
Copy link
Member

I've been leaving this open for folks using Swift prior to version 1.2. I think we can close it soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants