1479035 - Cleanup lifetime handling of thread-local nsThread wrappers

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Kris Maglione [:kmag]]

The lifetime handling of nsThread wrappers is pretty hairy, and got hairier with bug 1476405.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1479035: Part 1 - Don't create event queues for stub nsThread wrappers.

Most of the times when we automatically create nsThread wrappers for threads
that don't already have them, we don't actually need the event targets, since
those threads don't run XPCOM event loops. Aside from wasting memory, actually
creating these event loops can lead to leaks if a thread tries to dispatch a
runnable to the queue which creates a reference cycle with the thread.

Not creating the event queues for threads that don't actually need them helps
avoid those foot guns, and also makes it easier to figure out which treads
actually run XPCOM event loops.

Review commit: https://reviewboard.mozilla.org/r/260106/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260106/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1479035: Part 2 - Get rid of PRThread to nsThread map.

These maps hold strong references which complicate nsThread lifetime handling
considerably, and only have a couple of fringe uses. We have a linked list of
active threads that the thread manager can use for its internal enumeration
purposes, and the external uses are easily done away with, so there doesn't
seem to be much reason to keep the map around.

Review commit: https://reviewboard.mozilla.org/r/260108/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260108/

Comment 3

[Code Review Bot [:reviewbot]]

Comment on attachment 8995742 [details]
Bug 1479035: Part 1 - Don't create event queues for stub nsThread wrappers.

https://reviewboard.mozilla.org/r/260106/#review267142


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/storage/StorageDBThread.cpp:497
(Diff revision 1)
>  void
>  StorageDBThread::ThreadFunc(void* aArg)
>  {
> +  {
> +    auto queue = MakeRefPtr<ThreadEventQueue<EventQueue>>(MakeUnique<EventQueue>());
> +    nsCOMPtr<nsIThread> thread =

Error: Unused "kungFuDeathGrip" 'nsCOMPtr<nsIThread>' objects constructed from temporary values are prohibited [clang-tidy: mozilla-kungfu-death-grip]

    nsCOMPtr<nsIThread> thread =
    ^

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8995742 [details]
Bug 1479035: Part 1 - Don't create event queues for stub nsThread wrappers.

https://reviewboard.mozilla.org/r/260106/#review267308

This seems good.

::: xpcom/threads/nsThread.h:63
(Diff revision 1)
>  
>    nsThread(NotNull<mozilla::SynchronizedEventQueue*> aQueue,
>             MainThreadFlag aMainThread,
>             uint32_t aStackSize);
>  
> +  nsThread();

I suppose the other constructor is already public, but it would be nice if this and the existing constructor were private. =/  A follow-up, perhaps?

::: xpcom/threads/nsThread.h:187
(Diff revision 1)
>    RefPtr<mozilla::SynchronizedEventQueue> mEvents;
>    RefPtr<mozilla::ThreadEventTarget> mEventTarget;

These could probably use some comments to describe their new discriminatory role in the implementation?

::: xpcom/threads/nsThread.cpp:690
(Diff revision 1)
> +nsThread::nsThread()
> +  : mEvents(nullptr)
> +  , mEventTarget(nullptr)
> +  , mShutdownContext(nullptr)
> +  , mScriptObserver(nullptr)
> +  , mThread(nullptr)
> +  , mStackSize(0)
> +  , mNestedEventLoopDepth(0)
> +  , mCurrentEventLoopDepth(-1)
> +  , mShutdownRequired(false)
> +  , mPriority(PRIORITY_NORMAL)
> +  , mIsMainThread(NOT_MAIN_THREAD)
> +  , mCanInvokeJS(false)
> +  , mCurrentEvent(nullptr)
> +  , mCurrentEventStart(TimeStamp::Now())
> +  , mCurrentPerformanceCounter(nullptr)

Can we convert some of these member variables to member initializers in a follow-up?  Or figure out a nice way to use delegating constructors to minimize the duplication between the two constructors?

::: xpcom/threads/nsThreadManager.cpp:471
(Diff revision 1)
>    // OK, that's fine.  We'll dynamically create one :-)
> -  RefPtr<ThreadEventQueue<EventQueue>> queue =
> +  RefPtr<nsThread> thread = new nsThread();

Should we expand the comment to explicitly note that we assume anybody implicitly creating a thread through this method doesn't actually want an event queue for the thread?

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8995743 [details]
Bug 1479035: Part 2 - Get rid of PRThread to nsThread map.

Eric, are you able to review this?  I haven't had the mental space to, but I can review it early next week if you don't have time.

Comment 6

[Eric Rahm [:erahm]]

Comment on attachment 8995743 [details]
Bug 1479035: Part 2 - Get rid of PRThread to nsThread map.

Sorry I don't have time to get to this before PTO.

Comment 7

[Andreea Pavel [:apavel]]

Hi guys, can you please review the patch or point to someone who can? This bug has other dependencies, i'm referring here to Bug 1479273 that has a very high frequency rate. 

Thank you.

Comment 8

[Eric Rahm [:erahm]]

I'm back, I can review. Kris can you rebase and attach as a splinter review?

Comment 9

[Kris Maglione [:kmag]]

Attachment: Part 2 - Get rid of PRThread to nsThread map

These maps hold strong references which complicate nsThread lifetime handling
considerably, and only have a couple of fringe uses. We have a linked list of
active threads that the thread manager can use for its internal enumeration
purposes, and the external uses are easily done away with, so there doesn't
seem to be much reason to keep the map around.

MozReview-Commit-ID: x7dsj6C4x8

Comment 10

[Eric Rahm [:erahm]]

Comment on attachment 9007105 [details] [diff] [review]
Part 2 - Get rid of PRThread to nsThread map

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

Thanks, this is a nice cleanup. r=me with minor tweaks.

::: dom/workers/WorkerPrivate.cpp
@@ +5350,5 @@
>  WorkerPrivate::IsOnWorkerThread() const
>  {
>    // This is much more complicated than it needs to be but we can't use mThread
>    // because it must be protected by mMutex and sometimes this method is called
>    // when mMutex is already locked. This method should always work.

I guess we can get rid of the wording about this being complicated :)

::: xpcom/threads/nsThread.cpp
@@ +422,5 @@
> +  return sMaxActiveThreads;
> +}
> +
> +uint32_t nsThread::sActiveThreads;
> +uint32_t nsThread::sMaxActiveThreads;

Please move these towards the beginning of the file with the other nsThread static.

::: xpcom/threads/nsThreadManager.cpp
@@ +319,5 @@
>  
>    {
>      // We gather the threads from the hashtable into a list, so that we avoid
> +    // holding the enumerator lock while calling nsIThread::Shutdown.
> +    nsTArray<RefPtr<nsThread>> threads;

Can you change the name of this to something like `threadsToShutdown` as it's no longer a full list of the threads.

::: xpcom/threads/nsThreadManager.h
@@ +83,5 @@
>                               bool aCheckingShutdown);
>  
>    static void ReleaseThread(void* aData);
>  
>    nsRefPtrHashtable<nsPtrHashKey<PRThread>, nsThread> mThreadsByPRThread;

We should remove this, right? Also we can get rid of the nsRefPtrHashtable.h include as well.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Kris, mind finishing the patches so we can get them landed? It would stop all the failures as dependent on bug 1479273. Thanks

Comment 12

[Andreea Pavel [:apavel]]

This seems to be missing checkin-needed, can i land it?

Comment 13

[Eric Rahm [:erahm]]

(In reply to Andreea Pavel [:apavel] from comment #12)
> This seems to be missing checkin-needed, can i land it?

There are a few minor review comments to address before landing.

Comment 14

[Andreea Pavel [:apavel]]

Thanks Eric :)

Comment 15

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a03a61d6d724503c3b7c5e31fe32ced1f5d1c219
Bug 1479035: Part 1 - Don't create event queues for stub nsThread wrappers. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f870621361012ba459943212d8c68a9ff81cb16
Bug 1479035: Part 2 - Get rid of PRThread to nsThread map. r=erahm

Comment 16

[Noemi Erli[:noemi_erli]]

Backed out 2 changesets (Bug 1479035) for build bustages bustages netwerk/cache2/target on CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed,busted,exception&classifiedState=unclassified&selectedJob=201621130&revision=5f870621361012ba459943212d8c68a9ff81cb16

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=201621130&repo=mozilla-inbound&lineNumber=14453

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/0f22d244af4dea74766377b7fc3e3bcd292f9057

Comment 17

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5152af6ab3e399216ef6db8f060c257b2ffbd330
Bug 1479035: Part 1 - Don't create event queues for stub nsThread wrappers. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/89a0c0874d400dd324df6fc3627c0c47d130df19
Bug 1479035: Part 2 - Get rid of PRThread to nsThread map. r=erahm

Comment 18

[Narcis Beleuzu [:NarcisB]]

Backed out for assertion failures: mEvents on nsThread.cpp

Push link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed,busted,exception,runnable&classifiedState=unclassified&selectedJob=201765631&revision=89a0c0874d400dd324df6fc3627c0c47d130df19

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/981bcfe73738b56bfb63ce5971cd7ec667b0bba8

Log link:  https://hg.mozilla.org/integration/mozilla-inbound/rev/981bcfe73738b56bfb63ce5971cd7ec667b0bba8

Comment 19

[Eric Rahm [:erahm]]

The IPC thread is setting an observer [1] and then asserting because there's no event queue. This is fallout from part 1 of the patch. I think we need some way for the ipc thread to opt in to having an event queue.

[1] https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/ipc/glue/MessagePump.cpp#392

Comment 20

[Kris Maglione [:kmag]]

(In reply to Eric Rahm [:erahm] from comment #19)
> The IPC thread is setting an observer [1] and then asserting because there's
> no event queue. This is fallout from part 1 of the patch. I think we need
> some way for the ipc thread to opt in to having an event queue.

There already is a way. I special-cased one of the event loop types, but there's another that's apparently used on Windows.

Comment 21

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf6b1314df05066e33bcdf980bb976af35a7d3f7
Bug 1479035: Part 1 - Don't create event queues for stub nsThread wrappers. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/6164a5ced4f748962157a1f0884056a569588cf9
Bug 1479035: Part 2 - Get rid of PRThread to nsThread map. r=erahm

Comment 22

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/cf6b1314df05
https://hg.mozilla.org/mozilla-central/rev/6164a5ced4f7

Comment 23

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12f5280f25c862decd0917eebbcf685853575678
Bug 1479035: Follow-up: Fix invarient which is no longer true. r=me

Comment 24

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/12f5280f25c8