Closed Bug 79196 Opened 23 years ago Closed 15 years ago

we should not call PR_UnloadLibrary on any plugins

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED INCOMPLETE

People

(Reporter: blizzard, Assigned: blizzard)

Details

Shaver has a good point in bug 76936 that we shouldn't be calling
PR_UnloadLibrary() on any new style plugins.  I agree with this.  It should be
up to the component manager to to release those libraries if and when it's
appropriate.

Second, he makes the point that we shouldn't ever unload old 4.x style plugins
because we can't guarentee that they can be unloaded safely.  The old code
didn't do it so the new code shouldn't either.

And the special code for the jvm in the unloading should go away.
Marking as critical for 0.9.1 since we need to get this right before someone
screws up and makes a plugin that we will never be able to recover from.
Whiteboard: critial for 0.9.1
It's mine since I've promised the ever optimistic shaver that I would see that
it was fixed for 0.9.1.
Assignee: av → blizzard
Don't forget that this will break updating plugins. If we decide to drop this 
feature it should be clearly stated.
This would only break live updating of classic 4x plugins.  XPCOM plugins can't 
be updated for the same reason that the plugin host can't truly unload them.
Actually, we could unload xpcom plugins, and safely, if we turned on the xpcom
module unloading.  The refresh code would have to force the component manager to
sweep out unreferenced components, but then it would be safe to load any XPCOM
plugins that weren't already/still loaded.

(Turning on the module unloading would require us to make some trivial but
sweeping changes to our modules, so that they didn't like about their
unloadability.)

Dynamically reloading plugins was specifically requested by one of our important 
embedding vendors several months ago. cc:ing Embedding folks to provide input 
and importance on the changing of this feature.

I think what is the largest concern is that 4.x style plugins need to be able to 
be reloaded just as they are in Nav 4.x. From what I understand, Arun correct me 
if I'm wrong, most plugin vendors aren't switching to new API anytime soon, 
especially given our current market share and no SDK. Shaver/Blizzard: we 
are having a hard enough time getting plugin vendors simply update their 
installers for locate Netscape/Mozilla let along wrap their plugin in XPCOM.

What troubles me is that on one hand we are telling plugin vendors to use XPCOM 
and the new plugin API but then we won't be able to dynamically install their 
plugin. It's an awful user experience to have to restart the browser after 
installing any plugins (like Java) but on the other hand, that's much better 
than crashing!

Perhaps we must compramize: For 4.x plugin, do EXACTLY what 4.x does <period>. 
Even IE has parity with our 4.x plugin API. For XPCOM, we could probably add new 
plugins without much trouble but can't remove or update currently running ones. 
Perhaps only for upgrade or remove will a user have to restart.
Well, I daresay that said ``important embedding vendor'' should have provided a
safe way to do it -- we don't have one now, because there's no way for a 4.x
plugin to say ``don't unload me, or we'll crash''.

How, exactly, are 4.x plugins reloaded?  If we just PR_UnloadLibrary them, I'll
be surprised, because the Java plugin doesn't seem to crash 4.x.  Can you point
me at the code in the Classic codebase, or provide a detailed description here,
if the 4.x mechanisms differ from those in Classic?

(I can't say that I blame plugin vendors for not updating -- Mozilla's plugin
API is the worst of both worlds: we don't have 4.x parity, especially for
scriptable plugins, and the new APIs are both broken and provide no way to take
advantage of Mozilla's features by participating in the content model or
tweaking the DOM, etc.  I sure wouldn't bother to write plugins for our current
APIs.  I'm hesitant to even say that Mozilla has a blessed new-style plugin API,
because I don't think we want to support this mess, ever.  Why are we telling
plugin vendors to use it?  Do we hate them?)

We can install new XPCOM components on the fly -- xpinstall does it all the
time.  If XPCOM plugins don't work the same way, well, someone should fix the
XPCOM plugin infrastructure (preferably with a flamethrower).  And while you
can't unload or reload _running_ plugins -- which is never safe, as we should
all know now -- we should be able to unload and reload plugins that are not
running, but have been loaded in the past.  (This requires the aforementioned
XPCOM module-unloading work.)

Mike,

Quoting the 4.x API on Shutdown:
http://developer.netscape.com/docs/manuals/communicator/plugin/init.htm#100558

"When the application no longer needs the plug-in, it is shut down and released. 
NPP_Shutdown gives you an opportunity to delete data
allocated in NPP_Initialize to be shared by all instances of a plug-in. 
Communicator calls the plug-in's NPP_Shutdown function,
which informs the plug-in that its library is about to be unloaded, and gives it 
a chance to cancel any outstanding I/O requests, delete
threads it created, free any memory it allocated, and perform any other closing 
tasks. 

The NPP_Shutdown function releases memory or resources shared across all 
instances of a plug-in. It is called once after the last
instance of the plug-in is destroyed, before releasing the plug-in library 
itself."

And then later down....
"WARNING: Library unloading for plug-ins: If a plug-in uses LiveConnect plug-ins 
on pages connected through
     LiveConnect, the plug-in's actual library may not be unloaded until Java 
garbage collection time, when the plug-in's peer
     class native methods are unloaded. Developers should be aware that this 
could happen at any time, even long after the
     NPP_Shutdown call."

My understanding of this is that it is safe for the 4.x plugin to be unloaded 
after Shutdown is called. My guess is that's how the 4.x client works. The 
plugin vendors are even warned that the java gc may still have references and 
that's something they already deal with.  cc:ing Brian Nesse who can provide 
more insight on how the 4.x codebase worked.
So, given that -- which I find pretty surprising, but hey -- why are we crashing
in Java, instead of just having it close up shop when it gets NPP_Shutdown?  And
why doesn't it crash when running in 4.x?

(How is a plugin supposed to delete its other threads, exactly?  I don't think
there's a way to do that with confidence on all our platforms.)
I don't know why the Java plugin isn't crashing in 4.x. Perhaps Ed can provide 
some details.

If I had to take a guess, however, it's because in 6.0 we take the code path of 
XPCOM and there is a problem in shutdown. I bet NPP_Shutdown for the 4.x side 
takes care of preparing to be unloaded as the spec says and in XPCOM we use 
refounts instead. Just guessing. Ed, can you take a look at the source. Who owns 
XPCOM?

What if we did something similar to:

  PRBool isXPCOM = PR_FALSE;
  if (mEntryPoint) {
    nsCOMPtr<nsI4xPlugin> the4xPlugin
    the4xPlugin = do_QueryInterface(mEntryPoint);
    if (!the4xPlugin) {
      isXPCOM = PR_TRUE;
    }
  }
  if (isXPCOM) return; // bail if XPCOM

>The plugin vendors are even warned that the java gc may still have references 
>and that's something they already deal with.

I don't think so.  The java gc holds onto the module but no calls are made into 
the plugin after NPP_Shutdown has been called.  That's different from the 
current situation in Mozilla where instance Release calls could be made after 
nsIPlugin::Shutdown is called.


>If I had to take a guess, however, it's because in 6.0 we take the code path 
>of XPCOM and there is a problem in shutdown.

What problem?


>  if (isXPCOM) return; // bail if XPCOM

That's bad unless you revert to pre-0.8.1 behavior to stop calling 
nsIPlugin::Initialize() and stop holding a reference to the nsIPlugin.  Keep 
the Initialize and Shutdown calls balanced or don't do them at all.


4.x destroys the plugin by calling NPP_Shutdown followed by PR_UnloadLibrary.

Reloading was handled by a mechanism in which the NPP_Destroy proc could return
a chunk of "save" data which could be used to restore the plugin to it's
previous state.
Whiteboard: critial for 0.9.1
QA Contact: shrir → plugins
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → INCOMPLETE
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.