319999 - XPCOM Shutdown: xpcom-shutdown-threads notification

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Benjamin Smedberg]

This is a sub-bug of 316414, to make the "xpcom-shutdown-threads" notification behave as the spec says it should, and to make the necko worker-thread pool use it.

Comment 1

[Benjamin Smedberg]

Attachment: xpcom-shutdown-threads and necko usage, rev. 1

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 205645 [details] [diff] [review]
xpcom-shutdown-threads and necko usage, rev. 1

+        CallGetService(NS_EVENTQUEUESERVICE_CONTRACTID,
+                       (nsEventQueueServiceImpl**) getter_AddRefs(eqs));

imo it would be better to get the service by CID if you're casting to the specific implementation class...

Comment 3

[Darin Fisher]

> imo it would be better to get the service by CID if you're casting to the
> specific implementation class...

Another option would be to define a IID for the concrete class :)

Comment 4

[Benjamin Smedberg]

That's what this code does, CallQI uses the nsEventQueueServiceImpl IID.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

oh sorry, I missed that.

Comment 6

[Darin Fisher]

Comment on attachment 205645 [details] [diff] [review]
xpcom-shutdown-threads and necko usage, rev. 1

>Index: xpcom/build/nsXPComInit.cpp

>-// - Shutdown the main event queue (TODO)
>+// - Shutdown the event queues

hrm...  what about other event queues that live on other threads?
The IMAP thread comes to mind as it is the only other thread that
creates nsIEventQueue instances.


>+        nsRefPtr<nsEventQueueServiceImpl> eqs;
>+        CallGetService(NS_EVENTQUEUESERVICE_CONTRACTID,
>+                       (nsEventQueueServiceImpl**) getter_AddRefs(eqs));

So, why not use nsCOMPtr<nsEventQueueServiceImpl> to avoid the casting?

>Index: xpcom/threads/nsEventQueueService.cpp


> PR_STATIC_CALLBACK(PLDHashOperator)
> hash_enum_remove_queues(const void *aThread_ptr,
>                         nsCOMPtr<nsIEventQueue>& aEldestQueue,
>                         void* closure)
> {
>+  aEldestQueue->StopAcceptingEvents();
>+  return PL_DHASH_REMOVE;
>+}

If there are younger queues, won't this trigger that NS_ERROR you added?
Whereas the older code would avoid that by calling StopAcceptingEvents
on the youngest queue up to the current queue.


>-  // stop accepting events for youngest to oldest
>-  pie->GetYoungest(getter_AddRefs(q));
>-  while (q) {
>-    q->StopAcceptingEvents();
>-    nsCOMPtr<nsPIEventQueueChain> pq(do_QueryInterface(q));
>-    pq->GetElder(getter_AddRefs(q));
>-  }



>+nsEventQueueServiceImpl::Shutdown()
...
>+  // Call StopAcceptingEvents on any queues that are left: this should be only
>+  // the main-thread event queue at this point, since xpcom-shutdown-threads has
>+  // already been notified.
>+  mEventQTable.Enumerate(hash_enum_remove_queues, nsnull);

How about some assertions to alert us if this isn't true?


>   nsIEventQueue* queue = mEventQTable.GetWeak(currentThread);

Maybe GetWeak should return nsPIEventQueueChain instead to avoid
QI calls?


>Index: xpcom/threads/nsPIEventQueueChain.h

> // {8f310040-82a7-11d3-95bc-0060083a0bcf}
> #define NS_IEVENTQUEUECHAIN_IID \
> { 0x8f310040, 0x82a7, 0x11d3, { 0x95, 0xbc, 0x0, 0x60, 0x8, 0x3a, 0xb, 0xcf } }
> 
> class nsIEventQueue;
> 
>-class nsPIEventQueueChain : public nsISupports
>+class nsPIEventQueueChain : public nsIEventQueue
> {
> public:
>     NS_DECLARE_STATIC_IID_ACCESSOR(NS_IEVENTQUEUECHAIN_IID)

NS_IEVENTQUEUECHAIN_IID should be changed.


What about the socket thread and DNS threads?  Should those also respond to
this new event?

Comment 7

[Benjamin Smedberg]

> hrm...  what about other event queues that live on other threads?
> The IMAP thread comes to mind as it is the only other thread that
> creates nsIEventQueue instances.

Client code is responsible for shutting down non-main-thread event queues before the end of the xpcom-shutdown-threads notification (just as they are responsible for joining these threads). I'm pretty sure IMAP and necko both do this currently during the "xpcom-shutdown" notification (which is fine, I think).

> So, why not use nsCOMPtr<nsEventQueueServiceImpl> to avoid the casting?

You can't use COMPtr on an implementation class that multiply-inherits from nsISupports.

> If there are younger queues, won't this trigger that NS_ERROR you added?
> Whereas the older code would avoid that by calling StopAcceptingEvents
> on the youngest queue up to the current queue.

Yes, this is intentional. XPCOM shutdown should not occur during a modal eventloop (this indicates a programming error that warrants an NS_ERROR).

> What about the socket thread and DNS threads?  Should those also respond to
> this new event?

It's not necessary at this point (they currently respond to "xpcom-shutdown").

Comment 8

[Benjamin Smedberg]

> How about some assertions to alert us if this isn't true?

at the top of the same function:
+  NS_ASSERTION(mEventQTable.Count() <= 1,
+               "Event queues for secondary threads exist during shutdown.");

Comment 9

[Benjamin Smedberg]

Attachment: xpcom-shutdown-threads and necko usage, rev. 1.1

This changes the hashtable to store nsPIEventQueueChain and revs the IID of nsPIEventQueueChain, all else should be the same.

Comment 10

[Benjamin Smedberg]

Fixed on trunk.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Would this have caused the event queue service's GetSpecialEventQueue(nsIEventQueueService::UI_THREAD_EVENT_QUEUE) to return null earlier in the shutdown sequence?  The number 1 (bug 327655) and number 2 (not filed) topcrashes on the trunk are both probably failures to null check the result of that function (although in both cases crashes could result if it fails due to dangling pointers) -- although I can't be certain due to lack of good line number information.

Comment 12

[Benjamin Smedberg]

Yes. Any call to nsIThread::GetMainThread (called in GetSpecialEventQueue) after the xpcom-shutdown-threads notification has fired will fail. I wonder if we should be asserting or warning to that effect.