google / EarlGrey

:tea: iOS UI Automation Test Framework
http://google.github.io/EarlGrey/
Apache License 2.0
5.61k stars 742 forks source link

Internal private APIs are now marked as "direct" in iOS 14 #1357

Open LeoNatan opened 4 years ago

LeoNatan commented 4 years ago

https://nshipster.com/direct/

Meaning that most synchronization points, where Earl Grey (and other projects) swizzles so it can monitor the system state, are no longer available for discovery using the ObjC runtime system (reflection), and can no longer be swizzled because the underlying functions are statically called inside the system.

I wanted to start this discussion as early as now, because this will create major issues for this project and should be looked at now.

tirodkar commented 4 years ago

Thanks a lot for filing this @LeoNatan. I'm doing an audit to find out what methods are no longer supported for Swizzling. Till now I've only had reports of some UITouch methods failing.

patchthecode commented 4 years ago

Do you know or have the issue# of the touch methods failing? I am assuming it was referenced in some other issue.

steipete commented 4 years ago

I'm doing some investigation here; it seems UITouch has been changed quite a bit.

KIF is facing the same issue: https://github.com/kif-framework/KIF/issues/1152

Just commenting out setIsTap: and _setIsFirstTouchForView: doesn't fix the issue, taps no longer work.

steipete commented 4 years ago

This seems to be an okay replacement - need more testing tho: (See the PSPDFKit marked part)

@implementation UITouch (GREYAdditions)

- (id)initAtPoint:(CGPoint)point relativeToWindow:(UIWindow *)window {
  GREYThrowOnNilParameter(window);

  point = CGPointAfterRemovingFractionalPixels(point);

  self = [super init];
  if (self) {
    [self setTapCount:1];
    [self setPhase:UITouchPhaseBegan];

    [self setWindow:window];
    [self _setLocationInWindow:point resetPrevious:YES];
    [self setView:[window hitTest:point withEvent:nil]];
    [self setTimestamp:[[NSProcessInfo processInfo] systemUptime]];

    // PSPDFKit: This was changed in iOS 14.
    if ([self respondsToSelector:@selector(_setIsFirstTouchForView:)]) {
        [self setIsTap:YES];
        [self _setIsFirstTouchForView:YES];
    } else {
        [self _setIsTapToClick:YES];

        // We modify the touchFlags ivar struct directly.
        // First entry is _firstTouchForView
        Ivar flagsIvar = class_getInstanceVariable(object_getClass(self), "_touchFlags");
        ptrdiff_t touchFlagsOffset = ivar_getOffset(flagsIvar);
        char *flags = (__bridge void *)self + touchFlagsOffset;
        *flags = *flags | (char)0x01;
    }
  }
  return self;
}

@end
tirodkar commented 4 years ago

We can track the issue here. Issues seen till now -

  1. recordFailureWithDescription is broken and should be a straight move to XCTIssue.
  2. UITouch issues shown by @steipete. We have fixes for these similar to what you've added.
  3. UIKeyboard typing failing or is extremely slow.

I'll have 1 / 2 sent to earlgrey2 on Github by EOD.

steipete commented 4 years ago

Curious with what workaround you came up with. Our EarlGrey v1 fork now passes all tests again with above change.

tirodkar commented 4 years ago

Sorry for the delay here as I was testing this out more.

For recordFailure, the fix is present here - https://github.com/google/EarlGrey/pull/1377 Will send the UITouch fix tomorrow.

We haven't seen any of our swizzling code fail till now, so the private API's we are using seem to be fine with the current beta. However, we would want to move away from the more private classes such as __NSCFLocalDataTask in the future.

LeoNatan commented 4 years ago

In our Detox Sync, we went even more all-in on the __NSCFLocalDataTask tracking because it was more accurate to track network like that. 😣 So far, that API has not been hidden, but some other stuff in the network classes was hidden.

tirodkar commented 4 years ago

Yes, the current swizzling code seems fine, though if you have any other alternatives in Detox, I'll be really happy to take a look.

BTW, UITouch fixes are here - https://github.com/google/EarlGrey/pull/1378. @steipete - our solution is a bit different from the code you shared. We are obtaining the struct for the UITouchFlags and then changing the _setIsFirstTouchToView to YES.

xavierjurado commented 4 years ago

Do you plan to backport the UITouch fix to EarlGrey 1 as well @tirodkar ? If not, would you accept a pull request?

LeoNatan commented 4 years ago

Probably a good idea to go over the internal API headers available in EG and see which is relevant and which isn't, to prevent usage of hidden API by people in the future.

yongjincho92 commented 4 years ago

@steipete I have tried your fix and hitting this error :

stderr: Vendors/Test/EarlGrey/Src/Additions/UITouch+GREYAdditions.m:45:13: error: no visible @interface for 'UITouch' declares the selector '_setIsTapToClick:'
      [self _setIsTapToClick:YES];
       ~~~~ ^~~~~~~~~~~~~~~~
1 error generated.

I do not see the function setIsTapToClick:YES on the Earlgrey repo either. Is this something custom in your fork of Earlgrey?

tirodkar commented 4 years ago

@xavierjurado we do not plan to add it or support EarlGrey 1.0 going forward. If you sent a PR, we could take a look at it, but it won't be vetted out from our side.

@LeoNatan Yes, I have a doc I'm working on about the private API's that we use. Some of the chief ones causing concern are the data tracking classes we use - __NSCFLocalDataTask. Happy to get any suggestions from your side on how to track network requests without it.

@ThreeFingerCoder - That code is pushed to the Github Repo here. Where are you building the code from?

yongjincho92 commented 4 years ago

@tirodkar Sorry I have a fork of the earlgrey locally, and was testing on it.

#if defined(__IPHONE_14_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_14_0
        [touch _setIsTapToClick:YES];
#endif

This actually solved the issue I had above.

LeoNatan commented 4 years ago

@tirodkar We too are swizzling __NSCFLocalDataTask methods, but mostly public ones, such as resume and suspend. We're also swizzling connection:didFinishLoadingWithError:, which is also "public" because it's called by another class (and also I think comes from a protocol), and I don't think their direct implementation survives this barrier. So for now, it's safe.

https://github.com/wix/DetoxSync/blob/master/DetoxSync/DetoxSync/Spies/NSURLSessionTask%2BDTXSpy.m

Otherwise we now swizzle public NSURLSession methods which should be safe:

https://github.com/wix/DetoxSync/blob/master/DetoxSync/DetoxSync/Spies/NSURLSession%2BDTXSpy.m

I took a slightly different approach to swizzling the user's delegate. I subclass the user's class dynamically so that we avoid the issues with analytic and crash handling frameworks (this is a historic issue we had with Earl Grey, which we solved in a PR but there is still potential for issues).

Ideas how to do this can be found here:

https://github.com/wix/DTXObjectiveCHelpers/blob/a05861bfe0a1b227cd6308fd834a7fce532e77e8/DTXSwizzlingHelper.h#L99


In another project, used for profiling applications, which I haven't yet looked to "modernize" for iOS 14, I have more private stuff swizzled for __NSCFLocalDataTask which I need to consider how to sidestep.

steipete commented 4 years ago

@ThreeFingerCoder Yup, just a private header to allow calling the function.