Closed Bug 316414 Opened 14 years ago Closed 12 years ago

Specify and implement precise steps for clean XPCOM shutdown

Categories

(Core :: XPCOM, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla2.0

People

(Reporter: benjamin, Assigned: benjamin)

References

()

Details

Attachments

(2 files)

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.
Depends on: 316416
I must be somewhat dyslexic.
Blocks: 54050, 271613, 283873
No longer blocks: 54040, 283783
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.
Attachment #203958 - Flags: review?(darin)
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!
Attachment #203958 - Flags: review?(darin) → review+
> 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!
> 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.
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.
I'm gone for Thanksgiving, I definitely don't need this till next week.
Attachment #204098 - Flags: review?(darin)
Blocks: 317657
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
Attachment #204098 - Flags: review?(darin) → review+
Depends on: 318559
Comment on attachment 204098 [details] [diff] [review]
Reverse enumerator [fixed on trunk]

Part 1 with the corrected reverse-enumerator fixed on trunk.
Attachment #204098 - Attachment description: Reverse enumerator → Reverse enumerator [fixed on trunk]
Priority: -- → P2
Depends on: 319999
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 #10 is covered by bug 319024
Depends on: 319024
Depends on: 320324
Depends on: 323262
No longer blocks: 324586
Depends on: 331117
(In reply to comment #11)
> Comment #10 is covered by bug 319024

fixed dec 2005

Target Milestone: mozilla1.9alpha1 → mozilla1.9beta1
All your deps are belong to fixage? It seems. Otherwise, is there still work going on this?
Priority: P2 → P3
Target Milestone: mozilla1.9 M8 → mozilla2.0
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.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.