Fix leak calling [NSThread currentThread] when underlying thread created outside NSThread API

[ehren]

This is an alternative for the naive leak fix in #1834 (which introduces a use after free).

I took a look at supporting the cdecl destructor argument of pthread_key_create but it looks like any fixes there would complicate the implementation of pthread_setspecific/pthread_getspecific. So this commit just calls the Fls functions directly from NSThread.

[DHowett-MSFT]

Also in CI build 😄

[DHowett-MSFT]

Actually, it seems like this could be wrapped up into the current thread machinery. At file scope, a thread_local StrongId<NSThread>, which _associateWith... and _threadObjectFrom... could refer to. That way, its lifetime would be managed by the same thing that keeps track of its current-threadiness.

[ehren]

That makes sense. I'll include that + some cleanups in the next commit - I can see there's e.g. an unused pthread tls key in that file too.

[ehren]

@DHowett-MSFT I don't think non-POD file scope thread_locals are supported by this version of clang (c2?).

e.g. this is fine at file scope

struct Blah {
    Blah() {
        abort();
    }

   ~Blah() {
        abort();
   }
};
 
thread_local Blah blah;

But even if thread_local worked the lazy approach avoids the static initialization/destruction overhead for every thread created outside the NS* APIs.

I also haven't confirmed but I think this means there's a leak from thread_local use in NSProgress: https://github.com/Microsoft/WinObjC/blob/develop/Frameworks/Foundation/NSProgress.mm#L50

[rajsesh]

Good find and somewhat worrisome. Yes, with NSProgress.mm, there is a leak. s_currentProgressStack could become a thread local static inside _getProgressStackForCurrentThread much like you have done here.

[rajsesh]

I am not sure I understand how this is correct. alloc init should add a reference and currentThread retain should add one?

[ehren]

In the body of the lambda we're not creating the NSThread via alloc init. We just get a non-owning reference by the call to |currentThread|. The call to |currentThread| does happen to create an owning reference (because it's called from a thread not created via the NSThread API), but this is relinquished on thread exit via the thread_local StrongId destructor.

I'll add some comments to the test case when I add in @DHowett-MSFT's suggested changes - likely sometime tonight. It might also be clearer to convert that test to ARC - but I had some difficulty attempting this initially.

[ehren]

@rajsesh-msft there's a few more asserts in the unit test which should improve things

[rajsesh]

Would really like to see the changes suggested by @DHowett-MSFT

[rajsesh]

nit: the private functions should be _set* per coding conventions.

[ehren]

@rajsesh-msft I'll open a pull with the NSProgress change

[rajsesh]

Good find and somewhat worrisome. Yes, with NSProgress.mm, there is a leak. s_currentProgressStack could become a thread local static inside _getProgressStackForCurrentThread much like you have done here.

[rajsesh]

would be great to get the nits addressed, but looks good otherwise.

[DHowett-MSFT]

Submitted these for CI build again.

[DHowett-MSFT]

It looks like these never completed build, but they did submit status updates... @pradipd do you know what happened?
I'll re-queue them in the mean time, but it was a week ago...

[rajsesh]

Very timely @DHowett-MSFT. Take a look at #1937 that @pradipd just opened.

[DHowett-MSFT]

facepalm
I saw @pradipd's pull request, but didn't put two and two together. Thank you!

[ehren]

@pradipd thanks for the test fix!

I took the opportunity to push another change. No change in behavior - but this avoids creating a new thread-lifetime NSThread object unnecessarily (isMainThread is called frequently via UIView RunSynchronouslyOnMainThread)

[rajsesh]

Back in CI build.

[DHowett-MSFT]

Back in CI build again. Sorry @ehren!

[DHowett-MSFT]

Thanks @ehren!