189064 - AppShellService should release the hidden window in response to the xpcom shutdown event

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Embedding: APIs, defect)

Description

[timeless]

Getting service on shutdown. Denied.
         CID: {43147b80-8a39-11d2-9938-0080c7cb1080}
         IID: {e5e5af70-8a38-11d2-9938-0080c7cb1080}
(this is appshell)

nsComponentManagerImpl::GetService(nsComponentManagerImpl * const 0x002e6a04, const nsID & {...}, const nsID & {...}, void * * 0x0012fccc) line 1965
nsGetServiceByCID::operator()(const nsID & {...}, void * * 0x0012fccc) line 101 + 38 bytes
nsCOMPtr_base::assign_from_helper(const nsCOMPtr_helper & {...}, const nsID & {...}) line 80 + 18 bytes
nsCOMPtr<nsIAppShellService>::nsCOMPtr<nsIAppShellService>(const nsCOMPtr_helper & {...}) line 555
nsXULWindow::Destroy(nsXULWindow * const 0x0034c550) line 359 + 28 bytes
nsWebShellWindow::Destroy(nsWebShellWindow * const 0x0034c550) line 1672 + 9 bytes
nsWebShellWindow::Close(nsWebShellWindow * const 0x0034c5a8) line 386
nsAppShellService::~nsAppShellService() line 128
nsAppShellService::`scalar deleting destructor'() + 15 bytes
nsAppShellService::Release(nsAppShellService * const 0x0036ae60) line 135 + 101 bytes
nsCOMPtr_base::assign_assuming_AddRef(nsISupports * 0x00000000) line 436
nsCOMPtr_base::assign_with_AddRef(nsISupports * 0x00000000) line 74
nsCOMPtr<nsISupportsArray>::operator=(nsISupportsArray * 0x00000000) line 586
FreeServiceContractIDEntryEnumerate(PLDHashTable * 0x002e6a48, PLDHashEntryHdr * 0x002dcae0, unsigned int 230, void * 0x00000000) line 1925
PL_DHashTableEnumerate(PLDHashTable * 0x002e6a48, int (PLDHashTable *, PLDHashEntryHdr *, unsigned int, void *)* 0x1004e290 FreeServiceContractIDEntryEnumerate(PLDHashTable *, PLDHashEntryHdr *, unsigned int, void *), void * 0x00000000) line 603 + 34 bytes
nsComponentManagerImpl::FreeServices() line 1937 + 19 bytes
NS_ShutdownXPCOM(nsIServiceManager * 0x00000000) line 730
main(int 4, char * * 0x002e4630) line 1913 + 8 bytes

Yes, appshell calls webshell which calls xulwindow which then tries to get appshell, all while we're deep in shutdown xpcom

The fix is to move the hidden window portion of this code into an Observe method:

nsAppShellService::~nsAppShellService()
{
  mDeleteCalled = PR_TRUE;
  nsCOMPtr<nsIWebShellWindow> hiddenWin(do_QueryInterface(mHiddenWindow));
  if(hiddenWin) {
    ClearXPConnectSafeContext();
    hiddenWin->Close();
  }
  /* Note we don't unregister with the observer service
     (RegisterObserver(PR_FALSE)) because, being refcounted, we can't have
     reached our own destructor until after the ObserverService has shut down
     and released us. This means we leak until the end of the application, but
     so what; this is the appshell service. */
}

Comment 1

[timeless]

Attachment: listen for shutdown

Comment 2

[timeless]

Attachment: listen for shutdown

Comment 3

[Alec Flett]

Comment on attachment 111760 [details] [diff] [review]
listen for shutdown

the code seems fine, so sr=alecf, on condition of r=danm and/or r=law

Comment 4

[Dan M]

Comment on attachment 111760 [details] [diff] [review]
listen for shutdown

Are you certain this patch is helpful? How old is the stack trace at the top of
this bug report? Since August 2002 the hidden window is typically no longer
closed in the nsAppShellService destructor. It's closed in the Quit method. I
just tried a few test runs; the hidden window was always gone by destructor
time, so this code within the "if" test is never executed:

>-  if(hiddenWin) {
>-    ClearXPConnectSafeContext();
>-    hiddenWin->Close();
>-  }

also this code

>+  } else if (!strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) {
>+    nsCOMPtr<nsIWebShellWindow> hiddenWin(do_QueryInterface(mHiddenWindow));
>+    if (hiddenWin) {
>+      ClearXPConnectSafeContext();
>+      hiddenWin->Close();
>+    }
>   }
>   return NS_OK;

will never be hit unless you also make the service an observer of the shutdown
topic.

The patch seems harmless enough because it shouldn't change anything. However
because it's cleaner, if you're motivated, do the shutdown topic observer thing
I just mentioned and I'll + it.

I'm curious, can you reproduce the assertion in this bug using a recent build?
If so, you've found a way to exit the app without going through Quit() and that
seems like a problem.

Comment 5

[Alec Flett]

Comment on attachment 111760 [details] [diff] [review]
listen for shutdown

(clearing my sr= request then!)

Comment 6

[Benjamin Smedberg]

Marking a bunch of bugs in the "Embedding: APIs" component INCOMPLETE in preparation to archive that component. If I have done this incorrectly, please reopen the bugs and move them to a more correct component as we don't have "embedding" APIs any more.