1648031 - NS_INLINE_DECL_REFCOUNTING not working with TaskQueue

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Jean-Yves Avenard [:jya]]

In a try run for bug 1647628 and 1634846 we're seeing heaps of assertions

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=307343832&repo=try&lineNumber=6983
Crash reason: SIGSEGV /SEGV_MAPERR
[task 2020-06-24T09:09:56.146Z] 09:09:56 INFO - Crash address: 0x0
[task 2020-06-24T09:09:56.146Z] 09:09:56 INFO - Process uptime: not available
[task 2020-06-24T09:09:56.146Z] 09:09:56 INFO -
[task 2020-06-24T09:09:56.146Z] 09:09:56 INFO - Thread 18 (crashed)
[task 2020-06-24T09:09:56.146Z] 09:09:56 INFO - 0 libxul.so!nsAutoOwningThread::AssertCurrentThreadOwnsMe(char const*) const [nsISupportsImpl.cpp:f877d8f65eb357a50eb621b2e724670cc1f2c37d : 40 + 0x1e]
[task 2020-06-24T09:09:56.146Z] 09:09:56 INFO - rax = 0x00005608b380ae58 rdx = 0x0000000000000000
[task 2020-06-24T09:09:56.147Z] 09:09:56 INFO - rcx = 0x0000000000000b40 rbx = 0x00007fedb9c7da60
[task 2020-06-24T09:09:56.147Z] 09:09:56 INFO - rsi = 0x00007fedd037d8b0 rdi = 0x00007fedd037c680
[task 2020-06-24T09:09:56.147Z] 09:09:56 INFO - rbp = 0x00007fedb34b0360 rsp = 0x00007fedb34b0350
[task 2020-06-24T09:09:56.147Z] 09:09:56 INFO - r8 = 0x00007fedd037d8b0 r9 = 0x00007fedb34b1700
[task 2020-06-24T09:09:56.147Z] 09:09:56 INFO - r10 = 0x0000000000000002 r11 = 0x0000000000000000
[task 2020-06-24T09:09:56.148Z] 09:09:56 INFO - r12 = 0x00007fedb34b0390 r13 = 0x0000000000000000
[task 2020-06-24T09:09:56.148Z] 09:09:56 INFO - r14 = 0x00007fedc3edb834 r15 = 0x00007fedb9d390f8
[task 2020-06-24T09:09:56.148Z] 09:09:56 INFO - rip = 0x00007fedbc8a10b2
[task 2020-06-24T09:09:56.148Z] 09:09:56 INFO - Found by: given as instruction pointer in context
[task 2020-06-24T09:09:56.148Z] 09:09:56 INFO - 1 libxul.so!mozilla::ipc::ActorLifecycleProxy::Release() [ProtocolUtils.h:f877d8f65eb357a50eb621b2e724670cc1f2c37d : 863 + 0x10]
[task 2020-06-24T09:09:56.148Z] 09:09:56 INFO - rbx = 0x00007fedb3b44200 rbp = 0x00007fedb34b0380
[task 2020-06-24T09:09:56.149Z] 09:09:56 INFO - rsp = 0x00007fedb34b0370 r12 = 0x00007fedb34b0390
[task 2020-06-24T09:09:56.149Z] 09:09:56 INFO - r13 = 0x0000000000000000 r14 = 0x00007fedb3b3e6d0
[task 2020-06-24T09:09:56.149Z] 09:09:56 INFO - r15 = 0x00007fedb9d390f8 rip = 0x00007fedbcf73eb0
[task 2020-06-24T09:09:56.149Z] 09:09:56 INFO - Found by: call frame info

The issue comes from ActorLifecycleProxy using NS_INLINE_DECL_REFCOUNTING

NS_INLINE_DECL_REFCOUNTING's code to verify that you are freeing the object on the right thread is as follow:

return mThread == PR_GetCurrentThread();
https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/xpcom/base/nsISupportsImpl.cpp#44-46

This is rarely going to work with a TaskQueue which uses a thread pool ; you're never guaranteed which actual PR_Thread is in use.

TaskQueue are thread-safe, and it should be allowed to use them with nsIObjects using NS_INLINE_DECL_REFCOUNTING

This problem will certainly slow-down moving from nsThread to background taskqueue.

Comment 1

[Nathan Froyd [:froydnj]]

In other words, you want the assertion to reflect the serial event target that the object was created on, not the actual physical thread?

Comment 2

[Jean-Yves Avenard [:jya]]

That's correct.

I had a quick go and there's difficulty in doing that: first the thread manager may not exist yet when the first nsISupports object are created, and if you change the size of nsAutoOwningThread we get static assertions that some dom objects size have changed.

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1648031 - P1. Don't instantiate an nsAutoOwningThread when it won't be used. r?froydnj

When an nsISupports class is created and NS_DECL_THREADSAFE_ISUPPORTS is used; a nsAutoOwningThread object is instantiated, even though it is not used.

We still need a _mOwningThread variable to exist for the macro to compile; so we use a nsDummyAutoOwningThread which does absolutely nothing.

We want to avoid this so that we don't unnecessarily call GetCurrentSerialEventTarget() which could instantiate a nsThreadManager.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1648031 - P2. Bypass thread-safety check for nsThreadManager.

If we want nsAutoOwningThread to query the currently running nsISerialEventTarget ; we need the thread manager to have been instantiated ; so we don't want nsThreadManager to use it.

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1648031 - P3. Make nsAutoOwningThread work with TaskQueue.

A TaskQueue can't guarantee which thread the code actually runs on, we can't as such check the address of the underlying PRThread.

Depends on D81082

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1648031 - wip nsISupports using nsISerialEventTarget

Depends on D81083

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:jya, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 8

[Jean-Yves Avenard [:jya]]

We can now use NS_INLINE_DECL_REFCOUNTING_ONEVENTTARGET ; not ideal but easier to do.