Closed Bug 521377 Opened 15 years ago Closed 15 years ago

Plugins: NPRuntime: Segfault when NPP_GetValue_NPPVpluginScriptableNPObject returns a null actor

Categories

(Core :: IPC, defect)

Product:
Core
Component:
IPC
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.3a1
Tracking Flags:
Tracking Status
status1.9.2 --- .4-fixed

People

(Reporter: cjones, Assigned: bent.mozilla)

References

Details

(Whiteboard: [fixed-lorentz])

Attachments

(5 files, 5 obsolete files)

Interdiff between v3 and v4
71.08 KB, patch
Details | Diff | Splinter Review
valgrind log from v4
14.11 KB, text/plain
Details
Patch, v4, merged
138.88 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
Use inlines, rev. 1
3.92 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
PluginScriptableObjectParent::ActorDestroy, rev. 1
2.78 KB, patch
Details | Diff | Splinter Review 
This was when loading a random youtube video.

In PluginInstanceParent::NPP_GetValue, when the firefox asks for the plugin's scriptable object and a NULL actor comes back from the child process, it triggers this crash.

I can't reliably reproduce this, but the bug seems pretty clear.

Backtrace from the parent process:

#6  0x00007fb8d43ba724 in mozilla::plugins::PluginInstanceParent::NPP_GetValue (this=0x26006b0, aVariable=NPPVpluginScriptableNPObject, _retval=0x7fffdd766cb0) at /home/cjones/mozilla/electrolysis/dom/plugins/PluginInstanceParent.cpp:416
#7  0x00007fb8d43cd077 in mozilla::plugins::PluginModuleParent::NPP_GetValue (instance=0x2633ca0, variable=NPPVpluginScriptableNPObject, ret_value=0x7fffdd766cb0) at /home/cjones/mozilla/electrolysis/dom/plugins/PluginModuleParent.cpp:335
#8  0x00007fb8d417c6f3 in nsNPAPIPluginInstance::GetValueFromPlugin (this=0x2633c80, variable=NPPVpluginScriptableNPObject, value=0x7fffdd766cb0) at /home/cjones/mozilla/electrolysis/modules/plugin/base/src/nsNPAPIPluginInstance.cpp:1403
#9  0x00007fb8d417d041 in nsNPAPIPluginInstance::GetJSObject (this=0x2633c80, cx=0x191bef0, outObject=0x7fffdd766da0) at /home/cjones/mozilla/electrolysis/modules/plugin/base/src/nsNPAPIPluginInstance.cpp:1487
#10 0x00007fb8d3bf563b in nsHTMLPluginObjElementSH::GetPluginJSObject (cx=0x191bef0, obj=0x113a640, plugin_inst=0x2633c80, plugin_obj=0x7fffdd766da0, plugin_proto=0x7fffdd766d98) at /home/cjones/mozilla/electrolysis/dom/base/nsDOMClassInfo.cpp:9768
#11 0x00007fb8d3bf75ae in nsHTMLPluginObjElementSH::SetupProtoChain (wrapper=0x2581420, cx=0x191bef0, obj=0x113a640) at /home/cjones/mozilla/electrolysis/dom/base/nsDOMClassInfo.cpp:9506
#12 0x00007fb8d3bf7864 in nsHTMLPluginObjElementSH::PostCreate (this=0x22c5040, wrapper=0x2581420, cx=0x191bef0, obj=0x113a640) at /home/cjones/mozilla/electrolysis/dom/base/nsDOMClassInfo.cpp:9604
#13 0x00007fb8d332e669 in FinishCreate (ccx=@0x7fffdd767478, Scope=0x22d9c90, Interface=0x116a270, cache=0x2734308, wrapper=0x2581420, resultWrapper=0x7fffdd767290) at /home/cjones/mozilla/electrolysis/js/src/xpconnect/src/xpcwrappednative.cpp:660
#14 0x00007fb8d333241a in XPCWrappedNative::GetNewOrUsed (ccx=@0x7fffdd767478, Object=0x1463210, Scope=0x22d9c90, Interface=0x116a270, cache=0x2734308, isGlobal=0, resultWrapper=0x7fffdd767290) at /home/cjones/mozilla/electrolysis/js/src/xpconnect/src/xpcwrappednative.cpp:590
#15 0x00007fb8d3301b66 in XPCConvert::NativeInterface2JSObject (lccx=@0x7fffdd767430, d=0x7fffdd767e50, dest=0x0, src=0x1463210, iid=0x7fb8d470db50, Interface=0x7fb8d5313278, cache=0x2734308, scope=0x113a5c0, allowNativeWrapper=1, isGlobal=0, pErr=0x7fffdd76739c) at /home/cjones/mozilla/electrolysis/js/src/xpconnect/src/xpcconvert.cpp:1199
#16 0x00007fb8d3354141 in xpc_qsXPCOMObjectToJsval (lccx=@0x7fffdd767430, p=0x1463210, cache=0x0, iid=0x7fb8d470db50, iface=0x7fb8d5313278, rval=0x7fffdd767e50) at /home/cjones/mozilla/electrolysis/js/src/xpconnect/src/xpcquickstubs.cpp:1074
#17 0x00007fb8d3360daf in nsIDOMEvent_GetTarget (cx=0x191bef0, obj=0x113a5c0, id=17480260, vp=0x7fffdd767e50) at dom_quickstubs.cpp:9976
#18 0x00007fb8d29e1d26 in JSScopeProperty::get (this=0x1eac150, cx=0x191bef0, obj=0x113a5c0, pobj=0x24ad980, vp=0x7fffdd767e50) at /home/cjones/mozilla/electrolysis/js/src/jsscope.h:800
#19 0x00007fb8d29d7ef7 in js_NativeGet (cx=0x191bef0, obj=0x113a5c0, pobj=0x24ad980, sprop=0x1eac150, getHow=0, vp=0x7fffdd767e50) at /home/cjones/mozilla/electrolysis/js/src/jsobj.cpp:4280
#20 0x00007fb8d29d8534 in js_GetPropertyHelper (cx=0x191bef0, obj=0x113a5c0, id=17480260, getHow=0, vp=0x7fffdd767e50) at /home/cjones/mozilla/electrolysis/js/src/jsobj.cpp:4461
#21 0x00007fb8d29d8eac in js_GetProperty (cx=0x191bef0, obj=0x113a5c0, id=17480260, vp=0x7fffdd767e50) at /home/cjones/mozilla/electrolysis/js/src/jsobj.cpp:4471
#22 0x00007fb8d2942cce in JSObject::getProperty (this=0x113a5c0, cx=0x191bef0, id=17480260, vp=0x7fffdd767e50) at /home/cjones/mozilla/electrolysis/js/src/jsobj.h:267
#23 0x00007fb8d29ac7c1 in js_Interpret (cx=0x191bef0) at /home/cjones/mozilla/electrolysis/js/src/jsops.cpp:1933
#24 0x00007fb8d29c3510 in js_Invoke (cx=0x191bef0, argc=1, vp=0x18ff048, flags=0) at /home/cjones/mozilla/electrolysis/js/src/jsinterp.cpp:1371
#25 0x00007fb8d33248bf in nsXPCWrappedJSClass::CallMethod (this=0x1692a00, wrapper=0x2784610, methodIndex=3, info=0x139a320, nativeParams=0x7fffdd7687d0) at /home/cjones/mozilla/electrolysis/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1696
#26 0x00007fb8d331b755 in nsXPCWrappedJS::CallMethod (this=0x2784610, methodIndex=3, info=0x139a320, params=0x7fffdd7687d0) at /home/cjones/mozilla/electrolysis/js/src/xpconnect/src/xpcwrappedjs.cpp:570
#27 0x00007fb8d452e881 in PrepareAndDispatch (self=0x2307940, methodIndex=3, args=0x7fffdd768940, gpregs=0x7fffdd7688c0, fpregs=0x7fffdd7688f0) at /home/cjones/mozilla/electrolysis/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:153
#28 0x00007fb8d452d937 in SharedStub () from ./libxul.so
#29 0x00007fb8d39bd56f in nsEventListenerManager::HandleEventSubType (this=0x22e45a0, aListenerStruct=0x2550e28, aListener=0x2307940, aDOMEvent=0x2019d38, aCurrentTarget=0x1a24600, aPhaseFlags=2) at /home/cjones/mozilla/electrolysis/content/events/src/nsEventListenerManager.cpp:1034
#30 0x00007fb8d39bdabe in nsEventListenerManager::HandleEvent (this=0x22e45a0, aPresContext=0x22cf870, aEvent=0x7fffdd768de0, aDOMEvent=0x7fffdd768d10, aCurrentTarget=0x1a24600, aFlags=2, aEventStatus=0x7fffdd768d18) at /home/cjones/mozilla/electrolysis/content/events/src/nsEventListenerManager.cpp:1140
#31 0x00007fb8d39e7de2 in nsEventTargetChainItem::HandleEvent (this=0x14f44a0, aVisitor=@0x7fffdd768d00, aFlags=2, aMayHaveNewListenerManagers=0) at /home/cjones/mozilla/electrolysis/content/events/src/nsEventDispatcher.cpp:244
#32 0x00007fb8d39e821d in nsEventTargetChainItem::HandleEventTargetChain (this=0x14f4938, aVisitor=@0x7fffdd768d00, aFlags=6, aCallback=0x7fffdd768e60, aMayHaveNewListenerManagers=1) at /home/cjones/mozilla/electrolysis/content/events/src/nsEventDispatcher.cpp:330
#33 0x00007fb8d39e8979 in nsEventDispatcher::Dispatch (aTarget=0x2734300, aPresContext=0x22cf870, aEvent=0x7fffdd768de0, aDOMEvent=0x0, aEventStatus=0x7fffdd768e8c, aCallback=0x7fffdd768e60) at /home/cjones/mozilla/electrolysis/content/events/src/nsEventDispatcher.cpp:539
#34 0x00007fb8d39c521a in nsEventStateManager::DispatchMouseEvent (this=0x22ccd40, aEvent=0x7fffdd769850, aMessage=332, aTargetContent=0x2734300, aRelatedContent=0x0) at /home/cjones/mozilla/electrolysis/content/events/src/nsEventStateManager.cpp:3587
#35 0x00007fb8d39c550e in nsEventStateManager::NotifyMouseOut (this=0x22ccd40, aEvent=0x7fffdd769850, aMovingInto=0x0) at /home/cjones/mozilla/electrolysis/content/events/src/nsEventStateManager.cpp:3652
#36 0x00007fb8d39c5963 in nsEventStateManager::GenerateMouseEnterExit (this=0x22ccd40, aEvent=0x7fffdd769850) at /home/cjones/mozilla/electrolysis/content/events/src/nsEventStateManager.cpp:3757
#37 0x00007fb8d39cbbcd in nsEventStateManager::PreHandleEvent (this=0x22ccd40, aPresContext=0x22cf870, aEvent=0x7fffdd769850, aTargetFrame=0x26ef448, aStatus=0x7fffdd76959c, aView=0x22cd5d0) at /home/cjones/mozilla/electrolysis/content/events/src/nsEventStateManager.cpp:1108
#38 0x00007fb8d3663360 in PresShell::HandleEventInternal (this=0x22dd850, aEvent=0x7fffdd769850, aView=0x22cd5d0, aStatus=0x7fffdd76959c) at /home/cjones/mozilla/electrolysis/layout/base/nsPresShell.cpp:6505
#39 0x00007fb8d3663f15 in PresShell::HandlePositionedEvent (this=0x22dd850, aView=0x22cd5d0, aTargetFrame=0x26ef448, aEvent=0x7fffdd769850, aEventStatus=0x7fffdd76959c) at /home/cjones/mozilla/electrolysis/layout/base/nsPresShell.cpp:6372
#40 0x00007fb8d3664cec in PresShell::HandleEvent (this=0x22dd850, aView=0x22cd5d0, aEvent=0x7fffdd769850, aEventStatus=0x7fffdd76959c) at /home/cjones/mozilla/electrolysis/layout/base/nsPresShell.cpp:6236
#41 0x00007fb8d3b8afea in nsViewManager::HandleEvent (this=0x22cd540, aView=0x22cd5d0, aPoint={x = -579430848, y = 32767}, aEvent=0x7fffdd769850) at /home/cjones/mozilla/electrolysis/view/src/nsViewManager.cpp:1199
#42 0x00007fb8d3b8f571 in nsViewManager::DispatchEvent (this=0x22cd540, aEvent=0x7fffdd769850, aView=0x22cd5d0, aStatus=0x7fffdd76977c) at /home/cjones/mozilla/electrolysis/view/src/nsViewManager.cpp:1178
#43 0x00007fb8d3b850db in HandleEvent (aEvent=0x7fffdd769850) at /home/cjones/mozilla/electrolysis/view/src/nsView.cpp:167
#44 0x00007fb8d43054f4 in nsWindow::DispatchEvent (this=0x22cd680, aEvent=0x7fffdd769850, aStatus=@0x7fffdd7698cc) at /home/cjones/mozilla/electrolysis/widget/src/gtk2/nsWindow.cpp:579
#45 0x00007fb8d43012df in nsWindow::OnLeaveNotifyEvent (this=0x22cd680, aWidget=0xf395a0, aEvent=0x2332260) at /home/cjones/mozilla/electrolysis/widget/src/gtk2/nsWindow.cpp:2488
#46 0x00007fb8d43013bf in leave_notify_event_cb (widget=0xf395a0, event=0x2332260) at /home/cjones/mozilla/electrolysis/widget/src/gtk2/nsWindow.cpp:5413
#47 0x00007fb8d19dedf8 in _gtk_marshal_BOOLEAN__BOXED (closure=0x144c6e0, return_value=0x7fffdd769ae0, n_param_values=<value optimized out>, param_values=0x1c35b00, invocation_hint=<value optimized out>, marshal_data=0x7fb8d43012ea) at /build/buildd/gtk+2.0-2.16.1/gtk/gtkmarshalers.c:84
#48 0x00007fb8cff5127d in IA__g_closure_invoke (closure=0x144c6e0, return_value=0x7fffdd769ae0, n_param_values=2, param_values=0x1c35b00, invocation_hint=0x7fffdd769aa0) at /build/buildd/glib2.0-2.20.1/gobject/gclosure.c:767
#49 0x00007fb8cff66e3b in signal_emit_unlocked_R (node=0xd71170, detail=0, instance=0xf395a0, emission_return=0x7fffdd769c20, instance_and_params=0x1c35b00) at /build/buildd/glib2.0-2.20.1/gobject/gsignal.c:3247
#50 0x00007fb8cff682bd in IA__g_signal_emit_valist (instance=0xf395a0, signal_id=<value optimized out>, detail=0, var_args=0x7fffdd769c80) at /build/buildd/glib2.0-2.20.1/gobject/gsignal.c:2990
#51 0x00007fb8cff68953 in IA__g_signal_emit (instance=0x0, signal_id=3470345512, detail=81) at /build/buildd/glib2.0-2.20.1/gobject/gsignal.c:3037
#52 0x00007fb8d1ae709e in gtk_widget_event_internal (widget=0xf395a0, event=0x2332260) at /build/buildd/gtk+2.0-2.16.1/gtk/gtkwidget.c:4761
#53 0x00007fb8d19d8852 in IA__gtk_main_do_event (event=0x2332260) at /build/buildd/gtk+2.0-2.16.1/gtk/gtkmain.c:1614
#54 0x00007fb8d0d4bf3c in gdk_event_dispatch (source=<value optimized out>, callback=<value optimized out>, user_data=<value optimized out>) at /build/buildd/gtk+2.0-2.16.1/gdk/x11/gdkevents-x11.c:2364
#55 0x00007fb8cfab520a in IA__g_main_context_dispatch (context=0xd6e9e0) at /build/buildd/glib2.0-2.20.1/glib/gmain.c:1814
#56 0x00007fb8cfab88e0 in g_main_context_iterate (context=0xd6e9e0, block=0, dispatch=1, self=<value optimized out>) at /build/buildd/glib2.0-2.20.1/glib/gmain.c:2448
#57 0x00007fb8cfab8a7c in IA__g_main_context_iteration (context=0xd6e9e0, may_block=0) at /build/buildd/glib2.0-2.20.1/glib/gmain.c:2511
#58 0x00007fb8d4307990 in nsAppShell::ProcessNextNativeEvent (this=0xef31c0, mayWait=0) at /home/cjones/mozilla/electrolysis/widget/src/gtk2/nsAppShell.cpp:147
#59 0x00007fb8d43293a8 in nsBaseAppShell::DoProcessNextNativeEvent (this=0xef31c0, mayWait=0) at /home/cjones/mozilla/electrolysis/widget/src/xpwidgets/nsBaseAppShell.cpp:155
#60 0x00007fb8d432982b in nsBaseAppShell::OnProcessNextEvent (this=0xef31c0, thr=0xdfc8f0, mayWait=0, recursionDepth=0) at /home/cjones/mozilla/electrolysis/widget/src/xpwidgets/nsBaseAppShell.cpp:293
#61 0x00007fb8d4513341 in nsThread::ProcessNextEvent (this=0xdfc8f0, mayWait=0, result=0x7fffdd76a0bc) at /home/cjones/mozilla/electrolysis/xpcom/threads/nsThread.cpp:508
#62 0x00007fb8d44a97aa in NS_ProcessNextEvent_P (thread=0xdfc8f0, mayWait=0) at nsThreadUtils.cpp:230
#63 0x00007fb8d43eb86a in mozilla::ipc::MessagePump::Run (this=0xdf2ae0, aDelegate=0xde2230) at /home/cjones/mozilla/electrolysis/ipc/glue/MessagePump.cpp:115
#64 0x00007fb8d441de69 in MessageLoop::RunInternal (this=0xde2230) at /home/cjones/mozilla/electrolysis/ipc/chromium/src/base/message_loop.cc:211
#65 0x00007fb8d441de89 in MessageLoop::RunHandler (this=0xde2230) at /home/cjones/mozilla/electrolysis/ipc/chromium/src/base/message_loop.cc:194
#66 0x00007fb8d441deea in MessageLoop::Run (this=0xde2230) at /home/cjones/mozilla/electrolysis/ipc/chromium/src/base/message_loop.cc:168
#67 0x00007fb8d4329b0d in nsBaseAppShell::Run (this=0xef31c0) at /home/cjones/mozilla/electrolysis/widget/src/xpwidgets/nsBaseAppShell.cpp:174
#68 0x00007fb8d40b81e0 in nsAppStartup::Run (this=0x10743f0) at /home/cjones/mozilla/electrolysis/toolkit/components/startup/src/nsAppStartup.cpp:182
#69 0x00007fb8d32c00a8 in XRE_main (argc=4, argv=0x7fffdd76ace8, aAppData=0xd2d690) at /home/cjones/mozilla/electrolysis/toolkit/xre/nsAppRunner.cpp:3473
#70 0x0000000000401405 in main (argc=4, argv=0x7fffdd76ace8) at /home/cjones/mozilla/electrolysis/browser/app/nsBrowserApp.cpp:156


I couldn't get a backtrace from the child process when the crash occurred, sorry, and message parameter logging doesn't work on linux :S.  Maybe this stdout log helps though

[PluginModuleParent] NPP_DestroyStream
[time:1255068015939783][PPluginInstanceParent] Call PBrowserStreamDestructor(FIXME, FIXME, FIXME, )
[time:1255068016363289][PPluginInstanceChild] Answer PBrowserStreamDestructor(FIXME, FIXME, FIXME, )
[time:1255068016363405][PPluginInstanceChild] replying with PBrowserStreamDestructor(FIXME, )
[time:1255068016363706][PPluginInstanceParent] got reply PBrowserStreamDestructor(FIXME, )
BrowserStreamParent::~BrowserStreamParent<0x2537200>
[PluginModuleParent] NPP_URLNotify
[time:1255068016363792][PPluginInstanceParent] Call PStreamNotifyDestructor(FIXME, FIXME, )
[time:1255068016364062][PPluginInstanceChild] Answer PStreamNotifyDestructor(FIXME, FIXME, )
[time:1255068016364085][PPluginInstanceChild] replying with PStreamNotifyDestructor(FIXME, )
[time:1255068016364307][PPluginInstanceParent] got reply PStreamNotifyDestructor(FIXME, )
[PluginInstanceParent] NPP_GetValue(NPPVpluginScriptableNPObject)
[time:1255068016367278][PPluginInstanceParent] Call NPP_GetValue_NPPVpluginScriptableNPObject()
[time:1255068016367521][PPluginInstanceChild] Answer NPP_GetValue_NPPVpluginScriptableNPObject()
[PluginModuleChild] _retainobject: object 0x7fb853aaa180, refcnt 2
[PluginModuleChild] _releaseobject: object 0x7fb853aaa180, refcnt 1
[time:1255068016367552][PPluginInstanceChild] replying with NPP_GetValue_NPPVpluginScriptableNPObject(FIXME, FIXME, )
[time:1255068016367787][PPluginInstanceParent] got reply NPP_GetValue_NPPVpluginScriptableNPObject(FIXME, FIXME, )

Program ./firefox-bin (pid = 5960) received signal 11.
I get this pretty regularly while testing windowless drawing. 

PluginModuleChild.cpp

case NPNVWindowNPObject: {
    PPluginScriptableObjectChild* actor;
    NPError result;
    if (!CallNPN_GetValue_NPNVWindowNPObject(&actor, &result)) {
        NS_WARNING("Failed to send message!");
        return NPERR_GENERIC_ERROR;
    }

    if (result != NPERR_NO_ERROR) {
        return result;
    }

    NS_ASSERTION(actor, "Null actor!");

xul.dll!mozilla::plugins::PluginInstanceChild::NPN_GetValue(NPNVariable aVar=NPNVWindowNPObject, void * aValue=0x0130e144)
xul.dll!_getvalue(_NPP * aNPP=0x02a2a1a0, NPNVariable aVariable=NPNVWindowNPObject, void * aValue=0x0130e144)
NPSWF32.dll!55b55aa1()
In the case of a PluginInstanceChild failure, there's a failed lookup occurring in PPPluginInstanceChild's CallNPN_GetValue_NPNVWindowNPObject. 

if (__readok) {
    // deserializing actor type
    (*(value)) = reinterpret_cast<PPluginScriptableObjectChild*>(((((*((&(value_deTemp_2))))).mId) == (0) ? 0 : Lookup(((*((&(value_deTemp_2))))).mId)));
}

Lookup returns NULL. 

http://mxr.mozilla.org/projects-central/source/electrolysis/ipc/chromium/src/base/id_map.h#75

(I'm still trying to track down why this is missing from that hash table.)

You can reproduce this quite easily by viewing Hulu videos:

http://www.slashcontrol.com/free-tv-shows/family-guy/333786640-dog-gone

A null actor is usually produced during the transition between the advertisement and the video.

A simple fix is to test for a null actor here:

http://mxr.mozilla.org/projects-central/source/electrolysis/dom/plugins/PluginInstanceChild.cpp#151

Although I wonder if there is something deeper here that needs to be addressed.

FWIW, this should probably block turning plugins on be default, it's a very common crash on windows.
OS: Linux → All
Hardware: x86_64 → x86
Jim, there are a few possibilities for why this might be happening.  Fortunately or unfortunately, there are 3 or 4 patches sitting in my mq that would give us the precise cause of this failure.  They'll be pushed by the end of week.
Blocks: 531142
With all my outstanding patches applied, firefox trips the debugger when the lookup of a bad actor ID fails.  As I think this is the same as what's happening now, just sooner, I'm going to go ahead and push the patches.
Attached patch Patch, v1 (obsolete) — — Splinter Review 
This fixes hulu for me. We basically need to take better care to defer destroying actors until we're sure we don't have other ipdl code on the stack. This bullet-proofs the child process and I think takes care of the parent process.
Attachment #415975 - Flags: review?(benjamin)
Attachment #415975 - Flags: review?(benjamin) → review-
Attached patch Patch, v2 (obsolete) — — Splinter Review 
Ok, this is a much more complicated approach that I should have done in the beginning. Now we can 'resurrect' proxy objects if we have races.
Attachment #415975 - Attachment is obsolete: true
Attachment #417796 - Flags: review?(benjamin)
FWIW, when running this patch valgrind still reports use-after-free errors on PluginScriptableObjects.  Also FWIW, this bug looks like it might be causing this tinderbox crash
http://tinderbox.mozilla.org/showlog.cgi?tree=Firefox&buildname=Linux%20mozilla-central%20debug%20test%20everythingelse&buildtime=1261059426&logfile=1261059426.1261064981.21660.gz&errorparser=unittest
I didn't review the entire patch yet: here's what I have so far, but I'd really like better documentation of which invariants apply in which cases.

I'm also wondering whether we shouldn't have two different classes which implement PPluginScriptableObjectChild: one for plugin objects created for the browser and a different one for browser objects created for the plugin. I'm having lots of trouble reasoning through the behaviors when they share an implementation like this:

> diff --git a/dom/plugins/PPluginScriptableObject.ipdl b/dom/plugins/PPluginScriptableObject.ipdl
> +  rpc Retain();
> +  rpc Release();

This protocol should document whether the actor starts out in the
retained state or not. We discussed both ways on IRC, and I don't know
which one you decided to use.

> diff --git a/dom/plugins/PluginInstanceChild.cpp b/dom/plugins/PluginInstanceChild.cpp
> @@ -489,18 +489,24 @@ PluginInstanceChild::Destroy()
>      count = objects.Length();
>      for (PRUint32 index = 0; index < count; index++) {
>          PluginScriptableObjectChild*& actor = objects[index];
>          NPObject* object = actor->GetObject();
> -        if (object->_class == PluginScriptableObjectChild::GetClass()) {
> -          PluginScriptableObjectChild::ScriptableInvalidate(object);
> +        if (object &&
> +            object->_class == PluginScriptableObjectChild::GetClass()) {
> +            // Make sure we're cleaned up.
> +            PluginScriptableObjectChild::ScriptableDeallocate(object);
> +        }
> +        else {
> +            // Delete all the actors that the child created.
> +            PluginScriptableObjectChild::Call__delete__(actor);

Followup bug if necessary: The array here (objects) is saved out
because plugin code might re-enter it. But if we enter the plugin
during this call, isn't it possible to end up deleting an actor in the
list, so that PluginScriptableObjectChild::Call__delete__ would be
deleting an actor which is already dead (then crash)? Are
mutation-guards of some sort necessary here, or a mutation-safe hash
enumerator such as the one they're using in JS-land?

> @@ -728,28 +734,29 @@ PluginInstanceChild::AllocPPluginScripta

>  bool
>  PluginInstanceChild::DeallocPPluginScriptableObject(
>      PPluginScriptableObjectChild* aObject)
>  {
>      AssertPluginThread();
>  
> -    PluginScriptableObjectChild* object =
> +    PluginScriptableObjectChild* actor =
>          reinterpret_cast<PluginScriptableObjectChild*>(aObject);
>  
> -    NPObject* npobject = object->GetObject();
> -    if (npobject &&
> -        npobject->_class != PluginScriptableObjectChild::GetClass()) {
> -        PluginModuleChild::current()->UnregisterNPObject(npobject);
> +    NPObject* object = actor->GetObject();
> +    if (object) {
> +        NS_ASSERTION(PluginModuleChild::current()->NPObjectIsRegistered(object),
> +                     "NPObject not in the hash!");
> +        PluginModuleChild::current()->UnregisterNPObject(object);
>      }

There are three cases where this method is called, I think:

* It's a plugin-side object and the child called __delete__ on it
  (because the parent no longer needs it, or because we called
  invalidate/deallocate on it). In this case I think the caller of
  __delete__ should have already nulled unregistered and nulled out
  mObject and we should assert this.

* It's a browser-side object which we no longer reference. In this
  case actor->GetObject() should be NULL.

* We're destroying a PPluginInstanceChild but haven't called
  NPP_Destroy on it (NPP_Destroy unregisters objects). This is a
  programming error from the parent which is assertion-worthy, but it
  seems like the right behavior is then either to call NP_Shutdown
  (which should invalidate the actors anyway) or to runtimeabort.


>  PluginInstanceChild::AnswerPPluginScriptableObjectConstructor(
>                                             PPluginScriptableObjectChild* aActor)

> +    // We don't have an NPObject here, the Initialize call will make one.
> +    actor->Initialize(const_cast<PluginInstanceChild*>(this), nsnull);
> +    NS_ASSERTION(actor->GetObject(), "Actor should have an object!");

The invariant here is if this is a browser object, Actor.mObject ==
NULL implies that the actor is non-retained, and Actor.mObject != NULL
implies that the actor is retained? And actors start out retained? Or
do we keep the NPObject allocated (but with a refcount of 0) in the non-retained state?

> diff --git a/dom/plugins/PluginInstanceParent.cpp b/dom/plugins/PluginInstanceParent.cpp

> +namespace {
> +PLDHashOperator
> +ActorCollect(const void* aKey,
> +             PluginScriptableObjectParent* aData,
> +             void* aUserData)
> +{
> +    nsTArray<PluginScriptableObjectParent*>* objects =
> +        reinterpret_cast<nsTArray<PluginScriptableObjectParent*>*>(aUserData);
> +    return objects->AppendElement(aData) ? PL_DHASH_NEXT : PL_DHASH_STOP;
> +}
> +
> +}

add a // anonymous namespace comment here for clarity

>  void
>  PluginInstanceParent::Destroy()
>  {
> -    // Copy the actors here so we don't enumerate a mutating array.
> +    // Copy the actors here so we don't enumerate a mutating table.

Same comment as above: if the table is actually mutating, you're
liable for the mutations to destroy objects which you've enumerated.

> +            // Delete all the actors that the parent created.
> +            PluginScriptableObjectParent::Call__delete__(actor);

Is it safe to do that here? Do we know that this call is not nested in some deep call stack with these scriptable objects in-use on the stack?

> diff --git a/dom/plugins/PluginScriptableObjectChild.cpp b/dom/plugins/PluginScriptableObjectChild.cpp


> @@ -299,29 +313,31 @@ PluginScriptableObjectChild::ScriptableI
>    nsAutoTArray<Variant, 10> args;
>    if (!args.SetLength(aArgCount)) {
>      NS_ERROR("Out of memory?!");
>      return false;
>    }
>  
>    for (PRUint32 index = 0; index < aArgCount; index++) {
>      Variant& arg = args[index];
> -    if (!ConvertToRemoteVariant(aArgs[index], arg, actor->GetInstance())) {
> +    if (!ConvertToRemoteVariant(aArgs[index], arg, actor->GetInstance(),
> +        true)) {
>        NS_WARNING("Failed to convert argument!");
> +      if (index) {
> +        ReleaseRemoteVariants(args, index - 1);
> +      }

So your stack guard is basically function call pairs to ConvertToRemoteVariant/ReleaseRemoteVariant pairs? I was hoping it would be an automatic C++ object, e.g.

class ArgArray
{
public:
  ArgArray(const NPVariant* aArgs, uint32_t aArgCount);
  ~ArgArray();

  operator const nsTArrray<Variant>&() { return mArray; }

private:
  nsAutoTArray<Variant, 10> mArray;
};

I'm also worried that though you're using the stack guards for variants, you're not using them on NPObject* passed directly, i.e. aObject in this function. I think that you probably need another helper class for that explicitly:

/**
 * Retain a scriptableobject actor while we're making a method call on it.
 */
class HoldObject
{
public:
  HoldObject(PluginScriptableObjectChild* child);
  ~HoldObject();

  PluginScriptableObjectChild* operator->();
};

I'm not sure whether you need to use this holder class for things like hasmethod/hasproperty, but it would probably be safer to do so.
Attached patch Patch, v3 (obsolete) — — Splinter Review 
Ok, so now we have double-delete protection in the destruction sequence. We also have stack helper objects for retain/release. And I moved the common code that kept building up into a separate file. Youtube and hulu work, as do the mochitests.
Attachment #417796 - Attachment is obsolete: true
Attachment #419599 - Flags: review?(benjamin)
Attachment #417796 - Flags: review?(benjamin)
> @@ -565,16 +591,17 @@ PluginInstanceChild::CreatePluginWindow(
>          return true;
>          
>      if (!RegisterWindowClass())
>          return false;
>  
>      mPluginWindowHWND =
>          CreateWindowEx(WS_EX_LEFT | WS_EX_LTRREADING |
>                         WS_EX_NOPARENTNOTIFY | // XXXbent Get rid of this!
> +                       WS_EX_NOACTIVATE |

Unrelated?

> diff -wNdprU8 electrolysis.179bad633581/dom/plugins/PluginInstanceChild.h electrolysis/dom/plugins/PluginInstanceChild.h
> --- electrolysis.179bad633581/dom/plugins/PluginInstanceChild.h	2009-12-30 14:15:07 -0600
> +++ electrolysis/dom/plugins/PluginInstanceChild.h	2009-12-30 14:15:07 -0600
> @@ -179,26 +179,28 @@ private:
>                                               UINT message,
>                                               WPARAM wParam,
>                                               LPARAM lParam);
>  #endif
>  
>      const NPPluginFuncs* mPluginIface;
>      NPP_t mData;
>      NPWindow mWindow;
> +
> +    nsTArray<nsAutoPtr<PluginScriptableObjectChild> > mScriptableObjects;
> +    nsTArray<PluginScriptableObjectChild*>* mDeletedObjectsDuringShutdown;

Per IRC discussion, I don't think either of these are
necessary. Because IPDL does child actor destruction automatically
now, mScriptableObjects isn't needed to do it manually. And therefore
mDeletedObjectsDuringShutdown should be useless.

If they are needed, they need substantial doccomments.

Ditto for PluginInstanceParent, I think, although more assertions
might be in order there.

> diff -wNdprU8 electrolysis.179bad633581/dom/plugins/PluginInstanceParent.cpp electrolysis/dom/plugins/PluginInstanceParent.cpp

> +namespace {
> +
> +PLDHashOperator
> +ActorCollect(const void* aKey,
> +             PluginScriptableObjectParent* aData,
> +             void* aUserData)
> +{
> +    nsTArray<PluginScriptableObjectParent*>* objects =
> +        reinterpret_cast<nsTArray<PluginScriptableObjectParent*>*>(aUserData);
> +    return objects->AppendElement(aData) ? PL_DHASH_NEXT : PL_DHASH_STOP;
> +}
> +
> +} // anonymous namespace
> +
>  void
>  PluginInstanceParent::Destroy()
>  {
> -    // Copy the actors here so we don't enumerate a mutating array.
> +    // Copy the actors here so we don't enumerate a mutating table.
>      nsAutoTArray<PluginScriptableObjectParent*, 10> objects;
> -    PRUint32 count = mScriptableObjects.Length();
> +    mScriptableObjects.EnumerateRead(ActorCollect, &objects);
> +    NS_ASSERTION(objects.Length() == mScriptableObjects.Count(), "OOM?!");

As noted above, I don't think we need to do this, for two reasons:

* The plugin host should really be invalidating all objects before calling NPP_Destroy. If there are objects left here, we either never passed them to the plugin host, or there's a bug.
* When we send the __delete__ message, IPDL will delete the child actors anyway.

> +namespace {
>  
> -    return object->get();
> +PLDHashOperator
> +ActorRemove(const void* aKey,
> +            PluginScriptableObjectParent*& aData,
> +            void* aUserData)
> +{
> +    PluginScriptableObjectParent* actor =
> +        reinterpret_cast<PluginScriptableObjectParent*>(aUserData);
> +    return aData == actor ? PL_DHASH_REMOVE : PL_DHASH_NEXT;
>  }
>  
> +} // anonymous namespace
> +
>  bool
>  PluginInstanceParent::DeallocPPluginScriptableObject(
>                                           PPluginScriptableObjectParent* aObject)
>  {
> -    PluginScriptableObjectParent* object =
> -        reinterpret_cast<PluginScriptableObjectParent*>(aObject);
> +    PluginScriptableObjectParent* actor =
> +        static_cast<PluginScriptableObjectParent*>(aObject);
>  
> -    PRUint32 count = mScriptableObjects.Length();
> -    for (PRUint32 index = 0; index < count; index++) {
> -        if (mScriptableObjects[index] == object) {
> -            mScriptableObjects.RemoveElementAt(index);
> -            return true;
> +    NPObject* object = actor->GetObject();
> +    if (object) {
> +        NS_ASSERTION(mScriptableObjects.Get(object, nsnull),
> +                     "NPObject not in the hash!");
> +        mScriptableObjects.Remove(object);
>          }
> +    else {
> +        mScriptableObjects.Enumerate(ActorRemove, nsnull);
>      }

Why is this necessary? I don't understand why we'd have something in mScriptableObjects which has a null NPObject*.

> diff -wNdprU8 electrolysis.179bad633581/dom/plugins/PluginMessageUtils.h electrolysis/dom/plugins/PluginMessageUtils.h

> +enum ScriptableObjectType
> +{
> +  None = 0,
> +  LocalObject,
> +  Proxy
> +};

None? It looks to me like we could avoid the None type, see below.

> diff -wNdprU8 electrolysis.179bad633581/dom/plugins/PluginScriptableObjectChild.cpp electrolysis/dom/plugins/PluginScriptableObjectChild.cpp
> -/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> - * vim: sw=4 ts=4 et :
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> + * vim: sw=2 ts=2 et :

Here and elsewhere, tab-width should be 8 or some equally huge number,
so that hard tabs don't sneak in by accident.

>  // static
>  NPObject*
>  PluginScriptableObjectChild::ScriptableAllocate(NPP aInstance,
>                                                  NPClass* aClass)
>  {
>    AssertPluginThread();
>  
> -  NS_ASSERTION(aClass == PluginScriptableObjectChild::GetClass(),
> -               "Huh?! Wrong class!");
> -
> -  ChildNPObject* object = reinterpret_cast<ChildNPObject*>(
> -    PluginModuleChild::sBrowserFuncs.memalloc(sizeof(ChildNPObject)));
> -  if (object) {
> -    memset(object, 0, sizeof(ChildNPObject));
> +  if (aClass != GetClass()) {
> +    NS_ERROR("Huh?! Wrong class!");
> +    return nsnull;

Hrm, I think this should be stronger. Why not NS_RUNTIMEABORT here, and in the
rest of the PluginScriptableObjectChild::Scriptable* methods? Although this isn't directly related to the patch, so free to file a followup.

Also, almost all of these methods should assert that PluginScriptableObjectChild::mType is Proxy.

>  // static
>  void
>  PluginScriptableObjectChild::ScriptableInvalidate(NPObject* aObject)
>  {
>    AssertPluginThread();
>  
> -  if (aObject->_class != PluginScriptableObjectChild::GetClass()) {
> +  if (aObject->_class != GetClass()) {
>      NS_ERROR("Don't know what kind of object this is!");
>      return;
>    }
>  
>    ChildNPObject* object = reinterpret_cast<ChildNPObject*>(aObject);
>    if (object->invalidated) {
>      // This can happen more than once, and is just fine.
>      return;
>    }
>  
> -  PluginScriptableObjectChild* actor = object->parent;
> -
> -  PluginInstanceChild* instance = actor ? actor->GetInstance() : nsnull;
> -  NS_WARN_IF_FALSE(instance, "No instance!");
> -
> -  if (actor && !actor->CallInvalidate()) {
> -    NS_WARNING("Failed to send message!");
> -  }
> -
>    object->invalidated = true;
> -
> -  if (instance &&
> -      !PPluginScriptableObjectChild::Call__delete__(object->parent)) {
> -    NS_WARNING("Failed to send message!");
> -  }
>  }

We don't use the scriptable invalidate message any more? At this point can't we just delete the actor (or would that introduce a race of some sort?)

And remind me, although I think you explained this on IRC: why can't we null out ChildNPObject::parent immediately here, such that parent == NULL means invalidated, instead of having a separate invalidated boolean?

Or to be more precise, why can't we move the actor->DropNPObject() call from ScriptableDeallocate to ScriptableInvalidate?

>  PluginScriptableObjectChild::PluginScriptableObjectChild()
>  : mInstance(nsnull),
> -  mObject(nsnull)
> +  mObject(nsnull),
> +  mInvalidated(false),
> +  mRetainedCount(0),
> +  mType(None)
>  {
>    AssertPluginThread();
>  }

Can we avoid the None type by passing the type into the constructor? That is, for plugin objects, call the pre-created form with LocalObject, and in AnswerPPluginScriptableObjectConstructor, call the constructor with the Proxy form.



>  void
>  PluginScriptableObjectChild::Initialize(PluginInstanceChild* aInstance,
>                                          NPObject* aObject)

Now that we have Manager() is aInstance still necessary?

It feels like this method should be separate for proxy and local objects, or just be an overloaded constructor: InitializeProxy() would create the NPObject (which PluginInstanceChild::AnswerPPluginScriptableObjectConstructor does currently) and set up mType = Proxy. InitializeLocal(NPObject* o) would wrap/retain the plugin object.

> +void
> +PluginScriptableObjectChild::Retain()
> +{
> +  NS_ASSERTION(mObject, "No object!");
> +  NS_ASSERTION(mRetainedCount >= 0, "Negative retain count?!");
> +  NS_ASSERTION(mType != None, "Bad type!");
> +
> +  if (mType == LocalObject) {
> +    ++mRetainedCount;

Can't we assert that mType == LocalObject? In what cases would we be
calling this method on a Proxy?

> +void
> +PluginScriptableObjectChild::DropNPObject()
> +{
> +  NS_ASSERTION(mObject, "Invalidated object!");
> +  NS_ASSERTION(mObject->_class == GetClass(), "Wrong type of object!");
> +  NS_ASSERTION(mType == Proxy, "Shouldn't call this for non-proxy object!");
> +
> +  // We think we're about to be deleted, but we could be racing with the other
> +  // process.
> +  PluginModuleChild::current()->UnregisterNPObject(mObject);
> +  mObject = nsnull;
> +
> +  if (!CallRelease()) {
> +    NS_WARNING("Failed to send message!");

Why are you result-checking here?

>  bool
>  PluginScriptableObjectChild::AnswerInvalidate()
>  {
>    AssertPluginThread();
>  
> -  if (mObject) {
> +  if (mInvalidated) {
> +    NS_WARNING("Called invalidate more than once?!");
> +    return true;
> +  }
> +
> +  mInvalidated = true;
> +
>      NS_ASSERTION(mObject->_class != GetClass(), "Bad object type!");
> +  NS_ASSERTION(mType == LocalObject, "Bad type!");
> +
>      if (mObject->_class && mObject->_class->invalidate) {
>        mObject->_class->invalidate(mObject);
>      }
> +
>      PluginModuleChild::current()->UnregisterNPObject(mObject);
> -    PluginModuleChild::sBrowserFuncs.releaseobject(mObject);
> -    mObject = nsnull;
> -  }
> +  Release();

Release() doesn't null out mObject... won't that leave us with a
pointer to a dead mObject? Previously it seems the invariant was
"mObject is NULL after invalidate", but now there's a separate flag,
and I'm not sure why. Doc-comments on the members are necessary at least.

> diff -wNdprU8 electrolysis.179bad633581/dom/plugins/PluginScriptableObjectChild.h electrolysis/dom/plugins/PluginScriptableObjectChild.h

> @@ -46,16 +46,19 @@
>  namespace mozilla {
>  namespace plugins {
>  
>  class PluginInstanceChild;
>  class PluginScriptableObjectChild;
>  
>  struct ChildNPObject : NPObject
>  {
> +  ChildNPObject()
> +    : NPObject(), parent(NULL), invalidated(false) { }
> +
>    PluginScriptableObjectChild* parent;
>    bool invalidated;

I'd love doc comments for whether `parent` is always valid or is
sometimes NULL, and how that relates to the invalidated state.

>  class PluginScriptableObjectChild : public PPluginScriptableObjectChild

> +  GetObject(bool aCanResurrect = false);

I'd be slightly happier if this didn't have a default value (to make it explicit at the callsites whether resurrection is required), but that can be postponed to another bug if it's too intrusive.

> +  void Retain();
> +  void Release();
> +
> +  void DropNPObject();

These need docs.

> diff -wNdprU8 electrolysis.179bad633581/dom/plugins/PluginScriptableObjectParent.cpp electrolysis/dom/plugins/PluginScriptableObjectParent.cpp

> @@ -417,43 +237,40 @@ PluginScriptableObjectParent::Scriptable
>  
>  // static
>  bool
>  PluginScriptableObjectParent::ScriptableInvokeDefault(NPObject* aObject,
>                                                        const NPVariant* aArgs,
>                                                        uint32_t aArgCount,
>                                                        NPVariant* aResult)

> +  ProtectedActor<PluginScriptableObjectParent> actor(object->parent);
> +  if (!actor) {
> +    NS_WARNING("Dead actor!");
>      return false;

Under what conditions will we hit this? It seems to me that this should either be an assertion or we shouldn't warn.

>  void
>  PluginScriptableObjectParent::Initialize(PluginInstanceParent* aInstance,
>                                           NPObject* aObject)

Ditto comments from PluginScriptableObjectChild::Initialize above.

> +PluginScriptableObjectParent::Retain()
> +PluginScriptableObjectParent::Release()

I think I know what's bothering me about these methods. They seem like parallels to NPObject retain/release, but in fact they are quite different (because this retain/release applies to stack calls and the single remote reference). Perhaps renaming them StackRetain or something would help make that clear.

It's still not perfectly clear why we would ever be calling Retain/Release on a Proxy. Is it because ProtectedActor/ProtectedVariant Retain/Release without knowing whether an object is Proxy/Local?

> +bool
> +PluginScriptableObjectParent::AnswerInvalidate()
> +{
> +  NS_NOTREACHED("Shouldn't get here!");

Hrm. I'd really like this to become a "kill the plugin process" event. Do we have a method for that yet (Manager()->Manager()->FatalError())? If not, file and mark a TODO.

> diff -wNdprU8 electrolysis.179bad633581/dom/plugins/PluginScriptableObjectParent.h electrolysis/dom/plugins/PluginScriptableObjectParent.h

>  struct ParentNPObject : NPObject
>  {
>    ParentNPObject()
> -    : parent(NULL) { }
> +    : NPObject(), parent(NULL), invalidated(false) { }
>  
>    PluginScriptableObjectParent* parent;
> +  bool invalidated;
>  };

Ditto docs from ChildNPObject above.

> diff -wNdprU8 electrolysis.179bad633581/dom/plugins/PluginScriptableObjectUtils.cpp electrolysis/dom/plugins/PluginScriptableObjectUtils.cpp

> +namespace {
> +
> +template<class InstanceType>
> +class VariantTraits { };

I think you want to forward-declare this (by omitting the braces) instead of declaring an empty class.

> +namespace {
> +
> +// This function only exists to get both flavors of the ConvertToRemoteVariant
> +// function instantiated and should never be called.

This feels very wrong to me. Why do we need to do this? Automatic instantiation should work fine.

> diff -wNdprU8 electrolysis.179bad633581/dom/plugins/PluginScriptableObjectUtils.h electrolysis/dom/plugins/PluginScriptableObjectUtils.h

> + * The Initial Developer of the Original Code is
> + *   Ben Turner <bent.mozilla@gmail.com>
> + * Portions created by the Initial Developer are Copyright (C) 2009
> + * the Initial Developer. All Rights Reserved.

The Mozilla Foundation...

> +#ifndef DOM_PLUGINS_PLUGINSCRIPTABLEOBJECTUTILS_H
> +#define DOM_PLUGINS_PLUGINSCRIPTABLEOBJECTUTILS_H

Why all-caps? I think we use mixedCase everywhere else.

> +namespace mozilla {
> +namespace plugins {
> +
> +inline PluginInstanceParent*
> +GetInstance(NPObject* aObject)
> +{
> +  NS_ASSERTION(aObject->_class == PluginScriptableObjectParent::GetClass(),
> +               "Bad class!");
> +
> +  ParentNPObject* object = reinterpret_cast<ParentNPObject*>(aObject);
> +  if (object->invalidated) {
> +    NS_WARNING("Calling method on an invalidated object!");
> +    return nsnull;
> +  }
> +  if (!object->parent) {
> +    NS_WARNING("Dead actor!");
> +    return nsnull;
> +  }

Again, a warning seems odd here, either an assertion or no warning.

> +inline NPObject*
> +NPObjectFromVariant(const Variant& aRemoteVariant)
> +{
> +  if (aRemoteVariant.type() == Variant::TPPluginScriptableObjectParent) {
...
> +  if (aRemoteVariant.type() == Variant::TPPluginScriptableObjectChild) {
...
> +  NS_NOTREACHED("Shouldn't get here!");

As a style nit, this feels like you want a switch statement, but I don't much care.

> +inline void
> +ReleaseRemoteVariant(Variant& aVariant)
> +{
> +  if (aVariant.type() == Variant::TPPluginScriptableObjectParent) {

style nit, switch here also

> +bool
> +ConvertToVariant(const Variant& aRemoteVariant,
> +                 NPVariant& aVariant,
> +                 PluginInstanceParent* aInstance = nsnull);

Why is aInstance allowed to be null sometimes?

> \ No newline at end of file

nit, please fix
Blocks: 530007
Attachment #419599 - Flags: review?(benjamin)
Attached patch Patch, v4 (obsolete) — — Splinter Review 
(In reply to comment #10)
> Unrelated?

Yup.

> Per IRC discussion, I don't think either of these are
> necessary. Because IPDL does child actor destruction automatically
> now, mScriptableObjects isn't needed to do it manually. And therefore
> mDeletedObjectsDuringShutdown should be useless.

Fixed.


> Why is this necessary? I don't understand why we'd have something in
> mScriptableObjects which has a null NPObject*.

You're right, we shouldn't. I've changed this to a DEBUG-only search/assertion.

> Here and elsewhere, tab-width should be 8 or some equally huge number,
> so that hard tabs don't sneak in by accident.

Fixed.

> Hrm, I think this should be stronger. Why not NS_RUNTIMEABORT here, and in the
> rest of the PluginScriptableObjectChild::Scriptable* methods? Although this
> isn't directly related to the patch, so free to file a followup.

Done.

> Also, almost all of these methods should assert that
> PluginScriptableObjectChild::mType is Proxy.

I think that was the only one that didn't :)

> We don't use the scriptable invalidate message any more? At this point can't we
> just delete the actor (or would that introduce a race of some sort?)

We don't send it to the other side this direction, no, as our pluginhost code can't handle it (leaks). The docs seem to suggest that it should only be called by the browser anyway. I forgot to update the IPDL spec, will do that in the next version.

Deleting the actor here may race, yes.

> And remind me, although I think you explained this on IRC: why can't we null
> out ChildNPObject::parent immediately here

We need the parent to be non-null so that we can resurrect if we need to.

> Or to be more precise, why can't we move the actor->DropNPObject() call from
> ScriptableDeallocate to ScriptableInvalidate?

That would cause leaks. Once we call DropNPObject we lose all reference to the NPObject (remove from hash and null out the actor's mObject member). If we don't immediately delete it we'll leak, so I think we can only do that in ScriptableDeallocate.

> Can we avoid the None type by passing the type into the constructor? 

Ok.

> Now that we have Manager() is aInstance still necessary?

No.

> It feels like this method should be separate for proxy and local objects

Done.

> Can't we assert that mType == LocalObject? In what cases would we be
> calling this method on a Proxy?

We call retain for every actor in the stack guard auto-helpers. Just depends on which objects a plugin uses in its method calls.

> Why are you result-checking here?

Oops, bad habit.

> Release() doesn't null out mObject... won't that leave us with a
> pointer to a dead mObject? Previously it seems the invariant was
> "mObject is NULL after invalidate", but now there's a separate flag,
> and I'm not sure why. Doc-comments on the members are necessary at least.

Correct, because we may have a Retain on the stack. Once we get to the last release then we'll call __delete__. This is one reason why we need the separate invalidate flag - it's not a dead object, just invalidated.

> I'd love doc comments for whether `parent` is always valid or is
> sometimes NULL, and how that relates to the invalidated state.

Ok.

> I'd be slightly happier if this didn't have a default value (to make it
> explicit at the callsites whether resurrection is required), but that can be
> postponed to another bug if it's too intrusive.

Done.

> These need docs.

Done.

> Under what conditions will we hit this? It seems to me that this should either
> be an assertion or we shouldn't warn.

I think this can only be null if the plugin process died. I'll remove the warnings.

> I think I know what's bothering me about these methods. They seem like
> parallels to NPObject retain/release, but in fact they are quite different
> (because this retain/release applies to stack calls and the single remote
> reference). Perhaps renaming them StackRetain or something would help make that
> clear.

Ok. Protect/Unprotect.

> It's still not perfectly clear why we would ever be calling Retain/Release on a
> Proxy. Is it because ProtectedActor/ProtectedVariant Retain/Release without
> knowing whether an object is Proxy/Local?

Yes.

> Hrm. I'd really like this to become a "kill the plugin process" event. Do we
> have a method for that yet (Manager()->Manager()->FatalError())? If not, file
> and mark a TODO.

Just removed it from IPDL.

> I think you want to forward-declare this (by omitting the braces) instead of
> declaring an empty class.

Indeed!

> This feels very wrong to me. Why do we need to do this? Automatic instantiation
> should work fine.

I agree, but something isn't right. I get two unresolved symbol errors on windows without it though.

> The Mozilla Foundation...

Ok. I think we've done this wrong before then.

> Why all-caps? I think we use mixedCase everywhere else.

Fixed.

> Again, a warning seems odd here, either an assertion or no warning.

Removed.

> As a style nit, this feels like you want a switch statement, but I don't much
> care.

Fixed.

> style nit, switch here also

Fixed.

> Why is aInstance allowed to be null sometimes?

Because it's only needed on the parent side. Child side can use the static module getter.

> nit, please fix

Done
Attachment #419599 - Attachment is obsolete: true
Attachment #421572 - Flags: review?(benjamin)
Here's the interdiff for easier review, I hope ;)
Oh, and you must have the patch for bug 539343 applied to get thing working.
Status: NEW → ASSIGNED
Build fail on linux with v4:

PluginScriptableObjectParent.cpp
In file included from /home/cjones/mozilla/electrolysis/ipc/glue/WindowsMessageLoop.h:44,
                 from /home/cjones/mozilla/electrolysis/dom/plugins/PluginInstanceParent.cpp:46:
../../dist/system_wrappers/windows.h:3:26: error: windows.h: No such file or directory
In file included from /home/cjones/mozilla/electrolysis/dom/plugins/PluginInstanceParent.cpp:46:
/home/cjones/mozilla/electrolysis/ipc/glue/WindowsMessageLoop.h:47:27: error: nsTraceRefCnt.h: No such file or directory
In file included from /home/cjones/mozilla/electrolysis/dom/plugins/PluginInstanceParent.cpp:46:
/home/cjones/mozilla/electrolysis/ipc/glue/WindowsMessageLoop.h:75: error: expected ‘)’ before ‘aHWnd’
/home/cjones/mozilla/electrolysis/ipc/glue/WindowsMessageLoop.h:88: error: ‘HWND’ does not name a type
/home/cjones/mozilla/electrolysis/ipc/glue/WindowsMessageLoop.h:89: error: ‘UINT’ does not name a type
/home/cjones/mozilla/electrolysis/ipc/glue/WindowsMessageLoop.h:90: error: ‘WPARAM’ does not name a type
/home/cjones/mozilla/electrolysis/ipc/glue/WindowsMessageLoop.h:91: error: ‘LPARAM’ does not name a type
/home/cjones/mozilla/electrolysis/ipc/glue/WindowsMessageLoop.h:99: error: expected ‘)’ before ‘aHWnd’
/home/cjones/mozilla/electrolysis/ipc/glue/WindowsMessageLoop.h:108: error: ‘HWND’ does not name a type
/home/cjones/mozilla/electrolysis/ipc/glue/WindowsMessageLoop.h:109: error: ‘UINT’ does not name a type
/home/cjones/mozilla/electrolysis/ipc/glue/WindowsMessageLoop.h:116: error: expected ‘)’ before ‘aHWnd’
/home/cjones/mozilla/electrolysis/ipc/glue/WindowsMessageLoop.h:123: error: ‘HWND’ does not name a type
/home/cjones/mozilla/electrolysis/ipc/glue/WindowsMessageLoop.h:131: error: expected ‘)’ before ‘aHWnd’
/home/cjones/mozilla/electrolysis/ipc/glue/WindowsMessageLoop.h:146: error: expected ‘)’ before ‘aHWnd’
/home/cjones/mozilla/electrolysis/ipc/glue/WindowsMessageLoop.h:155: error: ‘HWND’ does not name a type
/home/cjones/mozilla/electrolysis/ipc/glue/WindowsMessageLoop.h:156: error: ‘UINT’ does not name a type
/home/cjones/mozilla/electrolysis/ipc/glue/WindowsMessageLoop.h:162: error: expected ‘)’ before ‘aHWnd’
/home/cjones/mozilla/electrolysis/ipc/glue/WindowsMessageLoop.h:170: error: ‘HRGN’ does not name a type
In file included from /home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectChild.cpp:41:
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h: In function ‘void mozilla::plugins::ReleaseRemoteVariant(mozilla::plugins::Variant&)’:
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘T__None’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘Tvoid_t’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘Tnull_t’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘Tbool’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘Tint’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘Tdouble’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘TnsCString’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘T__First’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h: At global scope:
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:265: error: declaration of ‘typedef ActorType mozilla::plugins::ProtectedActor<ActorType>::ActorType’
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:261: error:  shadows template parm ‘class ActorType’
make[5]: *** [PluginScriptableObjectChild.o] Error 1
make[5]: *** Waiting for unfinished jobs....
In file included from /home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.cpp:41:
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h: In function ‘void mozilla::plugins::ReleaseRemoteVariant(mozilla::plugins::Variant&)’:
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘T__None’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘Tvoid_t’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘Tnull_t’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘Tbool’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘Tint’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘Tdouble’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘TnsCString’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘T__First’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h: At global scope:
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:265: error: declaration of ‘typedef ActorType mozilla::plugins::ProtectedActor<ActorType>::ActorType’
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:261: error:  shadows template parm ‘class ActorType’
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.cpp: In function ‘bool mozilla::plugins::ConvertToRemoteVariant(const NPVariant&, mozilla::plugins::Variant&, InstanceType*, bool)’:
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.cpp:177: error: ‘actor’ was not declared in this scope
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.cpp: At global scope:
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.cpp:220: error: expected ‘)’ before ‘*’ token
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.cpp:229: error: specialization of ‘mozilla::plugins::ProtectedActor<ActorType>::~ProtectedActor() [with ActorType = mozilla::plugins::PluginScriptableObjectParent]’ in different namespace
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.cpp:229: error:   from definition of ‘mozilla::plugins::ProtectedActor<ActorType>::~ProtectedActor() [with ActorType = mozilla::plugins::PluginScriptableObjectParent]’
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.cpp:237: error: expected ‘)’ before ‘*’ token
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.cpp:245: error: specialization of ‘mozilla::plugins::ProtectedActor<ActorType>::~ProtectedActor() [with ActorType = mozilla::plugins::PluginScriptableObjectChild]’ in different namespace
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.cpp:245: error:   from definition of ‘mozilla::plugins::ProtectedActor<ActorType>::~ProtectedActor() [with ActorType = mozilla::plugins::PluginScriptableObjectChild]’
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.cpp: In function ‘bool mozilla::plugins::ConvertToRemoteVariant(const NPVariant&, mozilla::plugins::Variant&, InstanceType*, bool) [with InstanceType = mozilla::plugins::PluginInstanceParent]’:
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:180:   instantiated from here
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.cpp:177: error: dependent-name ‘<unnamed>::VariantTraits::ScriptableObjectType’ is parsed as a non-type, but instantiation yields a type
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.cpp:177: note: say ‘typename <unnamed>::VariantTraits::ScriptableObjectType’ if a type is meant
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.cpp: In function ‘bool mozilla::plugins::ConvertToRemoteVariant(const NPVariant&, mozilla::plugins::Variant&, InstanceType*, bool) [with InstanceType = mozilla::plugins::PluginInstanceChild]’:
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:186:   instantiated from here
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.cpp:177: error: dependent-name ‘<unnamed>::VariantTraits::ScriptableObjectType’ is parsed as a non-type, but instantiation yields a type
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.cpp:177: note: say ‘typename <unnamed>::VariantTraits::ScriptableObjectType’ if a type is meant
make[5]: *** [PluginScriptableObjectUtils.o] Error 1
In file included from /home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectParent.cpp:41:
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h: In function ‘void mozilla::plugins::ReleaseRemoteVariant(mozilla::plugins::Variant&)’:
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘T__None’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘Tvoid_t’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘Tnull_t’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘Tbool’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘Tint’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘Tdouble’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘TnsCString’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:137: warning: enumeration value ‘T__First’ not handled in switch
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h: At global scope:
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:265: error: declaration of ‘typedef ActorType mozilla::plugins::ProtectedActor<ActorType>::ActorType’
/home/cjones/mozilla/electrolysis/dom/plugins/PluginScriptableObjectUtils.h:261: error:  shadows template parm ‘class ActorType’
make[5]: *** [PluginScriptableObjectParent.o] Error 1
make[5]: *** [PluginInstanceParent.o] Error 1

Hopefully it's something simple, I'll see what I can do.
Attached patch v4, makes gcc happy (obsolete) — — Splinter Review 
(1) Dropped the inclusion of WindowsMessageLoop.h from PluginInstanceParent.cpp.  (Quick scan suggested that it wasn't used, but this change may be wrong.)

(2) gcc didn't like ProtectedActor; converted it to take a template traits param instead.
Attached file valgrind log from v4 — 
Visited youtube with a debug build from the gcc-friendly v4.  The first two groups in the log look like ScriptableObject stuff.  The second group (assertion failures) was introduced by me, and although it seems odd to me, is perhaps expected.

The third and fourth groups look like new bugs, will file tomorrow.
Comment on attachment 421572 [details] [diff] [review]
Patch, v4

r=me based on the interdiff. I'd like a valgrind run of mochitest-ipcplugins to be clean if possible... I'll try confirming cjones' results in a bit.
Attachment #421572 - Flags: review?(benjamin) → review+
(In reply to comment #16)
> Created an attachment (id=421611) [details]
> valgrind log from v4
> 
> The second group
> (assertion failures) was introduced by me, and although it seems odd to me, is
> perhaps expected.
> 

As bent noted on IRC, I added this assertion, and it fails a lot.  I added it because gcc warned about ignoring all the other enumeration values in this switch statement.  It'd be nice to change this code to make gcc happy.

        Attachment #421572 -
        Attachment is obsolete: true

        Attachment #421588 -
        Attachment is obsolete: true

        Attachment #421883 -
        Flags: review+

    
    

    
  


  

        Attachment #421920 -
        Flags: review?(bent.mozilla)

    
    

    
  
Comment on attachment 421920 [details] [diff] [review]
Use inlines, rev. 1

Looks good to me!

        Attachment #421920 -
        Flags: review?(bent.mozilla) → review+

    
    

    
  
Pushed changesets 76cdc8296409 and 9baa220b27c0 to mozilla-central.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED

    
    

    
  
#0  0x00007f6e7c1de500 in mozilla::plugins::PPluginScriptableObjectParent::Lookup (this=0x7f6e66c7ff00, aId=-1) at PPluginScriptableObjectParent.cpp:1583

#1  0x00007f6e7c1deb7a in mozilla::plugins::PPluginScriptableObjectParent::OnCallReceived (this=0x7f6e66c7ff00, msg=@0x7fff851ce720, reply=@0x7fff851ce648)

    at PPluginScriptableObjectParent.cpp:848

#2  0x00007f6e7c1cea0c in mozilla::plugins::PPluginModuleParent::OnCallReceived

    (this=0x7f6e666f8400, msg=@0x7fff851ce720, reply=@0x7fff851ce648)

    at PPluginModuleParent.cpp:415

#3  0x00007f6e7c1c9206 in mozilla::ipc::RPCChannel::DispatchIncall (

    this=0x7f6e666f8410, call=@0x7fff851ce720)

    at ../../../src/ipc/glue/RPCChannel.cpp:347

#4  0x00007f6e7c1c95cd in mozilla::ipc::RPCChannel::Incall (

    this=0x7f6e666f8410, call=@0x7fff851ce720, stackDepth=1)

    at ../../../src/ipc/glue/RPCChannel.cpp:332

#5  0x00007f6e7c1c9ea3 in mozilla::ipc::RPCChannel::Call (this=0x7f6e666f8410, 

    msg=0x7f6e66d59420, reply=0x7fff851ce820)

    at ../../../src/ipc/glue/RPCChannel.cpp:180

#6  0x00007f6e7c1e2a23 in mozilla::plugins::PPluginScriptableObjectParent::CallInvalidate (this=0x7f6e66c7ff00) at PPluginScriptableObjectParent.cpp:126

#7  0x00007f6e7c1c08cb in mozilla::plugins::PluginScriptableObjectParent::ScriptableInvalidate (aObject=0x7f6e6671f4e0)

    at ../../../src/dom/plugins/PluginScriptableObjectParent.cpp:116

So we're doing CallInvalidate and receiving a call in response, but mManager is NULL: I think because the actor should already be dead... isn't it destroyed within NPP_Destroy which happens before all this?

    
  
Depends on: 540600

    
    

    
  


  
This currently leaks PPluginScriptableObjectParent because of an IPDL bug (filed as bug 540600), but with that fix I think it is correct.

        Attachment #422333 -
        Flags: review?(bent.mozilla)

    
    

    
  
Original checkin of this patch caused:
https://bugzilla.mozilla.org/show_bug.cgi?id=540572

which now seems fixed with the latest build that has this backed out.

    
    

    
  
FWIW, I don't think my latest patch is necessary because bent forgot to land bug
539343 along with this in mozilla-central. But I'm slightly unsure why landing
that is sufficient... is ~PluginScriptableObjectParent doing the necessary
invalidation?

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/8df73bd34304
http://hg.mozilla.org/mozilla-central/rev/dc5c1aad29be
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED

    
    

    
  
I still can't get Hulu video to go full-screen with OOPP enabled.

Running latest hourly build:

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20100119 Minefield/3.7a1pre Firefox/3.6 ID:2010011916114

See this in Error Console2:
Error: store.free is not a function
Source file: http://static.huluim.com/system/hulu_48671.js
Line: 844

    
    

    
  
Hulu full-screen is bug 539658 and is unrelated to this bug.
Target Milestone: --- → mozilla1.9.3a1

    
    

    
  
Comment on attachment 422333 [details] [diff] [review]
PluginScriptableObjectParent::ActorDestroy, rev. 1

Clearing review request until we decide we need this.

        Attachment #422333 -
        Flags: review?(bent.mozilla)

    
    

    
  
Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

    
    

    
  
This changeset from this bug seems to have introduced a behavioral different between in-process and out-of-process plugins:

http://hg.mozilla.org/mozilla-central/rev/76cdc8296409

If an in-process plugin returns an error from NPP_New we assume the instance was not created and we don't call NPP_Destroy. This changeset appears to make our out-of-process code call NPP_Destroy after a plugin returns an error from NPP_New.

I filed bug 564919 about this.
Blocks: 564919

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: