The checkin for bug 422926 caused a regression which can cause multiple plugin instances to run for a single <object> and leave those instances running after the page has closed. The first time the embedding page is loaded, the plugin loads and runs fine. As soon as the user navigates away from the embedding page, or as soon as they reload the page, we get a plugin instances being created when it shouldn't be, and instances can leak and will continue to run in the background until the browser is shut down. I've not figured out the details just yet, but I'll post more shortly. There is no such problem in Firefox 2 where everything works fine.

In the case of navigating away from the embedding page, here's what happens: Under nsObjectFrame::StopPlugin() the plugin instance's destructor is called, under which there is a check for a JS property called 'destroyed' on the plugin's JS object. Unfortunately, when it checks for this property, Mozilla goes and instantiates a new plugin instance. The stack looks something like this: <plugin ctor> ... ns4xPluginInstance::SetWindow() nsPluginNativeWindow::CallSetWindow() nsPluginNativeWindowWin::CallSetWindow() nsPluginHostImpl::InstantiateEmbeddedPlugin() nsObjectFrame::InstantiatePlugin() nsObjectFrame::Instantiate() nsObjectLoadingContent::Instantiate() nsObjectLoadingContent::EnsureInstantiation() nsHTMLPluginObjElementSH::GetPluginInstanceIfSafe() nsHTMLPluginObjElementSH::NewResolve() XPC_WN_Helper_NewResolve() js_LookupPropertyWithFlags() js_LookupProperty() LookupUCProperty() JS_HasUCProperty() nsJSObjWrapper::NP_HasProperty() _hasmethod() ... <plugin dtor, which checks for a property destroyed on the NPP> ... ns4xPluginInstance::Stop() DoStopPlugin() nsObjectFrame::StopPluginInternal() nsObjectFrame::StopPlugin() StopPluginInstance() PresShell::EnumeratePlugins() PresShell::Freeze() The reload issue seems a little more complex, but I'd guess it's the same fundamental issue. I'll describe it once I'm clearer on what's going on.

I should have said, the new plugin instance that gets created under the plugin dtor when you navigate away from the embedding page plays in the background until you close the browser.

jwatt, have a testcase? a site exhibiting the behavior?

This might be a regression from bug 401155

Damon: I'm afraid not at this point. I'm going to try and create a simple plugin that we can share with you. Ben: I can't access bug 401155 so I can't comment. I can say that the behavior first manifested itself after the checkin for bug 422926. I pulled by date and spun builds immediately before and after to check.

Here's what's going on when the page is reloaded: when the embedding <object> element is processed it posts an nsAsyncInstantiateEvent event to the event loop. After this has happened a <script> element is processed and its script is run. This script does two things of interest: it gets a reference to the <object> element, and it posts a JS timout that will access the <object> element when called. When the script gets the reference to the <object> element it causes nsObjectLoadingContent::EnsureInstantiation to be called and so nsObjectFrame's mInstanceOwner member gets assigned an nsPluginInstanceOwner instance. It's when we return to the event loop and process the nsObjectLoadingContent event that things start to go wrong. In nsObjectFrame::PrepareInstanceOwner we first call StopPluginInternal to destroy the "old" plugin created when the script accessed the plugin during parsing: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsObjectFrame.cpp&rev=1.652&mark=1618,1626#1611 Under the plugin's teardown code the event loop is re-spun. When this happens we end up re-entering nsObjectFrame::PrepareInstanceOwner either due to the JS timeout being processed, or due to the JS property check mentioned in comment 1. The inner PrepareInstanceOwner call assignes a new nsPluginInstanceOwner instance to the nsObjectFrame's mInstanceOwner member. It is this nsPluginInstanceOwner instance that leaks, and it leaks because when we end up back in the outer nsObjectFrame::PrepareInstanceOwner call, mInstanceOwner is immediately overwritten. As a result, nsPluginInstanceOwner::Destroy() is never called on this nsPluginInstanceOwner instance, and the RemoveEventListener calls in Destroy() never happen. Here's a stack showing the PrepareInstanceOwner rentrance where the JS timeout is being processed under the plugin teardown code: nsEventListenerManager::AddEventListener() nsEventListenerManager::AddEventListenerByIID() nsGenericElement::AddEventListenerByIID() nsPluginInstanceOwner::Init() > nsObjectFrame::PrepareInstanceOwner() nsObjectFrame::Instantiate() nsObjectLoadingContent::Instantiate() nsObjectLoadingContent::EnsureInstantiation() nsHTMLPluginObjElementSH::GetPluginInstanceIfSafe() nsHTMLPluginObjElementSH::NewResolve() XPC_WN_Helper_NewResolve() js_LookupPropertyWithFlags() js_GetPropertyHelper() js_Interpret() js_Invoke() js_InternalInvoke() JS_CallFunctionValue() nsJSContext::CallEventHandler() nsGlobalWindow::RunTimeout() nsGlobalWindow::TimerCallback() nsTimerImpl::Fire() nsTimerEvent::Run() nsThread::ProcessNextEvent() NS_ProcessPendingEvents_P() nsBaseAppShell::NativeEventCallback() nsAppShell::EventWindowProc() _InternalCallWinProc@20() _UserCallWinProcCheckWow@32() _DispatchMessageWorker@8() _DispatchMessageW@4() ... <plugin instance dtor under which we end up spinning the event loop> ... NPP_Destroy() ns4xPluginInstance::Stop() DoStopPlugin() nsObjectFrame::StopPluginInternal() > nsObjectFrame::PrepareInstanceOwner() nsObjectFrame::Instantiate() nsObjectLoadingContent::Instantiate() nsAsyncInstantiateEvent::Run() nsThread::ProcessNextEvent() NS_ProcessNextEvent_P() nsBaseAppShell::Run() nsAppStartup::Run() The leaking of the nsPluginInstanceOwner takes an absolute ton of other objects with it, although I'm a bit less worried about that than the fact that you get multiple instances of the plugin all sending different audio to the sound card. :-)

A testcase that shows this would be wonderful. Jonathan, is there a testcase somewhere that I can get my hands on?

So, do we not have enough information to pursue a fix?

Poke. Any more info here? If we don't have a way to reproduce, it's going to fall off the blocker list.

I have a half-baked patch for this that may fix it, but I'd love to be able to test it before claiming that it really does fix this problem. Jonathan, any chance you can provide steps to reproduce here, or if not, can you test a patch for me once I have one ready?

Jonathan, please test this if you can't provide a testcase for this problem.

Jonathan do you have a chance to test this out? We are closing out for RC1 this week so it would be great to know one way or the other... Mike

i could help too, but i would need some steps to reproduce or a testcase

jst: your patch fixes the symptoms for me, so I don't get multiple instances of the plugin running for a single <object> any more. However, I'm still seeing re-entrance into nsObjectLoadingContent::Instantiate when the page embedding the plugin is reloaded. When the page is reloaded, nsObjectLoadingContent::Instantiate is called when the <script> tag that follows the <object> tag is parsed and has it's contents synchronously evaluated (the JS accesses the <object>). Later, when the <object> is reflowed nsObjectLoadingContent::HasNewFrame posts an nsAsyncInstantiateEvent to the event loop. When this event is run, the instance of the plugin created during parsing of the <script> tag is destroyed, under that the plugin spins the event loop, the JS timeout is evaluated and accesses the <object>, and we re-enter nsObjectLoadingContent::Instantiate. The stack looks like this: nsObjectLoadingContent::Instantiate() nsObjectLoadingContent::EnsureInstantiation() nsHTMLPluginObjElementSH::GetPluginInstanceIfSafe() nsHTMLPluginObjElementSH::NewResolve() XPC_WN_Helper_NewResolve() js_LookupPropertyWithFlags() js_GetPropertyHelper() js_Interpret() js_Invoke() js_InternalInvoke() JS_CallFunctionValue() nsJSContext::CallEventHandler() nsGlobalWindow::RunTimeout() nsGlobalWindow::TimerCallback() nsTimerImpl::Fire() nsTimerEvent::Run() nsThread::ProcessNextEvent() NS_ProcessPendingEvents_P() nsBaseAppShell::NativeEventCallback() nsAppShell::EventWindowProc() ... <plugin dtor under which the event loop is spun> ... ns4xPluginInstance::Stop() DoStopPlugin() nsObjectFrame::StopPluginInternal() nsObjectFrame::PrepareInstanceOwner() nsObjectFrame::Instantiate() nsObjectLoadingContent::Instantiate() nsAsyncInstantiateEvent::Run() Also, here are some comments on the patch itself: >@@ -1656,10 +1652,10 @@ nsObjectFrame::Instantiate(nsIChannel* aChannel, nsIStreamListener** aStreamList > // This must be done before instantiating the plugin > FixupWindow(mRect.Size()); > >- NS_ASSERTION(!mInstantiating, "Say what?"); >- mInstantiating = PR_TRUE; >+ NS_ASSERTION(!mInstantiationPrevented, "Say what?"); >+ mInstantiationPrevented = PR_TRUE; > rv = pluginHost->InstantiatePluginForChannel(aChannel, mInstanceOwner, aStreamListener); >- mInstantiating = PR_FALSE; >+ mInstantiationPrevented = PR_FALSE; I'd think an assertion after the InstantiatePluginForChannel call asserting that |mInstantiationPrevented = PR_TRUE| would be just as valuable, if not more so, than the assertion before. >@@ -1691,6 +1687,8 @@ nsObjectFrame::Instantiate(const char* aMimeType, nsIURI* aURI) > return rv; > mInstanceOwner->SetPluginHost(pluginHost); > >+ mInstantiationPrevented = PR_TRUE; >+ > rv = InstantiatePlugin(pluginHost, aMimeType, aURI); > > // finish up >@@ -1699,6 +1697,8 @@ nsObjectFrame::Instantiate(const char* aMimeType, nsIURI* aURI) > CallSetWindow(); > } Perhaps assert |mInstantiationPrevented == PR_TRUE| here? >+ mInstantiationPrevented = PR_FALSE; >+ >@@ -1940,8 +1940,13 @@ nsObjectFrame::StopPluginInternal(PRBool aDelayedStop) > // touch it! > owner->PrepareToStop(aDelayedStop); > >+ PRBool oldVal = mInstantiationPrevented; >+ mInstantiationPrevented = PR_TRUE; >+ > DoStopPlugin(owner, aDelayedStop); You can't do that according to the comment just above which says |this| could have been deleted. If it wasn't for that, I'd suggest asserting |mInstantiationPrevented == PR_TRUE| here too. :-) >+ mInstantiationPrevented = oldVal; >+ >- PRBool mInstantiating; >+ PRBool mInstantiationPrevented; I think the code is easier to follow with the name mInstantiating. When I first saw mInstantiationPrevented I thought it was a past-tense thing, and meant that initialization had been prevented somewhere and recovery was needed or something. If you want to change the name, perhaps mPreventInitialization would be a better name?

Err, one stack deeper and the re-entrance hits a mInstantiationPrevented check and returns: nsObjectFrame::Instantiate() nsObjectLoadingContent::Instantiate() nsObjectLoadingContent::EnsureInstantiation()

This fixes the issues jwatt pointed out in his feedback. jwatt, would you mind re-testing and reviewing?

Comment on attachment 319472 [details] [diff] [review] Possible fix, updated. How come the code in nsObjectFrame::InstantiatePlugin can go away? sr=bzbarsky with that explained.

(In reply to comment #17) > (From update of attachment 319472 [details] [diff] [review]) > How come the code in nsObjectFrame::InstantiatePlugin can go away? Because the only caller (nsObjectFrame::Instantiate(), the one taking a mimetype as an argument) sets the mPreventInstantiation member now. I could leave an assert to that effect in InstantiatePlugin() if you want me to. Thanks for looking! Oh, and this apparently fixes bug 405357 as well.

Yeah, if we can assert that, that would be good.

Comment on attachment 319472 [details] [diff] [review] Possible fix, updated. It works great, and looks good. r=jwatt, with bz's assertion.

This fixes all the memory leaks I was seeing too.

Comment on attachment 319472 [details] [diff] [review] Possible fix, updated. a1.9=beltzner

Fix checked in.

Excellent. Thanks jst!

Can we open this? trunk-only bug, now fixed?

(Open meaning "make public".)

Nope, can't do it yet. Get in touch with me for details if you want.

Okay, we're good to open this now Mike.

Great, thanks.