293972 - npruntime API does not support ambiguous members

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Pete Zha]

In some cases, we need to support ambiguous members, a property and a methos has
same name. For example, java suppport this kind of thing. You can define a
property and a function with same name. But currently NPAPI+ does not support
it. The implementation in nsJSNPRuntime will not handle this case. It will throw
exception.
 
To resolve this problem, we need to implement a Member class when the object has
ambiguous members.

I will attach a patch soon.

Comment 1

[Pete Zha]

Attachment: patch v1

Comment 2

[Mike Shaver (:shaver emeritus)]

Comment on attachment 183472 [details] [diff] [review]
patch v1

I don't understand why you can't just return a callable proxy from your npobj's
hasProperty, which does the right things for convert and call.	Can you explain
to my why that wouldn't work?

Comment 3

[Pete Zha]

(In reply to comment #2)
> (From update of attachment 183472 [details] [diff] [review] [edit])
> I don't understand why you can't just return a callable proxy from your npobj's
> hasProperty, which does the right things for convert and call.	Can you explain
> to my why that wouldn't work?
> 

nsJSNPRuntime will throw exception if I return true in both haaMethod and
hasProperty. I don't know how to return a callable proxy through NPObject... If
you have idea, please let me know. Thanks.

Comment 4

[Pete Zha]

Attachment: patch v2

small fix for last patch

Comment 5

[timeless]

Comment on attachment 185657 [details] [diff] [review]
patch v2

>Index: nsJSNPRuntime.cpp
>+JS_STATIC_DLL_CALLBACK(JSBool)
>+NPObjectMember_convert(JSContext *cx, JSObject *obj,
>+                              JSType type, jsval *vp);
			 ^ doesn't line up

>@@ -1357,4 +1402,129 @@
>+CreateNPObjectMember(NPP npp, JSContext *cx, JSObject *obj,
>+                     NPObject* npobj, jsval id, jsval *vp)
>+{
if !vp you're not going to throw an exception:
>+  NS_ENSURE_TRUE(vp, false);
is that ok?
>+  if (!npobj || !npobj->_class || !npobj->_class->getProperty) {
>+    ThrowJSException(cx, "Bad NPObject");
>+
>+    return false;
>+  }
>+
>+  jsval fieldValue, methodValue;
>+
>+  NPVariant npv;
>+  VOID_TO_NPVARIANT(npv);
>+  if (!npobj->_class->getProperty(npobj, (NPIdentifier)id, &npv))
>+    return false;
>+
>+  fieldValue = NPVariantToJSVal(npp, cx, &npv);
>+  
>+  JSObject *methodObject, *funObject;
>+  methodObject =
>+    JS_GetFunctionObject(JS_NewFunction(cx, CallNPMethod, 0, JSFUN_BOUND_METHOD,
>+                                        NULL, JS_GetStringBytes(JSVAL_TO_STRING(id))));
JS_NewFunction can fail, you just crashed. Don't do that.

>+  funObject = JS_CloneFunctionObject(cx, methodObject, obj);
JS_CloneFunctionObject can fail, i'm not quite sure where/when you'll crash,
but i'm sure you will. deal with it.
afaik JS_CloneFunction will not preserve its parameter such that it doesn't get
destroyed by a last ditch gc in the process of constructing the function.
Either establish a localrootscope or somehow make sure methodObject is rooted
before you call this creature.
>+  methodValue = OBJECT_TO_JSVAL(funObject);
>+  
>+  NPObjectMethodOrFieldValue *memberValue =
>+    (NPObjectMethodOrFieldValue *)JS_malloc(cx, sizeof(*memberValue));
>+  if (!memberValue)
>+    return false;
>+    
>+  JSObject *objp = JS_NewObject(cx, &sNPObjectMemberClass, nsnull, nsnull);
JS_NewObject performed a last ditch GC and just destroyed your nice memberValue
because it wasn't rooted. see above.

>+  if (!objp) {
>+    JS_free(cx, memberValue);
>+    return false;
>+  }
>+
>+  JS_SetPrivate(cx, objp, (void *)memberValue);
>+  
>+  memberValue->methodValue = methodValue;  
>+  memberValue->fieldValue = fieldValue;
>+  memberValue->npp = npp;
>+
>+  *vp = OBJECT_TO_JSVAL(objp);
>+  return true;
>+}

>+JS_STATIC_DLL_CALLBACK(JSBool)
>+NPObjectMember_convert(JSContext *cx, JSObject *obj, JSType type, jsval *vp)
>+{
>+  NPObjectMethodOrFieldValue *memberValue;
>+  memberValue = (NPObjectMethodOrFieldValue *)JS_GetPrivate(cx, obj);
>+
>+  NPVariant npVar;
>+  NPObject* npobj;
>+  if (!memberValue) {
>+    if (type == JSTYPE_OBJECT) {
>+      *vp = OBJECT_TO_JSVAL(obj);
	v this doesn't line up.
>+        return JS_TRUE;
>+    }

>+    if (!JSValToNPVariant(memberValue->npp, cx, 
>+                          memberValue->fieldValue, &npVar))
why the block:
>+    {
>+      return JS_FALSE;
>+    }
you don't use a block here:
>+    if (!npobj)
>+      return JS_FALSE;
>+  default:
>+    NS_ASSERTION(0, "illegal operation on JSObject prototype object");
NS_ERROR. But what does this mean? if js code can trigger this case then you
should not use NS_ERROR/NS_ASSERTION. If this means someone writing c++/c code
screwed up, then this is ok.
>+    return JS_FALSE;
>+  }
>+}

>+// JS engine should not call it anyway
>+JS_STATIC_DLL_CALLBACK(JSBool)
>+NPObjectMember_Call(JSContext *cx, JSObject *obj,
>+                    uintN argc, jsval *argv, jsval *rval)
>+{
>+  NS_ASSERTION(0, "Should not call NPObjectMember");
NS_ERROR.
>+  return JS_TRUE;
> }

Comment 6

[Pete Zha]

Comment on attachment 185657 [details] [diff] [review]
patch v2

Thanks, timeless.
I still want jst to look at it before I made a new patch.

Comment 7

[Pete Zha]

Comment on attachment 185657 [details] [diff] [review]
patch v2

Thanks, timeless.
I still want jst to look at it before I made a new patch.

Comment 8

[Pete Zha]

> >+  JSObject *methodObject, *funObject;
> >+  methodObject =
> >+    JS_GetFunctionObject(JS_NewFunction(cx, CallNPMethod, 0, JSFUN_BOUND_METHOD,
> >+                                        NULL,
JS_GetStringBytes(JSVAL_TO_STRING(id))));
> JS_NewFunction can fail, you just crashed. Don't do that.
> 
> >+  funObject = JS_CloneFunctionObject(cx, methodObject, obj);
> JS_CloneFunctionObject can fail, i'm not quite sure where/when you'll crash,
> but i'm sure you will. deal with it.
> afaik JS_CloneFunction will not preserve its parameter such that it doesn't get
> destroyed by a last ditch gc in the process of constructing the function.
> Either establish a localrootscope or somehow make sure methodObject is rooted
> before you call this creature.
What do you mean? I should add the function obj to root before clone it?

> >+  JSObject *objp = JS_NewObject(cx, &sNPObjectMemberClass, nsnull, nsnull);
> JS_NewObject performed a last ditch GC and just destroyed your nice memberValue
> because it wasn't rooted. see above.
Can you explain whay above code works for me if it can be destroyed?

Comment 9

[timeless]

build w/ TOO_MUCH_GC and WAY_TOO_MUCH_GC, you should crash.
http://www.mozilla.org/js/spidermonkey/gctips.html

Comment 10

[Pete Zha]

update dependency

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 185657 [details] [diff] [review]
patch v2

+  // To support ambiguous members, we should return a Member class

remove "should", and use the full class name here (NPObject Member class".

+  if (npobj->_class->hasProperty(npobj, (NPIdentifier)id) &&
+      npobj->_class->hasMethod(npobj, (NPIdentifier)id))
...
+
   if (npobj->_class->hasProperty(npobj, (NPIdentifier)id)) {

That's two hasProperty() calls in a row (and potentially two hasMethod() calls
too. Make only one of each calls and use PRBool locals to hold the result of
those calls and check those...

- In CreateNPObjectMember():

+  fieldValue = NPVariantToJSVal(npp, cx, &npv);
+  
+  JSObject *methodObject, *funObject;
+  methodObject =
+    JS_GetFunctionObject(JS_NewFunction(cx, CallNPMethod, 0,
JSFUN_BOUND_METHOD,
+					 NULL,
JS_GetStringBytes(JSVAL_TO_STRING(id))));
+  funObject = JS_CloneFunctionObject(cx, methodObject, obj);
+  methodValue = OBJECT_TO_JSVAL(funObject);
+  
+  NPObjectMethodOrFieldValue *memberValue =
+    (NPObjectMethodOrFieldValue *)JS_malloc(cx, sizeof(*memberValue));
+  if (!memberValue)
+    return false;
+    
+  JSObject *objp = JS_NewObject(cx, &sNPObjectMemberClass, nsnull, nsnull);
+  if (!objp) {
+    JS_free(cx, memberValue);
+    return false;
+  }
+
+  JS_SetPrivate(cx, objp, (void *)memberValue);
+  
+  memberValue->methodValue = methodValue;  
+  memberValue->fieldValue = fieldValue;
+  memberValue->npp = npp;

All of that is likely to crash if GC runs in the middle of this code
(last-ditch GC if we're low on memory), as timeless already mentioned. And the
new JS member class also needs to ensure that the value/function values don't
get GC'd and deleted before the member object does. I'm not sure this general
approach of exposing callable members is what we'd want (see comment below re
JSTYPE_FUNCTION), but if it is, I'd suggest rewriting the above like this (note
the use of PR_Malloc() instead of JS_Malloc(), JS_Free() needs to be replaced
with PR_Free() in all places where the memberValue is freed too):

  NPObjectMethodOrFieldValue *memberValue =
    (NPObjectMethodOrFieldValue *)PR_malloc(cx, sizeof(*memberValue));
  if (!memberValue) {
    return false;
  }

  memberValue->npp = npp;
  memberValue->fieldValue = JSVAL_VOID;
  memberValue->methodValue = JSVAL_VOID;

  JSObject *objp = ::JS_NewObject(cx, &sNPObjectMemberClass, nsnull, nsnull);
  if (!objp) {
    PR_free(cx, memberValue);

    return false;
  }

  // This will pass ownership of memberValue to obj
  ::JS_SetPrivate(cx, objp, memberValue);

  ::JS_LockGCThing(cx, obj);

  memberValue->fieldValue = NPVariantToJSVal(npp, cx, &npv);

  JSObject *methodObject, *funObject;

  JSFunction *fun =
    ::JS_NewFunction(cx, CallNPMethod, 0, JSFUN_BOUND_METHOD,
		     NULL, ::JS_GetStringBytes(JSVAL_TO_STRING(id)));
  if (!fun) {
    ::JS_UnlockGCThing(cx, obj);

    return false;
  }

  funObject = ::JS_CloneFunctionObject(cx, ::JS_GetFunctionObject(fun), obj);

  if (!funObject) {
    ::JS_UnlockGCThing(cx, obj);

    return false;
  }

  memberValue->methodValue = OBJECT_TO_JSVAL(funObject);

  ::JS_UnlockGCThing(cx, obj);

And in addition to that, you'll need a mark hook that looks something like
this:

JS_STATIC_DLL_CALLBACK(uint32)
NPObjectMember_Mark(JSContext *cx, JSObject *obj, void *arg)
{
  NPObjectMethodOrFieldValue *memberValue;

  memberValue = (NPObjectMethodOrFieldValue *)JS_GetPrivate(cx, obj);
  if (!memberValue)
    return;

  if (!JSVAL_IS_PRIMITIVE(memberValue->fieldValue)) {
    ::JS_MarkGCThing(cx, JSVAL_TO_OBJECT(memberValue->fieldValue),
		     "NPObject Member => fieldValue", arg);
  }

  if (!JSVAL_IS_PRIMITIVE(memberValue->methodValue)) {
    ::JS_MarkGCThing(cx, JSVAL_TO_OBJECT(memberValue->methodValue),
		     "NPObject Member => methodValue", arg);
  }

  return 0;
}

- In NPObjectMember_convert()

+  switch (type) {
+  case JSTYPE_VOID:
+  case JSTYPE_STRING:
+  case JSTYPE_NUMBER:
+  case JSTYPE_BOOLEAN:
+    if (JSVAL_IS_PRIMITIVE(memberValue->fieldValue)) {
+      *vp = memberValue->fieldValue;

Why do this only for primitive values?

[...]
+    npobj->_class->invoke(npobj, _getstringidentifier("toString"),
+			   nsnull, 0, &retVar);
+    *vp = NPVariantToJSVal(memberValue->npp, cx, &retVar);

And what's the purpose of invoking toString() on the NPObject here?

+  case JSTYPE_OBJECT:
+    *vp = memberValue->fieldValue;
+    return JS_TRUE;

Shouldn't object be grouped in with the above types (related the the comment
earlier about primitive values).

+  case JSTYPE_FUNCTION:
+    *vp = memberValue->methodValue;
+    return JS_TRUE;

Couldn't you use the NPObjectMember_Call() hook to make this member object
callable instead of hooking into the convert hook? If you did that, you
wouldn't need to create and clone a function in CreateNPObjectMember(), you'd
simply create a JSObject of your new class that carries the NPObject, field
value, and method name as private data (much like you now carry the field value
and the new function object), then in the call() hook you'd simply call the
method on the NPObject.

r- until we figure all that out.

Comment 12

[Pete Zha]

> 
> Couldn't you use the NPObjectMember_Call() hook to make this member object
> callable instead of hooking into the convert hook? If you did that, you
> wouldn't need to create and clone a function in CreateNPObjectMember(), you'd
> simply create a JSObject of your new class that carries the NPObject, field
> value, and method name as private data (much like you now carry the field value
> and the new function object), then in the call() hook you'd simply call the
> method on the NPObject.
> 
I tried this way, but the JSObject passed in Call() method is strange. It has no
class and I can not get private data from it. Actually it has no slots...
Johnny, do you have any idea? Thanks!

Comment 13

[Pete Zha]

OK, I have resolved last comments. As usual, I need to get it from argv[-2]. I
will try to address other comments soon.

Comment 14

[Pete Zha]

Attachment: patch v3

Johnny,
This is the modified patch. Please review.

Comment 15

[Pete Zha]

Attachment: patch v3

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 188531 [details] [diff] [review]
patch v3

Yeah, that's a lot better! But you left you the mark() hook to ensure the
member object's private data's members doen't get collected, see my earlier
comments. Add that, and I think this is good to go.

Comment 17

[Pete Zha]

Attachment: patch v4

new patch with jst's comments

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 188622 [details] [diff] [review]
patch v4

+JS_STATIC_DLL_CALLBACK(JSBool)
+NPObjectMember_convert(JSContext *cx, JSObject *obj,
+			       JSType type, jsval *vp);

Fix indentation.

+static JSClass sNPObjectMemberClass =
+  {
+    "NPObject Member class", JSCLASS_HAS_PRIVATE,

Maybe name this "NPObject Ambigous Member class"?

+  // To support ambiguous members, we return NPObject Member class here.
+  if (hasProperty && hasMethod) {
+    NPP npp = LookupNPP(npobj);
+
+    if (!npp) {
+      ThrowJSException(cx, "No NPP found for NPObject!");
+
+      return JS_FALSE;
+    }

This npp lookup and check also exists further down this method, you could
simply pull that code up above this check and reduce the code size a bit.

- In CreateNPObjectMember():

+  if (!npobj || !npobj->_class || !npobj->_class->getProperty) {

You should check that npobj->_class->invoke() is non-null here too, or in
NPObjectMember_Call() if you prefer to do it there.

- In NPObjectMember_convert():

+    JS_ReportError(cx, "illegal operation on JSObject prototype object");

Prefix JS_Report... with '::'.

- In NPObjectMember_Call():

+  if (!JS_InstanceOf(cx, objp, &sNPObjectMemberClass, nsnull))

Add :: here too.

+  NPObjectMemberPrivate *memberPrivate =
+    (NPObjectMemberPrivate *)::JS_GetPrivate(cx, objp);

Actually, you could replace the JS_InstanceOf() and JS_GetPrivate() calls with
one call to JS_GetInstancePrivate(), like this:

  memberPrivate = ::JS_GetInstancePrivate(cx, obj, &sNPObjectMemberClass,
argv);

- In NPObjectMember_Mark():

+  memberPrivate = (NPObjectMemberPrivate *)JS_GetPrivate(cx, obj);

Add ::

r=jst with those changes. Thanks for doing all this work!

Comment 19

[Pete Zha]

Attachment: patch v5

Johnny, thanks for review. Please check this patch and give r if it is OK for
you.

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 189009 [details] [diff] [review]
patch v5

r=jst

Comment 21

[Pete Zha]

Comment on attachment 189009 [details] [diff] [review]
patch v5

Brendan, please review this patch. Thanks!

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 189009 [details] [diff] [review]
patch v5

CreateNPObjectMember:

- Nit: call the sNPObjectMemberClass instance "memobj" instead of "objp".

- If a last-ditch GC nests under JS_NewObject while trying to allocate the
  slots vector for the already-allocated new memobj (whose allocation just
  overwrote the cx->newborn[GCX_OBJECT] root), and if the fieldValue was
  of type object, requiring a new JSObject from nsNPObjWrapper::GetNewOrUsed,
  then that new object would be GC'd.

  The best way to handle this is to create memobj (objp in the patch) first,
  then set *vp = OBJECT_TO_JSVAL(memobj), call JS_AddRoot(cx, vp), then set
  memobj's private, then call npobj->_class->getProperty and NPVariantToJSVal
  to initialize the marked memberPrivate->fieldValue.  Don't return after
  making a successful JS_AddRoot call without calling JS_RemoveRoot, when
  you no longer need to protect *vp from being GC'd away.

NPObjectMember_convert:

- Nit: combine NPObjectMemberPrivate *memberPrivate and next line, which
  sets that variable, using an initializer (wrapped to the next line, of
  course).  Same nit later on, in a different method.

- There's no need to handle the !memberPrivate case here, you can assert
  that memberPrivate is non-null.  Nothing calls convert on an improperly
  constructed JSObject.

NPObjectMember_Call:

- Early Throw/return JS_FALSE in loop to "// Convert arguments" leaks
  npargs if npargs_buf wasn't big enough.

- Nit: NPVariant npv; instead of NPVariant v; (the latter name is used
  for jsvals).

How about another patch addressing these, then I'll sr= -- I'm assuming
you have tests working and can retest.	Thanks,

/be

Comment 23

[Pete Zha]

Attachment: patch v6

Thanks a lot, Brendan. This is a new patch addressing your comments, please
review.

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 189024 [details] [diff] [review]
patch v6

Noting jst's r=, stamping my sr=, requesting 1.8b4 approval.

/be

Comment 25

[Pete Zha]

Just checked in
http://tinderbox.mozilla.org/bonsai/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/modules/plugin/base/src&command=DIFF_FRAMESET&file=nsJSNPRuntime.h&rev1=1.5&rev2=1.6&root=/cvsroot
I will mark FIXD once the tree is OK with this patch.

Comment 26

[Pete Zha]

->FIXED