532700 - nsNPAPIPluginInstance can outlive its nsNPAPIPlugin [@DoStopPlugin]

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Plug-ins, defect)

Description

[Benjamin Smedberg]

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.

Comment 1

[Benjamin Smedberg]

Attachment: Make nsNPAPIPluginInstance hold its parent nsNPAPIPlugin alive, rev. 1

Comment 2

[Josh Aas]

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?

Comment 3

[Benjamin Smedberg]

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.

Comment 4

[Josh Aas]

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 5

[Josh Aas]

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.

Comment 6

[Benjamin Smedberg]

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.

Comment 7

[Benjamin Smedberg]

Attachment: Un-do the crash notification to follow the "normal" NPAPI path, rev. 1

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

Comment 8

[Benjamin Smedberg]

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.

Comment 9

[Josh Aas]

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.

Comment 10

[Marcia Knous [:marcia]]

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?

Comment 11

[Marcia Knous [:marcia]]

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.

Comment 12

[chris hofmann]

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

Comment 15

[Benjamin Smedberg]

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