1064672 - Task Tracer: Deleting TLS doesn't function properly

Status: RESOLVED WONTFIX

(Core :: MFBT, defect)

Description

[Shelly Lin [:shelly]]

TraceInfo of a content process needs to be flushed after itself is created by Nuwa. Otherwise the TraceInfo data of Nuwa process will carry over into this content process. We tend to flush the TraceInfo data by deleting and re-creating each TLS of TraceInfo, however, our <mozilla::ThreadLocal> does not implement destructor nor pthread_key_delete(), it causes memory leak eventually.

Comment 1

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Part 1: Add cleanup function to ThreadLocal

Comment 2

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Part 2: Test case

Comment 3

[Cervantes Yu [:cyu] [:cervantes]]

Try push log: https://tbpl.mozilla.org/?tree=Try&rev=d901ecf098f0

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8504610 [details] [diff] [review]
Part 1: Add cleanup function to ThreadLocal

Review of attachment 8504610 [details] [diff] [review]:
-----------------------------------------------------------------

Using thread-local storage and expecting automatic cleanup is probably not a good idea.  It's almost always far better to have it be initialized and cleaned up by explicit operation.

Does this cause every ThreadLocal to require a static initializer?  If it does, I think that's the nail in the coffin for this.  If it doesn't, I'll have to ponder a little more.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8504610 [details] [diff] [review]
Part 1: Add cleanup function to ThreadLocal

Review of attachment 8504610 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this will cause every (static, global-scope) ThreadLocal to require a static initializer.  There are ways around this, of course:

  ThreadLocal<T>& get() {
    static ThreadLocal<T> instance;
    return instance;
  }

But I don't know that we want to go down that road.  ThreadLocal requires explicit initialization right now, and encouraging patterns like the above runs counter to that.  (Some people consider ThreadLocal's current behavior a bug, of course...)

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8504610 [details] [diff] [review]
Part 1: Add cleanup function to ThreadLocal

Review of attachment 8504610 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I don't think we want this.  ThreadLocal is supposed to be something roughly similar to C++11's thread_local storage duration.  It is not a generic way to store something thread-locally, that's amenable to dynamic creation and destruction.  And the static initializer issue only seals things, with the workaround not being something we are going to force on every user.

To sum it up, I don't think you should be using ThreadLocal for your use case.  This would avoid leaking the TLS key, but it wouldn't avoid leaking any allocations the associated value keeps alive -- say, if someone had

static ThreadLocal<int*> intval;
intval.set(new int);

somewhere in their program, then didn't bother clearing it out and deleting the stored pointer prior to destruction.

I wouldn't be opposed to some sort of DynamicThreadLocal class in a separate header that did some of this.  But don't try changing ThreadLocal for this purpose.