Closed Bug 1171716 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 5 obsolete files)

81.32 KB, text/plain
Details
3.35 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.09 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.94 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.22 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.17 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
> 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
Attached file Shutdown log
By using NS_ERROR we'll get stack traces during testing which makes tracking
down errors easier.
Attachment #8616072 - Flags: review?(nfroyd)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
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).
Attachment #8616073 - Flags: review?(nfroyd)
Depends on: 1145893
Attachment #8616185 - Flags: review?(nfroyd)
Depends on: 1146580
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.
Attachment #8616073 - Flags: review?(nfroyd) → review+
Attachment #8616172 - Flags: review?(nfroyd) → review+
Attachment #8616185 - Flags: review?(nfroyd) → review+
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.
Attachment #8616107 - Flags: review?(nfroyd) → review+
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...
Attachment #8616072 - Flags: review?(nfroyd) → review+
(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.
Attachment #8616996 - Flags: review?(nfroyd)
Attachment #8616073 - Attachment is obsolete: true
Attachment #8616107 - Attachment is obsolete: true
Attachment #8616172 - Attachment is obsolete: true
Attachment #8616185 - Attachment is obsolete: true
Attachment #8616072 - Attachment is obsolete: true
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.
Attachment #8616996 - Flags: review?(nfroyd) → review+
Attachment #8616998 - Flags: review?(nfroyd) → review+
Attachment #8617000 - Flags: review?(nfroyd) → review+
Attachment #8617001 - Flags: review?(nfroyd) → review+
Attachment #8617002 - Flags: review?(nfroyd) → review+
(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.
Nathan, it looks like inlining that function caused catastrophic failure. Thoughts?
Flags: needinfo?(erahm) → needinfo?(nfroyd)
(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.
Flags: needinfo?(nfroyd)
(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...
You need to log in before you can comment on or make changes to this bug.