Address Sanitizer fix: remove fishhook dependency and hook C functions with GREYInterposer

[axi0mX]

1. Remove fishhook
2. Implement GREYInterposer and use it to hook C functions
3. Make EarlGrey compatible with Address Sanitizer
4. Enable Address Sanitizer for unit tests
5. Fix a deadlock in dyld_sim affecting 64-bit iOS 8.x simulators which made dyld_dynamic_interpose unusable

Instead of manually rebinding tables with fishhook, GREYInterposer uses DYLD's built-in interpose functionality to rebind symbols inside the dynamic linker.

DYLD interposing is also what Address Sanitizer uses to hook hundreds of C functions, including dispatch_after and dispatch_async.

The implementation should work when EarlGrey is linked statically into the main executable, but I did not test this use case. Please verify.

There are 4 use cases GREYInterposer considers for symbols that are statically interposed by Address Sanitizer. This requires using the macro which chains EarlGrey's interpose to Address Sanitizer's static interpose.

1. EarlGrey is a dynamic library, Address Sanitizer ON

• dynamic interposing is performed in Address Sanitizer binary

2. EarlGrey is a dynamic library, Address Sanitizer OFF

• static interposing is performed in all binaries except EarlGrey
• dynamic interposing is performed in EarlGrey binary

3. EarlGrey is a static library, Address Sanitizer ON

• dynamic interposing is performed in Address Sanitizer binary

4. EarlGrey is a static library, Address Sanitizer OFF

• dynamic interposing is performed in all binaries

Resolves #194.

[tirodkar]

We could have asan turned on for the Objective-C tests and off for the Swift ones, that way we could have both areas covered.

[tirodkar]

Maybe change hook to replacement based on the DYLD_INTERPOSE syntax?

[axi0mX]

Changed.

[axi0mX]

Root cause for these issues was a bug in dyld_sim in 64-bit iOS 8.x simulators. Detailed description of the bug and its fix is in GREYInterposer.m. This code should now work properly.

[khandpur]

Thanks for the update @axi0mX . I will run it through internal tests

[steipete]

We're using this branch over at PSPDFKit and I just re-based it for 1.9.2 - still works great. Are there any reasons this hasn't been pursued further? We run Address Sanitizer on all our tests, always, by default. It's just that good.

[khandpur]

hey Peter, apologies for the delay on this one. As I was trying to get it merged internally, I ran into some issues with Google's internal CI but now it seems to be resolved. I have made changes to this PR which need to be applied before I can merge it. @axi0mX can you give me commit privileges to I can make them directly and merge this hopefully by next week.

[steipete]

This no longer works on iOS 11b1. The code is a bit over my head so not sure how to fix. There are 242 static tuples.

'NSInternalInconsistencyException', reason: 'multiple matching tuples found'
*** First throw call stack:
(
	0   CoreFoundation                      0x000000010c8e7f6b __exceptionPreprocess + 171
	1   libobjc.A.dylib                     0x0000000108c7a121 objc_exception_throw + 48
	2   CoreFoundation                      0x000000010c8ecfd2 +[NSException raise:format:arguments:] + 98
	3   Foundation                          0x0000000108730c8a -[NSAssertionHandler handleFailureInMethod:object:file:lineNumber:description:] + 193
	4   EarlGrey                            0x0000000123a74c95 -[GREYInterposer commit] + 7413
	5   EarlGrey                            0x0000000123a6c83a +[GREYDispatchQueueTracker load] + 1114
	6   libobjc.A.dylib                     0x0000000108c7c3bb call_load_methods + 695
	7   libobjc.A.dylib                     0x0000000108c7d0bd load_images + 70
	8   ???                                 0x0000000107febe2c 0x0 + 4429102636
	9   ???                                 0x0000000107ff81f7 0x0 + 4429152759
	10  ???                                 0x0000000107ff819f 0x0 + 4429152671
	11  ???                                 0x0000000107ff73ca 0x0 + 4429149130
	12  ???                                 0x0000000107ff745e 0x0 + 4429149278
	13  ???                                 0x0000000107fee97d 0x0 + 4429113725
	14  ???                                 0x0000000107ff3b9a 0x0 + 4429134746
	15  libdyld.dylib                       0x000000010cecd9c3 dlopen + 86
	16  libclang_rt.asan_iossim_dynamic.dylib 0x000000010ba5a91e wrap_dlopen + 910
	17  CoreFoundation                      0x000000010c898dc8 _CFBundleDlfcnLoadBundle + 152
	18  CoreFoundation                      0x000000010c898c54 _CFBundleLoadExecutableAndReturnError + 324
	19  Foundation                          0x000000010868a6a5 -[NSBundle loadAndReturnError:] + 520
	20  IDEBundleInjection                  0x00000001080d2860 __XCBundleInjection + 798
	21  ???                                 0x0000000107ffc746 0x0 + 4429170502
	22  ???                                 0x0000000107ffc976 0x0 + 4429171062
	23  ???                                 0x0000000107ff820c 0x0 + 4429152780
	24  ???                                 0x0000000107ff73ca 0x0 + 4429149130
	25  ???                                 0x0000000107ff745e 0x0 + 4429149278
	26  ???                                 0x0000000107fec1dd 0x0 + 4429103581
	27  ???                                 0x0000000107fef919 0x0 + 4429117721
	28  ???                                 0x0000000107feb454 0x0 + 4429100116
	29  ???                                 0x000000010923c510 0x0 + 4448306448
	30  ???                                 0x000000010923aa39 0x0 + 4448299577
	31  ???                                 0x0000000109236249 0x0 + 4448281161
	32  ???                                 0x0000000109236036 0x0 + 4448280630
)
libc++abi.dylib: terminating with uncaught exception of type NSException

[steipete]

I've just updated to latest EarlGrey master (1.13+) and re-applied this patch. Should I try to clean this up for merging, or is there anything else planned?

Running tests with ASan enabled is incredibly useful.

[steipete]

This still works, we are using that with the 1.15 release.

Is there any interest in getting this merged?

[tirodkar]

Do you use this in production or for debugging mostly?

[steipete]

We run all our tests with ASAN enabled by default, and can trigger TSAN enabled ones on request (both doesn't work together).

That way we find more issues and races vs running tests without additional checks, especially for UI flow testing.

[tirodkar]

That's interesting. I'll look into adding this for 2.0.

[steipete]

We're still debating if we should migrate fully to EarlGrey from KIF.
EarlGrey has the better API and is more reliable, however as it does a full screenshot for every step, it's by a factor of 10-20x slower than KIF.

I'd love to read up on plans for 2.0, and what the major changes will be (XCUI bridge?) and if performance is something you're considering to improve + a timeline.

[tirodkar]

@steipete Thanks for the performance comparison, I haven't done one myself between kif and eg. If you have a doc or so about your investigation, I'd love to take a look and see what areas could be improved.

We don't take screenshots for every EG interaction - are you pointing to the Visibility Checker?

We'll have docs explaining the design for EG 2 once we get the final release prepped.

[steipete]

(I don't want to highjack this thread, if there's a better place to discuss, let me know)

So a while back we decided that EarlGrey is better than KIF, has a more coherent API and better structure. However our KIF tests mostly just use busy waiting and run insanely fast. I disabled the syncing in EarlGrey:

greyConfiguration.setValue(0.01, forConfigKey: kGREYConfigKeyDelayedPerformMaxTrackableDuration)
greyConfiguration.setValue(false, forConfigKey: kGREYConfigKeySynchronizationEnabled)

We can enable this for most of our tests + some busy spinning to make things really fast, however the visibility checker does full-screen rendering, so our tests now max out CPU and are easily by a factor of 10x slower than a comparable KIF test.

When looking at Instruments what takes up the majority of the time, it's the screen rendering + pixel comparison. Something KIF doesn't do. If visibility checking could opt out of this (by accepting that it might not be that accurate), we could probably get our testing time down from 15 to 5 or fewer minutes.

[khandpur]

Thanks for making us aware of that Peter...we're looking into speeding up EarlGrey 2 visibility checker this quarter.