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.

Sign up for GitHub

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

Jump to bottom

Include common templates for injecting diagnostic dylibs #4

Closed
lawrencelomax opened this issue Sep 10, 2015 · 1 comment
Closed

Include common templates for injecting diagnostic dylibs #4

lawrencelomax opened this issue Sep 10, 2015 · 1 comment
Labels
enhancement

Comments

@lawrencelomax
Copy link
Contributor

Apps like Reveal allows additional data gathering by loading a dylib in the Application process at launch. DYLD_INSERT_LIBRARIES can be used to automate this from FBSimulatorControl. There could be some helper methods that do the right thing by looking in the right place.

There's also dylibs that Apple uses for Xcode debugging features like the View Hierarchy Debugger, ASAN & the backtrace-across-dispatches. These can be seen by printing the environment when launching Simulators from Xcode.
@lawrencelomax
Copy link
Contributor Author

FBSIMCTL_CHILD prefixed environment variables make this pretty trivial on fbsimctl. The problem is now quite different on Xcode 8 since all dylibs are now required to be signed.

plu added a commit to plu/FBSimulatorControl that referenced this issue Feb 26, 2017
@plu
Hey Travis facebook#4
2eb3789
plu added a commit to plu/FBSimulatorControl that referenced this issue Feb 26, 2017
@plu
Test retry deleteDevice facebook#4
a49eb85
facebook-github-bot pushed a commit that referenced this issue Nov 22, 2017
@lawrencelomax @facebook-github-bot
Fix further memory-management in Video Encoder
af31e75 
Summary:
This code-path has possibly bit-rotted inside `SimulatorKit`, since it's pretty easy to cause a crash with the current implementation. Running with zombies enabled will occasionally yield the following, immediately after the owner of the `SimDisplayVideoWriter` has deallocated:

```
fbsimctl[58503:12175268] *** -[SimVideoMP4File release]: message sent to deallocated instance 0x104ba2c30
fbsimctl[58503:12175284] *** -[SimVideoMP4File completionHandler]: message sent to deallocated instance 0x104ba2c30

````

Consequently causing a segfault on release. I believe that the reason for this may be down to the fact that the `completionHandler` is called *multiple times* which is likely driven by `dispatch_io` completion handlers. This suggests to me that the `SimVideoFile` is not weakly-referenced in a callback, but can become a dangling pointer.

In the deallocation of `SimVideoFile`:

```
frame #0: 0x000000010560b110 SimulatorKit`-[SimVideoFile dealloc]
frame #1: 0x000000010561b31d SimulatorKit`-[SimDisplayVideoWriter .cxx_destruct] + 43
frame #2: 0x00007fffcd38fbb4 libobjc.A.dylib`object_cxxDestructFromClass(objc_object*, objc_class*) + 127
frame #3: 0x00007fffcd3885f6 libobjc.A.dylib`objc_destructInstance + 92
frame #4: 0x00007fffb86000c6 CoreFoundation`-[NSObject(NSObject) __dealloc_zombie] + 150
frame #5: 0x00000001056190c5 SimulatorKit`-[SimDisplayVideoWriter dealloc] + 104
frame #6: 0x0000000100457f3c FBSimulatorControl`-[FBVideoEncoderSimulatorKit .cxx_destruct](self=0x00000001054210b0, _cmd=<no value available>) at FBVideoEncoderSimulatorKit.m:35
frame #7: 0x00007fffcd38fbb4 libobjc.A.dylib`object_cxxDestructFromClass(objc_object*, objc_class*) + 127
frame #8: 0x00007fffcd3885f6 libobjc.A.dylib`objc_destructInstance + 92
frame #9: 0x00007fffb86000c6 CoreFoundation`-[NSObject(NSObject) __dealloc_zombie] + 150
frame #10: 0x0000000100456e94 FBSimulatorControl`-[FBVideoEncoderSimulatorKit dealloc](self=0x00000001054210b0, _cmd="dealloc") at FBVideoEncoderSimulatorKit.m:89
frame #11: 0x00007fffcd3893bd libobjc.A.dylib`(anonymous namespace)::AutoreleasePoolPage::pop(void*) + 781
frame #12: 0x0000000100344e2d libdispatch.dylib`_dispatch_root_queue_drain + 1728
frame #13: 0x000000010034471d libdispatch.dylib`_dispatch_worker_thread3 + 114
frame #14: 0x00000001003b97da libsystem_pthread.dylib`_pthread_wqthread + 1299
frame #15: 0x00000001003b92b5 libsystem_pthread.dylib`start_wqthread + 13
```

However, this sperlunking is likely indicative of why `simctl` has now changed to using `-[SimDisplayVideoWriter videoWriterForDispatchIO: fileType: completionQueue: completionHandler:]`. The implementation of the URL-based constructors is to wrap the construction of the `dispatch_io` constructor. Another interesting aspect is that the `dealloc` of `SimVideoFile` itself stimulates the `completionHandler` being called. Over-retaining the `SimVideoFile` so that it is not dealloced will cause the callbacka to *never* be called.

During this, I discovered that the `completionHandler` was missing a parameter, which is a `nullable NSError *`, inline with how the other `completionHandler`s work in `CoreSimulator`. Upon cancellation, `ECANCELED` is wrapped in the `NSError *`, so we can handle this case as well.

This may be slightly more reliable than the prior implementation, if there are further issues, we should migrate to using `dispatch_io` from above.

Reviewed By: antiarchit

Differential Revision: D6395394

fbshipit-source-id: 3077efdd471dd0265c44addfe84c0f8a1941ceba
facebook-github-bot pushed a commit that referenced this issue Jun 17, 2021
@lawrencelomax @facebook-github-bot
FBProcessIO: Attachment only once, detachment allowed multiple times
41e65c2 
Summary:
`-[FBProcessIO detach]` should be called at least one time once a process has exited. However, the current implementation for pipe-based consumers (data consumers, block consumers etc) if `-[FBProcessIO detach]` will error out and bail early another detachment is occurring. That started detachment may finish after the error condition, if the caller is listening for the errored-out attachment, then detachment errors can occur before the detachment has finished elsewhere

This identifies another race condition; D29194120 (dac5809) fixes problems in `FBTask` where the properties on it do not wait until IO is drained. However, there's still a problem that relates to `-[FBProcessIO detach]`.

1) A process is running, with the process futures all wrapped with detachment handling.
2) The `-[FBProcessIO detach]` is in sequence for `statLoc`, `exitCode` and `signal`. This causes each output to teardown.
3) The same detachment code is called for each of stdIn, stdErr, stdOut.
4) The teardown for `statLoc`'s `stdOut` teardown can succeed first
5) The teardown for `exitCode`'s `stdOut` teardown then fails and the error is handled
6) However, the real teardown in #4 is still pending. This means that `exitCode` can have resolved, before the teardown has actually occurred, which we explicitly wanted to avoid

There was a further problem with the `FBProcessIO` class. Since the internal queue used for serialisation was a concurrent queue, all of the serialisation that was necessary to prevent attachments and detachments from fighting with each other was effectively redundant. This could have resulted in crashes and other indeterminate states if attachments and detachments ran concurrently with themselves or each other.

Reviewed By: jbardini

Differential Revision: D29195990

fbshipit-source-id: 50722ce1eb4293ac6cf6183459d169b42e0c40e7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@lawrencelomax