316414 - Specify and implement precise steps for clean XPCOM shutdown

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Benjamin Smedberg]

The current "xpcom-shutdown" notification is not sufficient to properly shut down components and XPCOM in a clean way. I have written up http://wiki.mozilla.org/XPCOM_Shutdown to specify a precise set of steps of how to shut down XPCOM cleanly with component loaders and event queues and such. There is some prerequisite and postrequisite work I'd like to do along with this.

Comment 1

[Benjamin Smedberg]

I must be somewhat dyslexic.

Comment 2

[Benjamin Smedberg]

Attachment: Part 1 - xpcom-shutdown-threads and xpcom-shutdown-loaders notifications

Turns out I need to land this piece of the puzzle before bug 316416 so that I can reproduce the functionality of nsIComponentLoader.unloadAll which is disappearing.

Comment 3

[Darin Fisher]

Comment on attachment 203958 [details] [diff] [review]
Part 1 - xpcom-shutdown-threads and xpcom-shutdown-loaders notifications

What happens if NS_ShutdownXPCOM is called while inside ProcessPendingEvents?


>Index: xpcom/build/nsXPComInit.cpp

>+    // grab the event queue so that we can process events one last time before exiting
>+    nsCOMPtr <nsIEventQueue> currentQ;
>+    {
>+        nsCOMPtr<nsIEventQueueService> eventQService =
>+                 do_GetService(kEventQueueServiceCID);
>+
>+        if (eventQService) {
>+            eventQService->GetThreadEventQueue(NS_CURRENT_THREAD,
>+                                               getter_AddRefs(currentQ));
>+        }
>+    }

NS_GetCurrentEventQ for the win!


>Index: xpcom/ds/nsObserverList.h

>+  nsCOMArray<nsISupports> mObservers;

Yay, nsCOMArray!

Comment 4

[Benjamin Smedberg]

> What happens if NS_ShutdownXPCOM is called while inside ProcessPendingEvents?

That's can't be allowed, though I'm not sure where the best place to document that is (don't call NS_ShutdownXPCOM from within any COM method!

Comment 5

[Darin Fisher]

> That's can't be allowed, though I'm not sure where the best place to document
> that is (don't call NS_ShutdownXPCOM from within any COM method!

I guess I was thinking that it might be good if there was an easy to determine if that were the case, but whatever... it is a silly question on my part because anyone doing that would surely catch the mistake rather quickly.  I don't think we need to do much about this other than perhaps document what you said.

Comment 6

[Benjamin Smedberg]

I tried to land this today and had to back out. It turns out that lots of our code is dependent on the current ordering of observerservice notifications, which is LIFO. This patch (accidentally) changed that to FIFO ordering, which ends up doing bad things like uninitializing the http connection manager three times and other things. I'll have a patch up shortly which uses the same basic approach but reverses the enumerator so that the existing ordering is preserved.

I'm not sure whether we want to *fix* the ordering issues or document that the observerservice has LIFO behavior.

Comment 7

[Benjamin Smedberg]

Attachment: Reverse enumerator [fixed on trunk]

I'm gone for Thanksgiving, I definitely don't need this till next week.

Comment 8

[Darin Fisher]

Comment on attachment 204098 [details] [diff] [review]
Reverse enumerator [fixed on trunk]

>Index: xpcom/ds/nsArrayEnumerator.cpp

>+nsCOMArrayEnumerator::operator new (size_t size, const nsCOMArray_base& aArray,
...
>+    PRUint32 cur = aReverse ? max - 1 : 0;
>+
>+    for (PRUint32 i = 0; i < max; ++i) {
>+        result->mValueArray[cur] = aArray[i];
>+        NS_IF_ADDREF(result->mValueArray[cur]);
>+        if (aReverse)
>+            --cur;
>+        else
>+            ++cur;
>     }

It might be nice to do something like this instead:

      const PRUint32 incr = aReverse ? -1 : 1;
      for (PRUint32 i = 0; i < max; ++i, cur += incr) {
          result->mValueArray[cur] = aArray[i];
          NS_IF_ADDREF(result->mValueArray[cur]);      
      }


>Index: xpcom/ds/nsObserverList.cpp

> nsresult
> nsObserverList::AddObserver(nsIObserver* anObserver, PRBool ownsWeak)
> {
>     NS_ENSURE_ARG(anObserver);
> 
>     nsAutoLock lock(mLock);

Why is the observer service threadsafe anyways?  The callbacks to
the observers happen on the thread that called nsIObserverService's
observe method, so it seems like there isn't much to be gained by
supporting calls to AddObserver from a background thread.  Such
consumers can (and should) just proxy the call to AddObserver.
Another bug I guess... :-/


>     nsCOMPtr<nsISupports> observerRef;
>     if (ownsWeak) {
>         nsCOMPtr<nsISupportsWeakReference> weakRefFactory = do_QueryInterface(anObserver);
>         NS_ASSERTION(weakRefFactory, "AddObserver: trying weak object that doesnt support nsIWeakReference");

nit: how about wrapping these long lines to 80 chars?


>         if ( weakRefFactory )

nit: kill extra whitespace for consistency


r=darin

Comment 9

[Benjamin Smedberg]

Comment on attachment 204098 [details] [diff] [review]
Reverse enumerator [fixed on trunk]

Part 1 with the corrected reverse-enumerator fixed on trunk.

Comment 10

[[e-mail]]

Comment on attachment 204098 [details] [diff] [review]
Reverse enumerator [fixed on trunk]

>+    return NS_NewArrayEnumerator(anEnumerator, mObservers, PR_TRUE);
Sadly callers to nsObserverService::EnumerateObservers expect a list of *strong* references...

Comment 11

[Benjamin Smedberg]

Comment #10 is covered by bug 319024

Comment 12

[Wayne Mery (:wsmwk)]

(In reply to comment #11)
> Comment #10 is covered by bug 319024

fixed dec 2005

Comment 13

[Gábor Stefanik]

All your deps are belong to fixage? It seems. Otherwise, is there still work going on this?

Comment 14

[Benjamin Smedberg]

I guess we can call this fixed... the bits about module loader shutdown loops isn't done, but probably isn't needed, especially when we get to XPCOMGC.