nsNPAPIPluginInstance references data on nsNPAPIPlugin by pointer (mCallbacks and mLibrary). In cases where plugin instance creation fails (NPP_New returns an error code), the nsNPAPIPlugin is destroyed while existing instances may still live, which leads to memory corruption. This patch makes the nsNPAPIPluginInstance hold a strong reference to the nsNPAPIPlugin. This happens more frequently if a plugin crashes in e10s. I think we should take this on 1.9.2 eventually, because even though it's an edge case it could possibly lead to weird memory-corruption crashes with plugins.

IIRC, it is an invariant that nsNPAPIPluginInstance should should never outlive their associated nsNPAPIPlugin objects. If that is happening then we should fix the underlying cause, not cover up the lifetime issue by adding strong references (certain future pain). This reminds me of this: https://bugzilla.mozilla.org/show_bug.cgi?id=477475#c60 and its resolution: https://bugzilla.mozilla.org/show_bug.cgi?id=499600#c10 The summary on this bug says: "in cases where plugin instance creation fails (NPP_New returns an error code), the nsNPAPIPlugin is destroyed while existing instances may still live" If this is true, why not simply stop destroying the nsNPAPIPlugin when it has outstanding instances when adding an instance fails?

Who maintains this invariant? The patch here just makes it an explicit invariant by actually holding onto the object. Currently the nsPluginTag holds the nsIPlugin alive. However, when NPP_New fails, the nsPluginTag releases the nsIPlugin from the nsPluginTag, and there's nothing else holding it alive. So I believe this is the correct patch, and not just a bandaid.

The nsPluginHost object has a strong-reference list (mPlugins) of nsPluginTag objects, and those nsPluginTag objects hold strong references (mEntryPoint) to their associated nsIPlugin objects. nsIPlugin objects should only need to be retained by their nsPluginTag objects, and an nsPluginTag object should never be removed from the nsPluginHost list if it has an outstanding instance. This is what *should* be true. Where exactly (what lines of code) do you see an nsPluginTag object being removed from the nsPluginHost's mPlugins list when there are outstanding instances? "when NPP_New fails, the nsPluginTag releases the nsIPlugin from the nsPluginTag" Where do you see this?

Comment on attachment 415904 [details] [diff] [review] Make nsNPAPIPluginInstance hold its parent nsNPAPIPlugin alive, rev. 1 Discussed this with bsmedberg, if a plugin because invalid for any reason we need to clean up (get rid of) all of its instances - they should never be holding it alive.

The patch now on bug 532751 mostly avoids this problem by, when a plugin crashes, immediately invalidating the nsPluginInstanceTag which unhooks the object frame. But there's definitely an underlying bug here which I'm going to try to ferret out.

To test this, I "un-did" the crash notification from bug 532751 and then ran the mochitest-ipcplugins tests. The crash occurs in whatever test immediately follows test_crashing.html, and the stack is: #0 0x00007f69f9fa387e in nsNPAPIPluginInstance::Stop (this=0x7f69e527b120) at ../../../../../src/modules/plugin/base/src/nsNPAPIPluginInstance.cpp:1018 #1 0x00007f69f94f3808 in DoStopPlugin (aInstanceOwner=0x7f69e4ffa9d0, aDelayedStop=0) at ../../../src/layout/generic/nsObjectFrame.cpp:2170 #2 0x00007f69f94f3d24 in nsStopPluginRunnable::Run (this=0x7f69e3e80310) at ../../../src/layout/generic/nsObjectFrame.cpp:2219 #3 0x00007f69fa41280c in nsThread::ProcessNextEvent (this=0x7f69f7338700, mayWait=1, result=0x7fff032a88bc) at ../../../src/xpcom/threads/nsThread.cpp:527 #4 0x00007f69fa3a9d6e in NS_ProcessNextEvent_P (thread=0x7f69f7338700, mayWait=1) at nsThreadUtils.cpp:250 #5 0x00007f69fa412c38 in nsThread::Shutdown (this=0x7f69e5147af0) at ../../../src/xpcom/threads/nsThread.cpp:468 #6 0x00007f69fa42d228 in NS_InvokeByIndex_P (that=0x7f69e5147af0, methodIndex=6, paramCount=0, params=0x0) at ../../../../../../../src/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:208 #7 0x00007f69fa41cf75 in nsProxyObjectCallInfo::Run (this=0x7f69e4117a10) at ../../../../src/xpcom/proxy/src/nsProxyEvent.cpp:181 #8 0x00007f69fa41280c in nsThread::ProcessNextEvent (this=0x7f69f7338700, mayWait=0, result=0x7fff032a8b3c) at ../../../src/xpcom/threads/nsThread.cpp:527 #9 0x00007f69fa3a9d6e in NS_ProcessNextEvent_P (thread=0x7f69f7338700, mayWait=0) at nsThreadUtils.cpp:250 mCallbacks->destroy is becoming invalid here: Old value = (NPP_DestroyProcPtr) 0x7f195eba8f44 <mozilla::plugins::PluginModuleParent::NPP_Destroy(_NPP*, _NPSavedData**)> New value = (NPP_DestroyProcPtr) 0 #0 0x000000336ae82dee in memset () from /lib64/libc.so.6 #1 0x00007f195e8ca22b in ~nsNPAPIPlugin (this=0x7f194a4f3160) at ../../../../../src/modules/plugin/base/src/nsNPAPIPlugin.cpp:290 #2 0x00007f195e8c9dcf in nsNPAPIPlugin::Release (this=0x7f194a4f3160) at ../../../../../src/modules/plugin/base/src/nsNPAPIPlugin.cpp:221 #3 0x00007f195e8eb6f3 in nsCOMPtr<nsIPlugin>::assign_assuming_AddRef (this=0x7f194edee770, newPtr=0x0) at ../../../../dist/include/nsCOMPtr.h:495 #4 0x00007f195e8eb72e in nsCOMPtr<nsIPlugin>::assign_with_AddRef (this=0x7f194edee770, rawPtr=0x0) at ../../../../dist/include/nsCOMPtr.h:1181 #5 0x00007f195e8ebbc7 in nsCOMPtr<nsIPlugin>::operator= (this=0x7f194edee770, rhs=0x0) at ../../../../dist/include/nsCOMPtr.h:640 #6 0x00007f195e8e3173 in nsPluginTag::TryUnloadPlugin (this=0x7f194edee700) at ../../../../../src/modules/plugin/base/src/nsPluginHost.cpp:965 #7 0x00007f195e8e96f0 in nsPluginHost::ReloadPlugins (this=0x7f194de09f00, reloadPages=0) at ../../../../../src/modules/plugin/base/src/nsPluginHost.cpp:2570 #8 0x00007f195e8dc55f in nsPluginHost::SetUpPluginInstance (this=0x7f194de09f00, aMimeType=0x7f194a010128 "application/x-test", aURL=0x7f19499655b0, aOwner=0x7f1949b6e300) at ../../../../../src/modules/plugin/base/src/nsPluginHost.cpp:3444 #9 0x00007f195e8e861a in nsPluginHost::InstantiateEmbeddedPlugin (this=0x7f194de09f00, aMimeType=0x7f194a010128 "application/x-test", aURL=0x7f19499655b0, aOwner=0x7f1949b6e300) at ../../../../../src/modules/plugin/base/src/nsPluginHost.cpp:3120 #10 0x00007f195de1a76b in nsObjectFrame::InstantiatePlugin (this=0x7f194a090740, aPluginHost=0x7f194de09f00, aMimeType=0x7f194a010128 "application/x-test", aURI=0x7f19499655b0) at ../../../src/layout/generic/nsObjectFrame.cpp:953

ok, what's happening appears to be: after the first plugin crashes, we reload plugins: there is a nsPluginTag for the first (crashed) plugin process, so we don't call TryToUnload on that tag. We then create a *new* nsPluginTag for the second plugin process. At some point in here we ReloadPlugins again. By this time, the instances of the first (dead) plugin are all stopped, and so nsPluginHost::IsRunningPlugin("application/x-test") returns PR_FALSE, even though there are other plugin instances for that same MIME type which are still alive. I think the invariant being broken here is that there aren't two nsPluginTag for the same MIME type. I'm not sure how that's happening yet. In any case, it's not a blocker any more with the other bug landed.

I think between bug 533030, bug 520639, and bug 532751 we're likely to have fixed this. Resolving FIXED for now, reopen if this returns or if I'm known to be wrong about this.

I hit this crash while testing Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.4) Gecko/20100413 Firefox/3.6.4 (.NET CLR 3.5.30729). https://crash-stats.mozilla.com/report/index/bp-370d75cb-1443-4bbb-a1df-99fcd2100414. I was playing around with http://flashcrash.dempsky.org/ when it occurred. Should I open a new bug since this occurred using 1.9.2?

Reopening as I see another crash in crash-stats today with this stack - https://crash-stats.mozilla.com/report/index/33bd8907-5f8d-4365-b2a5-b23592100418.

yeah, possible volume regression on 3.6.4 checking --- DoStopPlugin 20100421-crashdata.csv found in: 3.6.4 3.6.3 3.6b5 3.7a4 3.6b2 3.6.3plugin1 3.6 3.5.9 release total-crashes DoStopPlugin crashes pct. all 383147 24 6.26391e-05 3.6.4 16781 10 0.000595912 3.6.3 257898 6 2.3265e-05 3.6b5 840 3 0.00357143 3.7a4 849 1 0.00117786 3.6b2 553 1 0.00180832 3.6.3plugin1 397 1 0.00251889 3.6 17989 1 5.55895e-05 3.5.9 34290 1 2.9163e-05 os breakdown DoStopPluginTotal 24 Win5.1 0.42 Win6.0 0.42 Win6.1 0.17 https://kos.cvut.cz/kos/logout.do?page=f870936bee0c4367a66336a74dd7744 http://www.sdeurope.org/ http://onepieceofficial.com/videos.aspx http://atdhe.net/17854/watch-bayern-munchen-vs-olympique-lyon http://1000webgames.com/7390-Monster-Racers.html http://get.adobe.com/es/flashplayer/ flash versions around at time of crash 10 10.0.45.2 7 [blank] 3 10.0.22.87 2 10.0.42.34 1 10.1.53.7 1 10.0.32.18

Resolving old bugs which are likely not relevant any more, since NPAPI plugins are deprecated.