If javascript in a page causes an exception to be thrown and the plugin host was referenced, the plugin host gets leaked and fails to shutdown and release the nsIPlugins that it has addref'd but not yet released. Will attach testcase. This causes the Beatnik Player plugin to crash at shutdown.

There's a line in the nsPluginHostImpl ctor: obsService->AddObserver(this, NS_LITERAL_STRING("quit-application").get()); seems like that should address the leak, but I'm not seeing nsPluginHostImpl::Observe getting called. Did the "quit-application" topic string change recently?

FYI: just filed bug 73495. I'm getting a leak of the plugin host whenever I do document.plugins.length in javascript now (pretty sure I wasn't last week, so I think it's a new, separate bug).

The main problem here is that now that host is holding references to a plugin module's nsIPlugin (because of bug 45009), any time the host is not properly shutdown we have the potential for a crash in third party plugins. The patch for 45009 made the host an observer of the "quit-application" topic. I have not seen the topic ever fired. Will attach patch that makes the host an observer of the "xpcom-shutdown" topic.

The patch also ensures better cleanup of resources even if the host itself is never destroyed. Moved cleanup code from the host dtor into it's Destroy method which is already protected against multiple calls.

The attached patch might also address bug 65661. Andrei, can you please review?

Moving clean up code to Destroy method is right thing to do. Is observer reliably called on xpcom shutdown? I assume you have tested it and looks like it does address the issue. ar=av

xpcom-shutdown is called very reliably and I've tested the patch with Acrobat, Flash and Beatnik. cc'ing Chris. Can you please sr?

I should note that the patch doesn't actually address the leak of the plugin host, but it addresses the bad things that happen to plugins that occur when the host is leaked.

adding mozilla0.9, beatnik keywords severity -> critical

The patch looks fine - sr=attinasi One question though (not something you changed, but I'm curious): why is mPlugins set to mPlugins->next, previously in the destructor and now in the callback? Also, do you need to remove the observer that you added to the observer service? It doesn't look like it was being removed before but I thought I'd ask.

>One question though (not something you changed, but I'm curious): why is >mPlugins set to mPlugins->next, previously in the destructor and now in the >callback? This is a linked list demolition going from the head to the tail. You are talking about while cycle, right?

yup - I see now, my mistake - sorry.

>Also, do you need to remove the observer that you added to the >observer service? It doesn't look like it was being removed before Marc, you're question prompted me to revisit the Observe callback. I tried removing the observer but it appears that doing so screws up the notifications for other observers of the same topic. I've attached an updated patch with my attempt at removing the observer (but commented out with a reason). I also conditionalized the Destroy action so that any topics that might get added in the future don't cause problems. Could you take another gander at this patch?

ugh: you're -> your

OK - final patch hopefully. Moved the RemoveObserver calls to the host dtor.

Ah, nice. So I wonder if not removing the observer was causing some kind of leak before. (my previous sr still applies - thanks for checking sean).

I think nsObserverList holds a weak reference to observers. If it were a strong ref, then the host dtor wouldn't get called since there'd be a circular dependency - unless the observerList automatically releases the observers it contains, in which case failure to call RemoveObserver wouldn't be cause for a leak anyway. Thanks for the comments and sr.

changing summary to reflect what was actually fixed (old summary: plugin host leaked if js exception thrown) status -> fixed Checking in nsPluginHostImpl.cpp; /cvsroot/mozilla/modules/plugin/nglsrc/nsPluginHostImpl.cpp,v <-- nsPluginHost Impl.cpp new revision: 1.218; previous revision: 1.217 done

crap - my patch broke the Sun build. Commented out the offending lines: http://bonsai.mozilla.org/cvsquery.cgi? module=MozillaTinderboxAll&branch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=98 6007600&maxdate=986008320&who=sean%25beatnik.com Should this: if (NS_LITERAL_STRING(NS_XPCOM_SHUTDOWN_OBSERVER_ID) == aTopic || NS_LITERAL_STRING("quit-application") == aTopic) be this: if (NS_LITERAL_STRING(NS_XPCOM_SHUTDOWN_OBSERVER_ID).get() == aTopic || NS_LITERAL_STRING("quit-application").get() == aTopic) I don't have a Sun machine at my disposal to test this. Any advice?

No, use .Equals instead of ==, so: if (NS_LITERAL_STRING(NS_XPCOM_SHUTDOWN_OBSERVER_ID).Equals(aTopic) || NS_LITERAL_STRING("quit-application").Equals(aTopic))

jag, what sean did should work IMO. scc, please comment.

I sent mail to Scott Friday night and got the following response: >>> You do know you are comparing two pointers now? It seems unlikely those two pointers would ever be the same pointer. You probably want to say something like NS_LITERAL_STRING("foo").Equals(aTopic) // or NS_LITERAL_STRING("foo") == nsLiteralString(aTopic) ...so that you compare the _contents_ of two strings. Right now, you're just testing to see if two pointers both point to the same location in memory. A lot of people make this mistake. I would like to make it obvious to people that comparing two pointers does not compare strings. Can you tell me what gave you that impression so that I can figure out how to better educate people not to do this? By the way, it's not that I don't _want_ to make this compare two strings; it's that in C++, you can't override operations for built-in types. And pointers are built-in types. So I can't make |operator==(const PRUnichar*, const PRUnichar*)| do anything different than it already does, which is the same thing it does for any other pointer. <<< aTopic is const PRUnichar *. I thought I was using operator == defined in template class basic_nsAReadableString but didn't notice that it is only defined to compare to another basic_nsAReadableString. It's not that I thought I was comparing 2 pointers but that I was invoking the class nsLiteralString operator == which does a string compare.

So after wondering what was really happening with my original patch all weekend I find that it does do a string compare and not a pointer compare on win32 - it does the right thing. Although it's not actually invoking nsLiteralString operator ==. It works thanks to this macro in nsAReadableString.h: NS_DEF_TEMPLATE_STRING_COMPARISON_OPERATORS(basic_nsAReadableString<CharT>, CharT) which eventually routes operator == calls to: template <class CharT> int Compare( const basic_nsAReadableString<CharT>& lhs, const basic_nsAReadableString<CharT>& rhs ) Here's the Sun error log: nsPluginHostImpl.cpp: In method `unsigned int nsPluginHostImpl::Observe(class nsISupports *, const short unsigned int *, const short unsigned int *)': nsPluginHostImpl.cpp:4330: no match for `operator ==(class NS_ConvertASCIItoUCS2, const short unsigned int *)' nsPluginHostImpl.cpp:4331: no match for `operator ==(class NS_ConvertASCIItoUCS2, const short unsigned int *)' It looks like the reason the patch broke on Sun is that: 1. HAVE_CPP_2BYTE_WCHAR_T is not defined 2. there are no NS_DEF_TEMPLATE_STRING_COMPARISON_OPERATORS macros for NS_ConvertASCIItoUCS2. Scott, does this make sense?

Moot point. The macro NS_DEF_TEMPLATE_STRING_COMPARISON_OPERATORS no longer exists and the original patch no longer compiles on Win32 or Mac. Attaching patch correction. Does the previous sr apply to this correction?

Sean, has it got in?

No - I think I need an r and sr on the last patch that fixes the string comparison problem: http://bugzilla.mozilla.org/showattachment.cgi?attach_id=29626.

sr=attinas for last patch (id=29626)

According to scc, something like: NS_LITERAL_STRING(SOME_DEFINED_STRING) is not guaranteed to compile because it depends on the order the preprocessor processes the macros: on platforms where NS_LITERAL_STRING resolves to nsLocalString(L..., ...) you could end up with nsLocalString(LSOME_DEFINED_STRING, ...) -> unknown LSOME_DEFINED_STRING, instead of NS_LITERAL_STRING("some string") -> nsLocalString(L"some string", ...). I suggest you use NS_LITERAL_STRING("xpcom-shutdown") instead.

updated patch per Jag's comment but rather than NS_LITERAL_STRING("xpcom- shutdown") I assigned NS_XPCOM_SHUTDOWN_OBSERVER_ID to an nsAutoString.

Jag: can you please review? http://bugzilla.mozilla.org/showattachment.cgi?attach_id=31349

I knew I should've left in the "or if you want to keep NS_XPCOM_SHUTDOWN_OBSERVER_ID, use NS_ConvertASCIItoUCS2(NS_XPCOM_SHUTDOWN_OBSERVER_ID)." This can be used as a drop-in replacement (it's what NS_LITERAL_STRING defines to on platforms which don't support compile time conversion to 16-bit string literals). So: - obsService->AddObserver(this, NS_LITERAL_STRING(NS_XPCOM_SHUTDOWN_OBSERVER_ID).get()); + obsService->AddObserver(this, NS_ConvertASCIItoUCS2(NS_XPCOM_SHUTDOWN_OBSERVER_ID).get()); - obsService->RemoveObserver(this, NS_LITERAL_STRING(NS_XPCOM_SHUTDOWN_OBSERVER_ID).get()); + obsService->RemoveObserver(this, NS_ConvertASCIItoUCS2(NS_XPCOM_SHUTDOWN_OBSERVER_ID).get()); -// if (NS_LITERAL_STRING(NS_XPCOM_SHUTDOWN_OBSERVER_ID) == aTopic || -// NS_LITERAL_STRING("quit-application") == aTopic) + if (NS_ConvertASCIItoUCS2(NS_XPCOM_SHUTDOWN_OBSERVER_ID).Equals(aTopic) || + NS_LITERAL_STRING("quit-application").Equals(aTopic))

One macro or another... I think it might be better to keep the NS_XPCOM_SHUTDOWN_OBSERVER_ID macro in case the string ever changes (if we used a string literal, it would compile but stop working). Jag, one more time?

r=jag

Still looks OK to me - thanks sean - sr=attinasi

string changes finally checked in: Checking in nsPluginHostImpl.cpp; /cvsroot/mozilla/modules/plugin/nglsrc/nsPluginHostImpl.cpp,v <-- nsPluginHostImpl.cpp new revision: 1.234; previous revision: 1.233 done Thanks everyone!

verif(stamp)