shrirang khanzode Reporter
	Description • 24 years ago

0509 trunk, this might be the same problem as seen on 77319. But filing bcos the 
stack trace is all skrewed up and I cannot differentiate.

steps:
go to the link above

scroll down where it says "video" and click on the 'Play Movie' link
In the new window, select realplayer >200 and play the movie 
Now, click on any of the movie lins on the right and observe the browser crash

stack :
Call Stack:    (Signature = gklayout.dll + 0x70a2d (0x60370a2d) 90ebf8fa) 
     
   gklayout.dll + 0x70a2d (0x60370a2d) 
                                        
     
   gklayout.dll + 0xd2ad (0x6030d2ad) 
                                        
     
   gklayout.dll + 0x71605 (0x60371605) 
                                        
     
   gklayout.dll + 0x9a77 (0x60309a77) 
                                        
     
   gklayout.dll + 0xd2a4 (0x6030d2a4) 
                                        
     
   gklayout.dll + 0x1a1e5 (0x6031a1e5) 
                                        
     
   gklayout.dll + 0x1a24a (0x6031a24a) 
                                        
     
   gklayout.dll + 0x1a24a (0x6031a24a)

	Peter Lubczynski Assignee
	Comment 1 • 24 years ago

Ouch, this one is bad. CNN is a AOLTW site too! Attaching patch and cc:ing some 
people for review. Need to use the talkback data as testcases as well. This 
patch attempts to fix the RELEASE crash in ~nsPluginInstanceOwner.

	sean echevarria
	Comment 3 • 24 years ago

What's the cause of the problem?  On the face of it, the patch looks like it 
will introduce a leak of xpcom plugins.

	Peter Lubczynski Assignee
	Comment 4 • 24 years ago

This really is the strong ref problem. I'm afraid I may cause even more leaks 
or worse, crashes, trying to fix THAT problem, I'd rather wait for Andrei to 
return from sabatical. But we should not crash in internal betas releases on our 
sites!

It would seem as if I would have introduced a leak. Maybe, it was late, I need 
to run it through it again. Real player crasahes on the CNN testcase at the 
release. In the debugger, the raw pointer was off to space and the vtable was of 
course blown away which sounds like the instance was already released in the 
XPCOM case. However, in 4.x case (ns4xPlugin) this is not the case so I added 
the NS_IF_RELEASE in the StopPluginInstance right above where there is already 
logic to detect an "OLD_SCHOOL" plugin. 

This TODO:
1) Run the Beatnik XPCOM
2) Run the refcount ballancer on both beatnik and Real:
http://www.mozilla.org/performance/refcnt-balancer.html
Running this, I think I should be able to detect if I caused a leak running 
through some javascript, navigate, back/forward, reload, reloadplugins, close, 
etc... test suites and diffing the results. Perhaps QA could help.
3) Now add Java to mix both with and without the patch. There may be leaks here 
due to bug 77319.

	sean echevarria
	Comment 5 • 24 years ago

Peter: I'd bet that the instance pointer is good until the call to 
mActivePluginList.remove in nsPluginHostImpl::StopPluginInstance().  And after 
the remove() call, the pointer has the same value but points to invalid 
memory.  Can you verify?

	Peter Lubczynski Assignee
	Comment 6 • 24 years ago

What about using an nsCOMPtr for the member variable? Would that help us with 
some of the strong ref problems?

	sean echevarria
	Comment 7 • 24 years ago

If what I last wrote was correct, then this is simply a problem with the 
sequence of calls and nsComPtr will not help.

	Peter Lubczynski Assignee
	Comment 8 • 24 years ago

Yup Sean, you are correct.

Right after the call to mActivePluginList.remove in 
nsPluginHostImpl::StopPluginInstance(), aInstance looks valid. In fact, digging 
deeper, it's valid even until TryUnloadPlugin(). There we set mEntryPoint to 
null and call PR_UnloadLibrary(mLibrary); both of which blow away the pointer 
too.

Should we be doing the unloading for an XPCOM plugin here? Do you think we 
should just avoid calling TryUnloadPlugin for XPCOM plugins?

	Peter Lubczynski Assignee
	Comment 10 • 24 years ago

Okay, so now with the patch we will still have a matched nsIPlugin::Shutdown 
call, not break 4.x plugins, and not crash with trying to unload XPCOM ones. Let 
me know what you think.

	sean echevarria
	Comment 11 • 24 years ago

That looks better.

The underlying cause of this bug is that in
nsPluginInstanceOwner::~nsPluginInstanceOwner the Release() is called on the
mInstance pointer after the plugin could have been unloaded.  

As previously noted in bug 76936, calling Release before StopPluginInstance
(rather than after) would fix the problem but is an ugly pattern to promote (it
works because we know the implementation of the plugin host maintains a
reference to the instance which prevents the instance from being deleted prior
to the StopPluginInstance call).  

I previously suggested that mInstance should not be strong-ref'd to avoid this
problem.  Another solution would be to move the Release call from
nsPluginInstanceOwner::~nsPluginInstanceOwner to
nsPluginHostImpl::StopPluginInstance - then the logic of releasing the instance
prior to the nsIPlugin Shutdown/Release/Unload sequence would be localized to
the plugin manager and the strong-ref would be maintained (with the
understanding that StopPluginInstance calls would release the instance).

If we wanted consistency of host managed Release with the AddRef end, then the
initial Addref (when nsPluginHostImpl calls SetInstance) and final Release (when
the instance owner calls StopPluginInstance) could be made by the plugin host on
behalf of the nsPluginInstanceOwner (in nsObjectFrame.cpp) and the
pluginInstanceOwner (in pluginInstanceOwner.cpp).  In order to preserve the
strong ref, we change the  behavior to be more along the lines of the
getter_addrefs template patterns (I don't think there's anything in Mozilla
similar to someoneElse_releases though).

Of course, those changes have the possibilty that the ~nsObjectFrame crash is
fixed in these cases only to expose some other object calling Release on an
unloaded plugin instance.

	sean echevarria
	Comment 12 • 24 years ago

The patch probably needs this line after the Release call:
pluginTag->mEntryPoint = nsnull;

	Brendan Eich [:brendan]
	Comment 14 • 24 years ago

Don't comment out dead code, just delete it -- CVS will remember.

/be

	karnaze (gone)
	Comment 15 • 24 years ago

r=karnaze

	Marc Attinasi
	Comment 16 • 24 years ago

Like be said, just delete the commented-out block of code, and sr=attinasi

	Peter Lubczynski Assignee
	Comment 18 • 24 years ago

Patch checked in. Marking FIXED.

Checking in nsPluginHostImpl.cpp;
/cvsroot/mozilla/modules/plugin/nglsrc/nsPluginHostImpl.cpp,v  <--  
nsPluginHostImpl.cpp
new revision: 1.253; previous revision: 1.252

	shrirang khanzode Reporter
	Comment 19 • 24 years ago

verif on trunk 0522 windows. Plugin plays and no crash is seen.

	Keyser Sose
	Comment 20 • 24 years ago

*** Bug 81587 has been marked as a duplicate of this bug. ***