79872 - crash clicking on realplayer movie link

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[shrirang khanzode]

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)

Comment 1

[Peter Lubczynski]

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.

Comment 2

[Peter Lubczynski]

Attachment: patch #1

Comment 3

[sean echevarria]

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

Comment 4

[Peter Lubczynski]

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.

Comment 5

[sean echevarria]

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?

Comment 6

[Peter Lubczynski]

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

Comment 7

[sean echevarria]

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

Comment 8

[Peter Lubczynski]

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?

Comment 9

[Peter Lubczynski]

Attachment: new patch to fix XPCOM unloading

Comment 10

[Peter Lubczynski]

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.

Comment 11

[sean echevarria]

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.

Comment 12

[sean echevarria]

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

Comment 13

[Peter Lubczynski]

Attachment: better, cleaner, simpler patch to fix crash

Comment 14

[Brendan Eich [:brendan]]

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

/be

Comment 15

[karnaze (gone)]

r=karnaze

Comment 16

[Marc Attinasi]

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

Comment 17

[Peter Lubczynski]

Attachment: final patch (cleaned up)

Comment 18

[Peter Lubczynski]

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

Comment 19

[shrirang khanzode]

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

Comment 20

[Keyser Sose]

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