158670 - Real crashes if scripted after being hidden [@ NPPL3260.DLL] [@ nppl3260.dll]

Status: RESOLVED WORKSFORME

(Plugins Graveyard :: RealPlayer (Real), defect, P2)

Description

[Peter Lubczynski]

We crash when trying to script a plugin that has gone away. This may be related
to bug 157631 and bug 148889.

Steps to reproduce:
1. change the CSS display type of Real's embed tag to 'none'
2. attempt to call a scripting method

See attached testcase. Click the "Display:none" button then the "SetSource"
button. Here is the stack:
NPPL3260! 6155a6a3()
XPTC_InvokeByIndex(nsISupports * 0x03681704, unsigned int 3, unsigned int 2,
nsXPTCVariant * 0x0012db04) line 106
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode
CALL_METHOD) line 2026 + 42 bytes
XPC_WN_CallMethod(JSContext * 0x02c36bd0, JSObject * 0x02babd58, unsigned int 1,
long * 0x02e8a9d0, long * 0x0012ddb4) line 1266 + 14 bytes
js_Invoke(JSContext * 0x02c36bd0, unsigned int 1, unsigned int 0) line 788 + 23
bytes
js_Interpret(JSContext * 0x02c36bd0, long * 0x0012e6d4) line 2743 + 15 bytes
js_Invoke(JSContext * 0x02c36bd0, unsigned int 1, unsigned int 2) line 805 + 13
bytes
js_InternalInvoke(JSContext * 0x02c36bd0, JSObject * 0x02c5a9b0, long 46508472,
unsigned int 0, unsigned int 1, long * 0x0012e934, long * 0x0012e804) line 880 +
20 bytes
JS_CallFunctionValue(JSContext * 0x02c36bd0, JSObject * 0x02c5a9b0, long
46508472, unsigned int 1, long * 0x0012e934, long * 0x0012e804) line 3428 + 31 bytes
nsJSContext::CallEventHandler(nsJSContext * const 0x02c36a38, void * 0x02c5a9b0,
void * 0x02c5a9b8, unsigned int 1, void * 0x0012e934, int * 0x0012e938, int 0)
line 1016 + 33 bytes
nsJSEventListener::HandleEvent(nsJSEventListener * const 0x02eaee88, nsIDOMEvent
* 0x02dc1520) line 180 + 77 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x02eaef48,
nsIDOMEvent * 0x02dc1520, nsIDOMEventTarget * 0x02e7e648, unsigned int 4,
unsigned int 7) line 1219 + 20 bytes
nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x02eaee20,
nsIPresContext * 0x02c10b28, nsEvent * 0x0012f348, nsIDOMEvent * * 0x0012f154,
nsIDOMEventTarget * 0x02e7e648, unsigned int 7, nsEventStatus * 0x0012f67c) line
1394 + 36 bytes
nsGenericElement::HandleDOMEvent(nsGenericElement * const 0x02e8a798,
nsIPresContext * 0x02c10b28, nsEvent * 0x0012f348, nsIDOMEvent * * 0x0012f154,
unsigned int 1, nsEventStatus * 0x0012f67c) line 1667
nsHTMLButtonElement::HandleDOMEvent(nsHTMLButtonElement * const 0x02e8a798,
nsIPresContext * 0x02c10b28, nsEvent * 0x0012f348, nsIDOMEvent * * 0x00000000,
unsigned int 1, nsEventStatus * 0x0012f67c) line 475 + 29 bytes
PresShell::HandleEventInternal(nsEvent * 0x0012f348, nsIView * 0x00000000,
unsigned int 1, nsEventStatus * 0x0012f67c) line 6280 + 44 bytes
PresShell::HandleEventWithTarget(PresShell * const 0x02e88770, nsEvent *
0x0012f348, nsIFrame * 0x02eab9b4, nsIContent * 0x02e8a798, unsigned int 1,
nsEventStatus * 0x0012f67c) line 6249 + 22 bytes
nsEventStateManager::CheckForAndDispatchClick(nsEventStateManager * const
0x02e904d8, nsIPresContext * 0x02c10b28, nsMouseEvent * 0x0012f880,
nsEventStatus * 0x0012f67c) line 2644 + 63 bytes
nsEventStateManager::PostHandleEvent(nsEventStateManager * const 0x02e904e0,
nsIPresContext * 0x02c10b28, nsEvent * 0x0012f880, nsIFrame * 0x02eab9b4,
nsEventStatus * 0x0012f67c, nsIView * 0x02eaa818) line 1722 + 28 bytes
PresShell::HandleEventInternal(nsEvent * 0x0012f880, nsIView * 0x02eaa818,
unsigned int 1, nsEventStatus * 0x0012f67c) line 6300 + 43 bytes
PresShell::HandleEvent(PresShell * const 0x02e88774, nsIView * 0x02eaa818,
nsGUIEvent * 0x0012f880, nsEventStatus * 0x0012f67c, int 0, int & 1) line 6203 +
25 bytes
nsViewManager::HandleEvent(nsView * 0x02eaa598, nsGUIEvent * 0x0012f880, int 0)
line 2086
nsView::HandleEvent(nsViewManager * 0x02c87328, nsGUIEvent * 0x0012f880, int 0)
line 306
nsViewManager::DispatchEvent(nsViewManager * const 0x02c87328, nsGUIEvent *
0x0012f880, nsEventStatus * 0x0012f780) line 1891 + 23 bytes
HandleEvent(nsGUIEvent * 0x0012f880) line 83
nsWindow::DispatchEvent(nsWindow * const 0x02eaa654, nsGUIEvent * 0x0012f880,
nsEventStatus & nsEventStatus_eIgnore) line 1029 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f880) line 1050
nsWindow::DispatchMouseEvent(unsigned int 301, unsigned int 0, nsPoint *
0x00000000) line 4937 + 21 bytes
ChildWindow::DispatchMouseEvent(unsigned int 301, unsigned int 0, nsPoint *
0x00000000) line 5194
nsWindow::ProcessMessage(unsigned int 514, unsigned int 0, long 2753069, long *
0x0012fca4) line 3787 + 28 bytes
nsWindow::WindowProc(HWND__ * 0x004f0226, unsigned int 514, unsigned int 0, long
2753069) line 1294 + 27 bytes

Comment 1

[Peter Lubczynski]

Attachment: testcase (click display:none, then SetSource)

Comment 2

[Johnny Stenback (:jst)]

Is the plugin being unloaded here? Is that the reason for the crash? The DOM CI
code that wraps plugins when you're scripting them does tell the plugin host
that this plugin is being scriped (by calling
nsPIPluginHost::SetIsScriptableInstance()), that should make the plugin not
unload too early. Is that code broken by any chance?

Comment 3

[av (gone)]

This plugin is XPCOM and it does not need to be specifically scriptable for the
plugin code to treat its unloading business in a special way. We don't unload
XPCOM plugin dlls and in this particular case it is not unloaded. But on
Display:None click the object frame gets destroyed and we subsequently destroy
the plugin instance, which is probably expected to be alive when DOM calls it.

Peter, when I tried to skip destroying instance in |nsObjectFrame::Destroy| code
-- as if the plugin returned |doCache == true| -- no crash. Should we add yet
another case to our destruction/unloading logic? Like always cache plugin
instances for plugins who have been scripted? This can be done in the plugin
itself too. They need to set cache flag if scripting has been detected.

Comment 4

[Peter Lubczynski]

We are not unloading their library here nor calling NP_Shutdown.

ahh yes, Andrei, I think it's the nsIPluginInstance::Destroy call from
nsObjectFrame::Destory which cleans up something in their code and causes us to
crash later. If nsPluginInstanceVariable_DoCacheBool is true, we only call
|Stop| not |Destory|. Perhaps we could add a check for 'scriptability' here?

Comment 5

[serge (gone)]

hmm, reincarnation of bug 148889,
BTW: I forgot why we decided do not use SetCached(PR_TRUE) for all scriptable
plugins: patch http://bugzilla.mozilla.org/attachment.cgi?id=87323&action=view

Comment 6

[rubydoo123]

handing this one over to Andrei

Comment 7

[av (gone)]

serge: if I remember correctly, that bug was about keeping dll in memory, not
caching plugin instance. Dll was the cause of the problem. Another thing is that
although caching plugin instance seemed to help too, it will be kicked out of
the cache list with eleventh instance entering the cache, so we still needed
special handling of the dll unload process, which that patch did not provide.

Now, looks like instances are also part of the game. Caching them with our
regular means will help, but the problem of MAX_NUM_CACHED_PLUGINS still
remains. Do you think it would be appropriate to do something similar to what we
do with libraries?

Comment 8

[serge (gone)]

actually I was wrong :( bug 148889 was about scriptable plugin, but real is
XPCOM plugin which implements |NSGetFactory|, but the stack trace looks similar,
well the problem itself is similar - plugin deletes some objs while js still
keeps refs on.
BTW: if you leave that page alone for a while (but you have to serve the net,
because GC should be involve to clean up the garbage) after hitting
"display:none" and than click on SetSource, you will see
"Error: document.getElementById("plug").SetSource is not a function"
in javascript console, and there's no crash.

Comment 9

[av (gone)]

Right, patch you mentioned was in our instance wrapper, we don't have access to
it in XPCOM plugins.

The problem is even more complicated. For NPAPI plugins we know that the plugin
is scriptable when content tries to script it. In this test case we are
destroying the instance _before_ trying to use its scripting capabilities. So at
the time of destroying the object frame we don't really know how we should
handle its destruction. Maybe to querry for scriptable object/interface at early
stages of the instance life, like right after NPP_New e.g., just to learn if it
may exersize scripting or not?

Comment 10

[Peter Lubczynski]

Attachment: cache scriptable plugins

Here's some code I was working on to cache scriptable plugins. It attempts not
to call nsIPluginInstance::Destroy() on the plugin until later but doesn't seem
to work. :(

Comment 11

[av (gone)]

I am trying a similar approach. Looks like not calling |Destroy| is not enough.
I was able to stop crashes skipping |nsIPluginInstance::Stop| too. But this is
likely not acceptable, if we don't call |Stop| it will continue playing.

Comment 12

[av (gone)]

Looks like there is little we can do here. When the button Display:None is
pressed the object frame is destroyed, and we call |nsIPluginInstance::Stop| and
|nsIPluginInstance::Destroy|, but neither we call |nsIPlugin::Shutdown| nor we
unload the library. Looks like Real cleans some of its memory in its |Stop|
method which we cannot skip. I am afraid evangelizm and documentation is the
best solution here: don't screw refcount and delete anything unless you are
about to shutdown.

Comment 13

[rubydoo123]

adding nsbeta1+

Comment 14

[Peter Lubczynski]

Yeah, I think we may need to ask an engineering contact at Real to ensure their
code doesn't have any dangling pointers like in bug 159646.

Comment 15

[rubydoo123]

Will verify the issue with Real folks

Comment 16

[shrirang khanzode]

just fyi: this still happens on 1021 trunk

Comment 17

[rubydoo123]

removing nsbeta1+ marker, this is a Real issue and we need to work through this
with their engineers.

Comment 18

[rubydoo123]

reassign

Comment 19

[Peter Lubczynski]

adding dependancy to bug 90268:
If the plugin's lifetime was moved to content, it would not crash when scripted
if its frame was destroyed with a display: none.

re: comment #8:
as a temporary fix to this problem (and others), would it be possible and help
to force a GC on nsObjectFrame::Destory()?

Comment 20

[Kevin McCluskey (gone)]

nsbeta1-, since it depends on 90268 which is nbeta1- and Peter is overloaded
with higher priority issues. Fixing this involves keeping the plugin around
regardless of whether its frame is destroyed. This will be a large and risky change.

Comment 21

[Jay Patel [:jay]]

Making topcrash- since this seems to be a known problem.  I still see crashes on
recent MozillaTrunk builds...but not enough to make this a major topcrasher.

Comment 22

[Ronald Tilby]

testcase WFM using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060529 Minefield/3.0a1 ID:2006052910

Comment 23

[Samuel Sidler (old account; do not CC)]

WFM using Firefox 2.0.0.6 on Windows Vista. Closing as such, per comment 22 and this comment.

(However, there's another topcrash @ NPPL3260.dll. I'd love to find out which it is, it's just not this one, as reported.)