sammcewan / WYPopoverController

WYPopoverController is for the presentation of content in popover on iPhone / iPad devices. Very customizable.
Other
253 stars 74 forks source link

Crash when presenting an UIActivityViewController in popover on iPad #22

Open Bluezen opened 9 years ago

Bluezen commented 9 years ago

Hello, First of all, thanks for maintaining this project!

I encounter a crash when touching the "More" button of an UIActivityViewController presented in a system popover on iPad with iOS8.

ios simulator screen shot 1 nov 2014 15 13 33

Crash happens in WYPopoverController.m file: crash

You can reproduce the crash by using my modified version of the demo project (I simply added a toolbar with a button to present a UIActivityViewController 4f2e75564ceded8d799b1e8a789a404d1c125727) on my fork: https://github.com/Bluezen/WYPopoverController/tree/demoCrash You have to run the demo project on an iPad (real device or simulator) with iOS8, touch the button on the toolbar then touch the "More" button.

I still can't figure out why there is a EXC_BAD_ACCESS here so I'm using a "ghetto fix" for the moment:

if ([self isKindOfClass:[UINavigationController class]] == NO && self.navigationController != nil && [self respondsToSelector:@selector(popoverPresentationController)] == NO)
    {

but this doesn't solve the underlying problem and is just one ugly way to work around it.

I hope someone investigation will prove better than mine.

keremerkan commented 9 years ago

I have encountered the exact same problem and I am not even using WYPopoverController to present the ActivityViewController. I am using the system provided UIPopoverController. This is strange.

keremerkan commented 9 years ago

Ah, I did not see that that method was a UIViewController category method. That's why it is called even though I am not using WYPopoverController. Your temporary fix seems to be working on iOS 8.1 @Bluezen

Bluezen commented 9 years ago

Yes, you are describing the exact same problem. It is indeed because WYPopoverController swizzle the setPreferredContentSize:method of UIViewController that the crash occurs even though we are not explicitly using WYPopoverController.

My temporary solution is simply a safeguard, it prevents the faulty line to be called on iOS8 and could yield bad side effects. I chose to ignore it because I never use a UINavigationController in my WYPopovers... so be careful, this is not a fix!

Ok, after spending too much time again on it I might have a better idea of what the real problem is and much more importantly, I have a better understanding of what sizzled_setPreferredContentSize does.

I wrongly assumed that there was an error in sizzled_setPreferredContentSize but we are instead facing an unexpected side effect of messing with a widely used system method.

The recursive call that

[self.navigationController setPreferredContentSize:aSize];

creates is intended and

if ([self isKindOfClass:[UINavigationController class]] == NO && self.navigationController != nil)

is the safeguard that should stop it to loop indefinitely.

It seems that

[self.navigationController setPreferredContentSize:aSize];

is a necessary call to be able to resize a WYPopoverController dynamically but unfortunately the safeguard doesn't play its role well when a system UIPopover with a UINavigationController is the one to yield the call.

We have to understand why and how to stop the infinite loop in this case.

Bluezen commented 9 years ago

Ok, what is happening is very strange. The issue has something to do with the fact that we are dealing with private classes. In the internal hierarchy of a presented UIActivityViewController there is what seems to be a subclass of UINavigationController called _UIUserDefaultsActivityNavigationController (visible after pushing the "more" button).

In our situation the erroneous call starting the infinite loop happens in sizzled_setPreferredContentSize when self is of kind _UIActivityUserDefaultsViewController and self.navigationController an instance of kind _UIUserDefaultsActivityNavigationController.

The thing I cannot explain is what happen in this precise situation when the following message is sent:

[self.navigationController setPreferredContentSize:aSize];

The swizzled function sizzled_setPreferredContentSize is called, which seems normal, but we are in the exact same configuration than before; self is the same instance of _UIActivityUserDefaultsViewController than before instead of being the instance of navigationController _UIUserDefaultsActivityNavigationController. This causes the infinite loop.

I can only guess that the bizarre behavior is because the _UIUserDefaultsActivityNavigationController is internally forwarding the setPreferredContentSize to our instance of _UIActivityUserDefaultsViewController, hence the exact same configuration and the loop... or something like that.

Anyway, the best solution I can come up with for the moment is to prevent the call to be made only in this precise situation:

- (void)sizzled_setPreferredContentSize:(CGSize)aSize
{
    [self sizzled_setPreferredContentSize:aSize];

    BOOL isPrivateUIActivityNavigationController = [NSStringFromClass(self.navigationController.class) isEqualToString:@"_UIUserDefaultsActivityNavigationController"];

    if ([self isKindOfClass:[UINavigationController class]] == NO
        && self.navigationController != nil
        && isPrivateUIActivityNavigationController == NO)
    {
#ifdef WY_BASE_SDK_7_ENABLED
        if ([self respondsToSelector:@selector(setPreferredContentSize:)]) {
            [self.navigationController setPreferredContentSize:aSize];
        }
#endif
    }
}

That prevents the crash to happen but doesn't break dynamic resize of WYPopover presenting a UINavigationController.

keremerkan commented 9 years ago

Well, this still is not a perfect solution as you said, though thank you very much for trying to get to the bottom of the problem. This can happen on other private classes, so we need to come up with a better system.

Bluezen commented 9 years ago

Yeah, you are right, this could happen with other private classes. Maybe we should take the problem backwards, instead of preventing private classes to make a mess we should specify to do nothing if we are not dealing with a WYPopover.

keremerkan commented 9 years ago

Yes, that's exactly what I was trying to say. Adding categories on UIViewController is a little bit dangerous IMHO.

Bluezen commented 9 years ago

I agree with you, if possible we should avoid swizzle such methods of UIViewController. Maybe this should be refactored to be handled differently. In the meantime I am suggesting this:

- (void)sizzled_setPreferredContentSize:(CGSize)aSize
{
    [self sizzled_setPreferredContentSize:aSize];

    if ([self isKindOfClass:[UINavigationController class]] == NO && self.navigationController != nil)
    {
#ifdef WY_BASE_SDK_7_ENABLED
        if ([self.navigationController isEmbedInPopover] == NO) {
            return;
        } else if ([self respondsToSelector:@selector(setPreferredContentSize:)]) {
            [self.navigationController setPreferredContentSize:aSize];
        }
#endif
    }
}
YoniTsafir commented 9 years ago

:+1: This happens to me too, when will a fix be released?

mingshing commented 8 years ago

The pull request in the original repo will resolve this issue. https://github.com/nicolaschengdev/WYPopoverController/pull/182

vitalys commented 8 years ago

I'll check this issue tomorrow