1171716 - |nsThreadManager::GetMainThread| emits "WARNING: '!mMainThread'" over 11,000 times in linux debug test logs

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Eric Rahm [:erahm]]

> WARNING: '!mMainThread', file xpcom/threads/nsThreadManager.cpp, line 299

By dumping the stack trace at each location I've narrowed this down to the nsBaseChannel dtor [1] being invoked during CC after the thread manager has shut down (see attached logs).

This behavior was introduced in bug 1073282.

So we could:
  a) Remove this warning
  b) Somehow cleanup those channels before the thread manager is shutdown
  c) Add a way to check if the thread manager is shutdown and not attempt to get the main thread if it is

[1] https://hg.mozilla.org/mozilla-central/annotate/98820360ab66/netwerk/base/nsBaseChannel.cpp#l74

Comment 1

[Eric Rahm [:erahm]]

Attachment: Shutdown log

Comment 2

[Eric Rahm [:erahm]]

Attachment: Part 1: Convert !mMainThread WARN_IF to NS_ERROR

By using NS_ERROR we'll get stack traces during testing which makes tracking
down errors easier.

Comment 3

[Eric Rahm [:erahm]]

Attachment: Part 2: Only proxy the release of |nsBaseChannel::mInfo| if not on main thread

By using |NS_IsMainThread| we can avoid poking the nsThreadManager during
shutdown. This is logically equivalent to the current code in the that
|NS_ProxyRelease| just releases the passed in ref if the thread to proxy on
is null (which it is during shutdown).

Comment 4

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b295232aeaa

Comment 5

[Eric Rahm [:erahm]]

Attachment: Part 3: Only proxy the release of |WebSocketChannel| members if not on main thread

Comment 6

[Eric Rahm [:erahm]]

Attachment: Part 4: Only proxy the release of |JarChannel| members if not on main thread

Comment 7

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=446209444b12

Comment 8

[Eric Rahm [:erahm]]

Attachment: Part 5: Only proxy the release of |HttpBaseChannel| members if not on main thread

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8616073 [details] [diff] [review]
Part 2: Only proxy the release of |nsBaseChannel::mInfo| if not on main thread

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

::: netwerk/base/nsBaseChannel.cpp
@@ +68,5 @@
>  }
>  
>  nsBaseChannel::~nsBaseChannel()
>  {
> +  if (mLoadInfo && !NS_IsMainThread()) {

So, two things:

1. I wonder if we shouldn't just wrap all these changes up in an:

NS_ProxyReleaseToMainThread(nsISupports*)

function, with appropriate overloads for smart pointers, and then that could handle the weirdness of checking for NS_IsMainThread() with a centralized explanation of why.

I could see the NS_IsMainThread() check getting some strange looks, as NS_ProxyRelease already directly releases things on its provided thread if the provided thread is the current thread...so why insert extra checks?  And there are other places where we grab the main thread and NS_ProxyRelease to it, too, which makes this (and other patches) look a little strange.

2. Equivalently, we could use the nsMainThreadPtrHolder class from nsProxyRelease.h...which would probably be a lot more work.

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8616107 [details] [diff] [review]
Part 3: Only proxy the release of |WebSocketChannel| members if not on main thread

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

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +1188,5 @@
>    nsCOMPtr<nsIThread> mainThread;
>    nsIURI *forgettable;
> +
> +  if (!NS_IsMainThread()) {
> +    NS_GetMainThread(getter_AddRefs(mainThread));

This change, I think, requires a comment, since it's not at all obvious that it's safe to pass nullptr into nsProxyRelease.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8616072 [details] [diff] [review]
Part 1: Convert !mMainThread WARN_IF to NS_ERROR

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

::: xpcom/threads/nsThreadManager.cpp
@@ +296,5 @@
>  nsThreadManager::GetMainThread(nsIThread** aResult)
>  {
>    // Keep this functioning during Shutdown
> +  if (!mMainThread) {
> +    NS_ERROR("Attempting to get main thread during shutdown");

This change has apparently caused some errors in your try run...

Comment 12

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #11)
> Comment on attachment 8616072 [details] [diff] [review]
> Part 1: Convert !mMainThread WARN_IF to NS_ERROR
> 
> Review of attachment 8616072 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/threads/nsThreadManager.cpp
> @@ +296,5 @@
> >  nsThreadManager::GetMainThread(nsIThread** aResult)
> >  {
> >    // Keep this functioning during Shutdown
> > +  if (!mMainThread) {
> > +    NS_ERROR("Attempting to get main thread during shutdown");
> 
> This change has apparently caused some errors in your try run...

I'm contemplating reverting this one. It seems to just be uncovering the bugs we plan to work on in bug 1142799.

Comment 13

[Eric Rahm [:erahm]]

Attachment: Part 1: Add NS_ReleaseOnMainThread

Comment 14

[Eric Rahm [:erahm]]

Attachment: Part 2: Use NS_ReleaseOnMainThread in nsBaseChannel

Comment 15

[Eric Rahm [:erahm]]

Attachment: Part 3: Use NS_ReleaseOnMainThread in WebSocketChannel dtor

Comment 16

[Eric Rahm [:erahm]]

Attachment: Part 4: Use NS_ReleaseOnMainThread in JarChannel dtor

Comment 17

[Eric Rahm [:erahm]]

Attachment: Part 5: Use NS_ReleaseOnMainThread in HttpBaseChannel dtor

Comment 18

[Nathan Froyd [:froydnj]]

Comment on attachment 8616996 [details] [diff] [review]
Part 1: Add NS_ReleaseOnMainThread

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

r=me with the changes below.  rs=me if you want to fix grammar nits in the original comments that you copy-pasted from. ;)

::: xpcom/glue/nsProxyRelease.cpp
@@ +68,5 @@
> +NS_ReleaseOnMainThread(nsISupports* aDoomed,
> +                       bool aAlwaysProxy)
> +{
> +  // NB: We only need to get a handle to the main thread if we're not already
> +  //     on the main thread or proxying is required.

I think this comment just repeats what the code is saying, without telling you why.  How about:

// NS_ProxyRelease treats a null event target as "the current thread".  So a handle on the main
// thread is only necessary when we're not already on the main thread or the release must
// happen asynchronously.

::: xpcom/glue/nsProxyRelease.h
@@ +100,5 @@
> + *
> + * @param aDoomed
> + *        the doomed object; the object to be released on the main thread.
> + * @param aAlwaysProxy
> + *        normally, if NS_ReleaseOnMainThread is called on the target thread,

"is called on the main thread"

@@ +101,5 @@
> + * @param aDoomed
> + *        the doomed object; the object to be released on the main thread.
> + * @param aAlwaysProxy
> + *        normally, if NS_ReleaseOnMainThread is called on the target thread,
> + *        then the doomed object will released directly. However, if this

"will be released"

@@ +107,5 @@
> + *        thread for asynchronous release.
> + */
> +nsresult
> +NS_ReleaseOnMainThread(nsISupports* aDoomed,
> +                       bool aAlwaysProxy = false);

I would actually provide the definition inline here, as we can likely eliminate the test against aAlwaysProxy.

Comment 19

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=64572b776824

Comment 20

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #18)
> Comment on attachment 8616996 [details] [diff] [review]
> Part 1: Add NS_ReleaseOnMainThread
> 
> Review of attachment 8616996 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the changes below.  rs=me if you want to fix grammar nits in the
> original comments that you copy-pasted from. ;)

I'll update those as well.

> ::: xpcom/glue/nsProxyRelease.cpp
> @@ +68,5 @@
> > +NS_ReleaseOnMainThread(nsISupports* aDoomed,
> > +                       bool aAlwaysProxy)
> > +{
> > +  // NB: We only need to get a handle to the main thread if we're not already
> > +  //     on the main thread or proxying is required.
> 
> I think this comment just repeats what the code is saying, without telling
> you why.  How about:
> 
> // NS_ProxyRelease treats a null event target as "the current thread".  So a
> handle on the main
> // thread is only necessary when we're not already on the main thread or the
> release must
> // happen asynchronously.

Sounds good, will update.

> ::: xpcom/glue/nsProxyRelease.h
> @@ +100,5 @@
> > + *
> > + * @param aDoomed
> > + *        the doomed object; the object to be released on the main thread.
> > + * @param aAlwaysProxy
> > + *        normally, if NS_ReleaseOnMainThread is called on the target thread,
> 
> "is called on the main thread"

Will update.

> @@ +101,5 @@
> > + * @param aDoomed
> > + *        the doomed object; the object to be released on the main thread.
> > + * @param aAlwaysProxy
> > + *        normally, if NS_ReleaseOnMainThread is called on the target thread,
> > + *        then the doomed object will released directly. However, if this
> 
> "will be released"

Will update.

> @@ +107,5 @@
> > + *        thread for asynchronous release.
> > + */
> > +nsresult
> > +NS_ReleaseOnMainThread(nsISupports* aDoomed,
> > +                       bool aAlwaysProxy = false);
> 
> I would actually provide the definition inline here, as we can likely
> eliminate the test against aAlwaysProxy.

Good point, I'll move it out of the cpp.

Comment 21

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f75717d3eba0
https://hg.mozilla.org/integration/mozilla-inbound/rev/be499a3cae5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/10e18e494630
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc8405b07d10
https://hg.mozilla.org/integration/mozilla-inbound/rev/4986f8464f9c

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f7acbdc32c61 for android bustage:
https://treeherder.mozilla.org/logviewer.html#?job_id=10569896&repo=mozilla-inbound

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

More than android.

Comment 24

[Eric Rahm [:erahm]]

Nathan, it looks like inlining that function caused catastrophic failure. Thoughts?

Comment 25

[Nathan Froyd [:froydnj]]

(In reply to Eric Rahm [:erahm] from comment #24)
> Nathan, it looks like inlining that function caused catastrophic failure.
> Thoughts?

Probably because you didn't declare it static and/or inline, so every .o that included nsProxyRelease.h exported a copy of that function, which causes problems at link time.

Comment 26

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9455f80906f3

Comment 27

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #25)
> (In reply to Eric Rahm [:erahm] from comment #24)
> > Nathan, it looks like inlining that function caused catastrophic failure.
> > Thoughts?
> 
> Probably because you didn't declare it static and/or inline, so every .o
> that included nsProxyRelease.h exported a copy of that function, which
> causes problems at link time.

Ah yes, lets go with inline. I'm still a little confused why this didn't break locally though...

Comment 28

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/16921de2295b
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5bbc24eb8a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/a20e64e16a35
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4b34d4e6e7a
https://hg.mozilla.org/integration/mozilla-inbound/rev/242d698982c3

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/16921de2295b
https://hg.mozilla.org/mozilla-central/rev/d5bbc24eb8a1
https://hg.mozilla.org/mozilla-central/rev/a20e64e16a35
https://hg.mozilla.org/mozilla-central/rev/b4b34d4e6e7a
https://hg.mozilla.org/mozilla-central/rev/242d698982c3