107485 - nsPluginHostImpl's shutdown listener should be a weak reference

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[David Epstein]

reproduced with 10/27 debug mozilla nightly build.
1. Set up debugger in VC++ & set breakpoint in nsComponentManager.cpp: line 2075
2. Launch app in the debugger (F5).
3. Quit the app.
4. Check the stack trace and doubleclick on destructor line:
nsPluginHostImpl::~nsPluginHostImpl() line 2430
Result: Destructor in nsPluginHostImpl() calls do_GetService
Expected: Should register shutdown listener. 

Here's the code for the destructor. It's using nsIObserverService object:
nsPluginHostImpl::~nsPluginHostImpl()
{
  PLUGIN_LOG(PLUGIN_LOG_ALWAYS,("nsPluginHostImpl::dtor\n"));

#ifdef NS_DEBUG
  printf("nsPluginHostImpl dtor\n");
#endif
  nsCOMPtr<nsIObserverService> obsService =
do_GetService("@mozilla.org/observer-service;1");
  if (obsService)
  {
    obsService->RemoveObserver(this, "quit-application");
    obsService->RemoveObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID);
  }
  Destroy();
}

Comment 1

[Peter Lubczynski]

We are already using a shutdown listener here! In fact, the code in the
destructor is unregistering the listener and thefore needs to
do_GetService(nsIObserverService). Would it help to move this unregistering code
to nsPluginHostImpl::Destroy()? This can only be called once....whoever is
first, either the shutdown observer or ~nsPluginHostImpl. Please advise.

I recall one of the reason this being done was there once a bug (and could still
be) that ~nsPluginHostImpl would not always be called at browser shutdown and
plugins wouldn't be able to clean themselves up (bad!). I think one of the
reason it wasn't called was because the service manager was being held past
XPCOM shutdown. Well, it just so happens, that at least on Windows, the JRE
1.3.x Java plugin would hold on to the service manager and cause this bug! I'm
not sure if this is accurate, but I was told the reason Java did this was for
the Java console or taskbar icon or something.

Comment 2

[Bradley Baetz (:bbaetz)]

You should impl nsWeakReference, and then use PR_TRUE as the last argument to
AddObserver. That way you won't need to unregister yourself.

Comment 3

[Denis Antrushin]

Attachment: patch

did I get it right?

Comment 4

[Denis Antrushin]

removing myself from cc:

Comment 5

[timeless]

bug 100212 comment 38 fixed the core problem.

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 71484 [details] [diff] [review]
patch

yup, you got it right. r=dougt.

Comment 7

[Darin Fisher]

Comment on attachment 71484 [details] [diff] [review]
patch

sr=darin

Comment 8

[David Epstein]

wasn't this supposed to be checked in?

Comment 9

[timeless]

Comment on attachment 71484 [details] [diff] [review]
patch

2002-05-30:
mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp	1.390
mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp	1.389
mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp	1.388
mozilla/modules/plugin/base/src/nsPluginHostImpl.h	1.77