526401 - Electrolysis: crash with NULL aNPP in _getvalue NPNVSupportsXEmbedBool when initializing java and gnash plugins

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Karl Tomlinson (:karlt)]

For application/x-java-applet;version=1.5 found plugin libnpjp2.so
LoadPlugin() /opt/sun-jdk-1.6.0.15/jre/lib/amd64/libnpjp2.so returned 7f84d1786520
[PluginModuleParent] LoadModule
==> process 10333 launched child process 10361
WARNING: NS_ENSURE_TRUE(compMgr) failed: file nsComponentManagerUtils.cpp, line 90
WARNING: NS_ENSURE_TRUE(compMgr) failed: file nsComponentManagerUtils.cpp, line 90
WARNING: NS_ENSURE_TRUE(compMgr) failed: file nsComponentManagerUtils.cpp, line 90
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: file:///mnt/sda11/karl/moz/obj/electrolysis/dist/bin/components/fuelApplication.js :: extApp_initToolkitHelpers :: line 1306"  data: no]
************************************************************
WARNING: Cannot create startup observer : service,@mozilla.org/fuel/application;1: file /home/karl/moz/electrolysis/embedding/components/appstartup/src/nsAppStartupNotifier.cpp, line 113
[PluginModuleChild] Init
[PluginModuleParent] NP_Initialize
[PluginModuleChild] AnswerNP_Initialize
[PluginModuleChild] _getvalue

#0  0x00007fdc322bf805 in InstCast (aNPP=0x0)
    at /home/karl/moz/electrolysis/dom/plugins/PluginModuleChild.cpp:487
#1  0x00007fdc322c19ad in _getvalue (aNPP=0x0, 
    aVariable=NPNVSupportsXEmbedBool, aValue=0x7fdc2815d167)
    at /home/karl/moz/electrolysis/dom/plugins/PluginModuleChild.cpp:529
#2  0x00007fdc253b0244 in MozNPN_GetValue ()
   from /opt/sun-jdk-1.6.0.15/jre/lib/amd64/libnpjp2.so
#3  0x00007fdc253b06e6 in NP_Initialize ()
   from /opt/sun-jdk-1.6.0.15/jre/lib/amd64/libnpjp2.so
#4  0x00007fdc322bf9fe in mozilla::plugins::PluginModuleChild::AnswerNP_Initialize (this=0x9d6878, _retval=0x7fdc2815d22e)
    at /home/karl/moz/electrolysis/dom/plugins/PluginModuleChild.cpp:1243
#5  0x00007fdc322c7045 in mozilla::plugins::PPluginModuleChild::OnCallReceived
    (this=0x9d6878, msg=@0x7fdc2815d3f0, reply=@0x7fdc2815d338)
    at ../../ipc/ipdl/_ipdlheaders/mozilla/plugins/PPluginModuleChild.h:364
#6  0x00007fdc322eb64a in mozilla::ipc::RPCChannel::DispatchIncall (
    this=0x9d6888, call=@0x7fdc2815d3f0)
    at /home/karl/moz/electrolysis/ipc/glue/RPCChannel.cpp:316
#7  0x00007fdc322eba11 in mozilla::ipc::RPCChannel::Incall (this=0x9d6888, 
    call=@0x7fdc2815d3f0, stackDepth=0)
    at /home/karl/moz/electrolysis/ipc/glue/RPCChannel.cpp:301
#8  0x00007fdc322ebace in mozilla::ipc::RPCChannel::OnMaybeDequeueOne (
    this=0x9d6888) at /home/karl/moz/electrolysis/ipc/glue/RPCChannel.cpp:236
#9  0x00007fdc322ec528 in DispatchToMethod<mozilla::ipc::RPCChannel, void (mozilla::ipc::RPCChannel::*)()> (obj=0x9d6888, 
    method=0x7fdc322eba14 <mozilla::ipc::RPCChannel::OnMaybeDequeueOne()>, 
    arg=@0x7fdc20000a00)
    at /home/karl/moz/electrolysis/ipc/chromium/src/base/tuple.h:383

revision 06a506e68700

Comment 2

[Benjamin Smedberg]

bent has something in progress

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

This should do it. I'm leaving the namespace changes because I think we'll need them soon.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 415251 [details] [diff] [review]
Patch, v1

>diff --git a/dom/plugins/PPluginModule.ipdl b/dom/plugins/PPluginModule.ipdl
>--- a/dom/plugins/PPluginModule.ipdl
>+++ b/dom/plugins/PPluginModule.ipdl
>@@ -86,12 +86,16 @@ parent:
>+
>+  rpc NPN_GetValue_WithBoolReturn(uint32_t aVariable)
>+    returns (NPError aError,
>+             bool aBoolVal);
> };
> 

Drive-by comment: can we please change that |uint32_t| into 

  union NPNBoolVariable {
    NPNV_some_empty_c++_struct_1;
    NPNV_bool_foo;
    // ...
  }

That will compile into almost exactly the same machine code as the |uint32_t| version, with the added benefit of being typesafe.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1.1

This uses paramtraits and the c++ enum to make sure we don't pass some junk value in.

Comment 6

[Benjamin Smedberg]

Comment on attachment 415283 [details] [diff] [review]
Patch, v1.1

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

>+namespace mozilla {
>+namespace plugins {
>+namespace child {
>+
> // FIXME
> typedef void (*PluginThreadCallback)(void*);
> 
> PR_BEGIN_EXTERN_C
> 
> static NPError NP_CALLBACK
> _requestread(NPStream *pstream, NPByteRange *rangeList);
> 
>@@ -468,80 +474,88 @@ _popupcontextmenu(NPP instance, NPMenu* 
> 
> static NPBool NP_CALLBACK
> _convertpoint(NPP instance, 
>               double sourceX, double sourceY, NPCoordinateSpace sourceSpace,
>               double *destX, double *destY, NPCoordinateSpace destSpace);
> 
> PR_END_EXTERN_C
> 
>+} /* namespace child */
>+} /* namespace plugins */
>+} /* namespace mozilla */

You should remove the PR_BEGIN/END_EXTERN_C here... I think it will likely either nullify the effect of the namespace (in terms of linkage) or at least be very confusing.

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

>+namespace {
>+const NPP_t gNullNPP = { 0, 0 };

This feels really strange to me... it feels like we should be passing NPP(NULL) instead of an obviously-invalid NPP_t*.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Pushed changeset 0873b46c5ec9 with those changes to electrolysis.