Closed Bug 516524 Opened 15 years ago Closed 15 years ago

Remote NPObject

Categories

(Core :: IPC, defect)

x86
Windows NT
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: benjamin, Assigned: bent.mozilla)

References

Details

Attachments

(2 files, 1 obsolete file)

Remote NPObject so that plugins work.
Attached patch Patch, v1 (obsolete) — Splinter Review
Here we go. This gets us NPObjects in both directions.
Attachment #403833 - Flags: review?(jst)
Attachment #403833 - Flags: review?(benjamin)
Depends on: 519813
Comment on attachment 403833 [details] [diff] [review]
Patch, v1

>diff --git a/dom/plugins/PPluginScriptableObject.ipdl b/dom/plugins/PPluginScriptableObject.ipdl

>-child:
>+both:
>   // NPClass methods
>   rpc Invalidate();

Shouldn't Invalidate() be child-only?

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

>+void
>+PluginInstanceChild::Destroy()

>+    count = objects.Length();
>+    for (PRUint32 index = 0; index < count; index++) {
>+        NPObject* object = objects[index]->GetObject();
>+        if (object->_class == PluginScriptableObjectChild::GetClass()) {
>+            object->_class->invalidate(object);
>+            PluginModuleChild::sBrowserFuncs.releaseobject(object);

How does this mesh with calls to invalidate from the parent side? Or are those not forwarded? Why do we need to use object->_class->invalidate if we could just call a method PluginScriptableObjectChild::DoSomething directly?

> bool
>-PluginInstanceChild::DeallocPPluginScriptableObject(PPluginScriptableObjectChild* aObject)
>+PluginInstanceChild::DeallocPPluginScriptableObject(
>+                                          PPluginScriptableObjectChild* aObject)

>     PRUint32 count = mScriptableObjects.Length();
>     for (PRUint32 index = 0; index < count; index++) {
>         if (mScriptableObjects[index] == object) {
>             mScriptableObjects.RemoveElementAt(index);
>             return true;
>         }
>     }

This is potentially a pretty long list, no? And the IPDL machinery already maintains a mapping of actors IDs, right? I don't really understand why we have mScriptableObjects at all.

>+bool
>+PluginInstanceChild::AnswerPPluginScriptableObjectConstructor(
>+                                           PPluginScriptableObjectChild* aActor)
>+{
>+    // This is only called in response to the parent process requesting the
>+    // creation of an actor. This actor will represent an NPObject that is
>+    // created by the browser and returned to the plugin.
>+    ChildNPObject* object = reinterpret_cast<ChildNPObject*>(
>+        PluginModuleChild::sBrowserFuncs.createobject(
>+            GetNPP(), PluginScriptableObjectChild::GetClass()));

It feels very funny again to be wandering through sBrowserFuncs.createobject when you could just call `new childNPObject`.

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

>+void
>+PluginInstanceParent::Destroy()

>+        if (object->_class == PluginScriptableObjectParent::GetClass()) {
>+            object->_class->invalidate(object);

Now I'm really confused. Firstly, I thought that only the plugin host ever mucked around with object->_class directly. Secondly, invalidate is documented to be called by the plugin host, not by plugin instances.


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

>@@ -64,16 +64,18 @@ class PluginInstanceParent : public PPlu

>+protected:
>+    bool InternalGetValueForNPObject(NPNVariable aVariable,
>+                                     PPluginScriptableObjectParent** aValue,
>+                                     NPError* aResult);

PluginInstanceParent should never be subclassed, so this should be private, not protected.

> #if NP_VERSION_MINOR > 19

What are the cases where NP_VERSION_MINOR would be less than 19? This seems like an unnecessary ifdef.

>+void
>+ScriptableInvalidate(NPObject* aObject)
>+{
>+  if (aObject->_class != PluginScriptableObjectChild::GetClass()) {
>+    NS_WARNING("Don't know what kind of object this is!");
>+    return;
>+  }

Here and below, this check seems like it should be an assertion or a runtime abort, not a warning.

>+void
>+ScriptableDeallocate(NPObject* aObject)

>+  PluginModuleChild::sBrowserFuncs.memfree(aObject);

argh, just free() it already!

>+ScriptableHasMethod(NPObject* aObject,
>+                    NPIdentifier aName)

>+  bool result;
>+  if (!actor->CallHasMethod((NPRemoteIdentifier)aName, &result)) {

why are you result-checking this? I was pretty sure that child methods could never fail (failure would cause process-abort. Although I also thought that they were void, so perhaps there's some magic here!

> bool
>-PluginScriptableObjectChild::AnswerConstruct(const nsTArray<NPRemoteVariant>& aArgs,
>-                                             NPRemoteVariant* aResult,
>+PluginScriptableObjectChild::AnswerConstruct(const nsTArray<Variant>& aArgs,
>+                                             Variant* aResult,

>   for (PRUint32 index = 0; index < argCount; index++) {
>     if (!ConvertToVariant(aArgs[index], convertedArgs[index])) {
>+      // Don't leak things we've already converted!
>+      while (index-- > 0) {
>+        PluginModuleChild::sBrowserFuncs.releasevariantvalue(
>+          &convertedArgs[index]);
>+      }

What are the conditions where you ConvertToVariant could fail but we should continue running? This seems like a lot of code for an edge case, and the while loop with postfix -- gives me the willies.

>+  for (PRUint32 index = 0; index < argCount; index++) {
>+    PluginModuleChild::sBrowserFuncs.releasevariantvalue(&convertedArgs[index]);

Throughout you are accessing this through PluginModuleChild::sBrowserFuncs. We should just call the functions directly: PluginModuleChild::NPN_ReleaseVariantValue or somesuch.

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

> class PluginInstanceChild;
>+class PluginScriptableObjectChild;
>+
>+struct ChildNPObject : NPObject
>+{
>+  PluginScriptableObjectChild* parent;
>+  bool invalidated;
>+};

I'm surprised that we've got this intermediate class ChildNPObject. I figured it would just be:

class PluginScriptableObjectChild : public PPluginScriptableObjectChild
{
   ... blabblahblah
   NPObject mObject; // MUST BE THE LAST MEMBER
};

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

> inline PluginInstanceParent*
> GetInstance(NPObject* aObject)
> {
>+  if (aObject->_class != PluginScriptableObjectParent::GetClass()) {
>+    NS_WARNING("Can't get an instance from this type of object!");
>+    return nsnull;
>+  }

Why is this a warning? Shouldn't callers be class-checking the object before calling into this function? Sound like its worth of an assertion or even a runtime-abort.

I'm a little queasy about the serialization of NPIdentifier. In some ways I'd prefer for it to be a protocol, or at least have IPDL enforce that it be sanity-checked rather than spreading calls to EnsureValidIdentifier in each method which receives an NPIdentifier. e.g.:

protocol PPluginModule
{
parent:
  check NPIdentifier;
};

Which translates into a signature

bool
PPluginModuleParent::CheckNPIdentifier(NPIdentifier value) = 0;

And IPDL calls this signature whenever an NPIdentifier comes across the wire.

>+bool
>+PluginScriptableObjectParent::AnswerInvoke(const NPRemoteIdentifier& aId,
>+                                           const nsTArray<Variant>& aArgs,
>+                                           Variant* aResult,
>+                                           bool* aSuccess)

>+  if (!(mObject->_class && mObject->_class->invoke)) {
>+    *aSuccess = false;
>+    return true;
>+  }

In a bunch of these methods you are doing these checks against _class. Why? Have you encountered situations where a plugin object has a null class or invoke? That seems like it would be really rare. Also, you end up calling _class->invoke directly, instead of using the canonical NPN_Invoke method... is there some reason for that? In particular you are skipping the NPPAutoPusher in the standard plugin host method, which seems undesirable.

>diff --git a/dom/plugins/PluginScriptableObjectParent.h b/dom/plugins/PluginScriptableObjectParent.h

> struct ParentNPObject : NPObject
> {
>-  PluginScriptableObjectParent* parent;
>+  // This will initially be set with a valid |parent| and will not be changed
>+  // until |invalidated| is set to true. At that point the union will contain
>+  // a valid |npn| that will never be unset.
>+  union {
>+    PluginScriptableObjectParent* parent;
>+    const NPNetscapeFuncs* npn;
>+  };
>   bool invalidated;
> };

It seems like the only reason to have the NPNetscapeFuncs part of the union is one call which needs to free memory. Since we know what allocator the plugin host is using, can we just skip the complication and call NS_free() directly?

> class PluginScriptableObjectParent : public PPluginScriptableObjectParent

As with the child, but even easier, it seems that we could just do:

class PluginScriptableObjectParent : public NPObject, public PPluginScriptableObjectParent and avoid an allocation. As long as the destructor ordering isn't too weird, anyway. Maybe it's not worth it, though.

>   static NPClass sNPClass;

Any reason this shouldn't be const?
Attachment #403833 - Flags: review?(benjamin) → review-
Attached patch Patch, v2Splinter Review
(In reply to comment #3)
> Shouldn't Invalidate() be child-only?

I don't think so. There's code in nsJSNPRuntime.cpp that responds to invalidate calls by releasing objects so if we don't follow suit I think we will change expected/established behavior.

> How does this mesh with calls to invalidate from the parent side? Or are those
> not forwarded?

The idea is that we want to drop any refs to objects provided by the other process here. Invalidate does that.

> Why do we need to use object->_class->invalidate if we could
> just call a method PluginScriptableObjectChild::DoSomething directly?

Those are static funcs hidden in an anonymous namespace currently, not members of PluginScriptableObjectChild.

> This is potentially a pretty long list, no? And the IPDL machinery already
> maintains a mapping of actors IDs, right? I don't really understand why we have
> mScriptableObjects at all.

Those maps are not currently exposed from IPDL code, and as far as I know the actors are not destroyed on destruction of the manager yet. Also, cjones says there is no "tree" of actors yet, just one flat map that holds all subactors (PluginInstance and PluginScriptableObject alike). We talked a while ago about exposing them but the discussion stagnated. I don't have a real feel for how many objects we could have here, but I would hope that it would be fewer than would cause a perf hit using an array.

> It feels very funny again to be wandering through sBrowserFuncs.createobject
> when you could just call `new childNPObject`.

Really? CreateObject also sets the class and refcount, which I would have to do here as well. Just seems like duplicating code that could change at some point in the future.

> Now I'm really confused. Firstly, I thought that only the plugin host ever
> mucked around with object->_class directly.

I don't know if that's any kind of hard rule... The class struct is in NPRuntime and plugins can use it if they want. Here in the parent you're right, we should be using the NPN_ functions. Fixed that.

> Secondly, invalidate is documented
> to be called by the plugin host, not by plugin instances.

Documented, maybe, but we have code to handle plugins calling it. Who knows what would break if we dropped that.

> PluginInstanceParent should never be subclassed, so this should be private, not
> protected.

Ok.

> What are the cases where NP_VERSION_MINOR would be less than 19? This seems
> like an unnecessary ifdef.

This is copy/paste cruft from somewhere. Removed by cjones recently.

> Here and below, this check seems like it should be an assertion or a runtime
> abort, not a warning.

Fixed.

> argh, just free() it already!

I can, but I hate having separate code paths in case we ever change anything in there (like logging allocations or something). Do you feel strongly about that?

> 
> >+ScriptableHasMethod(NPObject* aObject,
> >+                    NPIdentifier aName)
> 
> >+  bool result;
> >+  if (!actor->CallHasMethod((NPRemoteIdentifier)aName, &result)) {
> 
> why are you result-checking this? I was pretty sure that child methods could
> never fail (failure would cause process-abort. Although I also thought that
> they were void, so perhaps there's some magic here!
> 

> What are the conditions where you ConvertToVariant could fail but we should
> continue running? This seems like a lot of code for an edge case, and the while
> loop with postfix -- gives me the willies.

Ok, made that nonfail.

> Throughout you are accessing this through PluginModuleChild::sBrowserFuncs. We
> should just call the functions directly:
> PluginModuleChild::NPN_ReleaseVariantValue or somesuch.

Those don't currently exist. Are you saying you want them to be created? And what does that buy us?

> I'm surprised that we've got this intermediate class ChildNPObject.

The PluginScriptableObjectChild wraps NPObjects from the browser side as well as NPObjects from the plugin so that isn't possible.

> Why is this a warning?

Fixed.


> I'm a little queasy about the serialization of NPIdentifier.

I'll see what cjones thinks and file a followup.

> In a bunch of these methods you are doing these checks against _class.

I switched these to all use the NPN_foo methods. *facepalm*.

> It seems like the only reason to have the NPNetscapeFuncs part of the union is
> one call which needs to free memory. Since we know what allocator the plugin
> host is using, can we just skip the complication and call NS_free() directly?

Indeed. Done.

> As with the child, but even easier, it seems that we could just do:

No, because it can wrap browser objects too.

> Any reason this shouldn't be const?

NPN_CreateObject takes a non-const NPClass pointer, but that's easy to fix.
Attachment #403833 - Attachment is obsolete: true
Attachment #404359 - Flags: review?(benjamin)
Attachment #403833 - Flags: review?(jst)
> I'm a little queasy about the serialization of NPIdentifier.

Filed bug 520302.
> I don't think so. There's code in nsJSNPRuntime.cpp that responds to invalidate
> calls by releasing objects so if we don't follow suit I think we will change
> expected/established behavior.

Can you add an API issue to https://wiki.mozilla.org/NPAPI_Questions on this, then?

> > Why do we need to use object->_class->invalidate if we could
> > just call a method PluginScriptableObjectChild::DoSomething directly?
> 
> Those are static funcs hidden in an anonymous namespace currently, not members
> of PluginScriptableObjectChild.

I'd prefer to have them be static functions of PluginScriptableObjectChild and call them directly... it's slightly more efficient and a lot less confusing.

> Those maps are not currently exposed from IPDL code, and as far as I know the
> actors are not destroyed on destruction of the manager yet. Also, cjones says
> there is no "tree" of actors yet, just one flat map that holds all subactors
> (PluginInstance and PluginScriptableObject alike). We talked a while ago about

Hrm... the map would have to know the subobject type, or else you could dispatch unsafe messages.

> exposing them but the discussion stagnated. I don't have a real feel for how
> many objects we could have here, but I would hope that it would be fewer than
> would cause a perf hit using an array.

Let's file a bug for followup: there's a lot of machinery here, in addition to potential perf issues, and we should reuse the IPDL machinery if at all possible.


> > Secondly, invalidate is documented
> > to be called by the plugin host, not by plugin instances.
> 
> Documented, maybe, but we have code to handle plugins calling it. Who knows
> what would break if we dropped that.

We could at least have a dummy function that asserted/aborted, and see what happens...

> > argh, just free() it already!
> 
> I can, but I hate having separate code paths in case we ever change anything in
> there (like logging allocations or something). Do you feel strongly about that?

I feel strongly about avoiding indirection through the function pointer table. If you want to make a static PluginModuleChild::FreeMemory and call it directly, that's ok. Still seems overly complicated, though.
Comment on attachment 404359 [details] [diff] [review]
Patch, v2

r=me with recommended changes, no need for another round
Attachment #404359 - Flags: review?(benjamin) → review+
Just wanted to add that this patch works like a charm on linux: youtube not only works, but has full framerate and smooth playback.

Three cheers for Mr. Turner!
This landed.

(In reply to comment #6)

> Can you add an API issue to https://wiki.mozilla.org/NPAPI_Questions on this,
> then?

Done.

> I'd prefer to have them be static functions of PluginScriptableObjectChild and
> call them directly... it's slightly more efficient and a lot less confusing.

Done.

> Let's file a bug for followup: there's a lot of machinery here, in addition to
> potential perf issues, and we should reuse the IPDL machinery if at all
> possible.

Done, bug 521272.

> I feel strongly about avoiding indirection through the function pointer table.
> If you want to make a static PluginModuleChild::FreeMemory and call it
> directly, that's ok. Still seems overly complicated, though.

Just free'd.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: