296159 - npruntime API does not support enumeration

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Pete Zha]

For backward compatibility, java plugin need this ability to support enumerate
the members of java object.

Comment 1

[Pete Zha]

Seems we have following things to do:
1) Implement enumerate function in sNPObjectJSWrapperClass. So that JE engine
could call it to enumerate sNPObjectJSWrapperClass
2) Need to a function to NPClass so that sNPObjectJSWrapperClass could enumerate
NPObject and return to JSEngine.

But, from my investigation, there are some problems to do that:
In sNPObjectJSWrapperClass, we don't define JSObjectOps. So, we have to define
all the property to JSObject. It's not reasonable. jst, can we define a
JSObjectOps and only implement the newEnumerate function?

Comment 2

[Pete Zha]

update dependency

Comment 3

[Pete Zha]

->Alfred who is working on this

Comment 4

[Alfred Peng]

Attachment: patch v1

The patch adds the "enumerate" function to NPAPI+, and modifies the file
nsJSNPRuntime.cpp accordingly to implement enumeration.

Comment 5

[Johnny Stenback (:jst)]

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

+#define JSJ_SLOT_COUNT (JSSLOT_PRIVATE+1)

What's up with this? Doesn't seem related to enumeration support...

- In NPObjWrapper_newEnumerate():

+  if (!elementArray) {
+    NPVariant result;
+
+    if (!npobj->_class->enumerate(npobj, &result))
+      return JS_FALSE;
+	 
+    elementArray = new nsCStringArray();
+    NS_ENSURE_TRUE(elementArray, JS_FALSE);
+
+    elementArray->ParseString(NPVARIANT_TO_STRING(result).utf8characters, "
");

This API won't fly, there's nothing that prevents properties with space
characters in the name, nor any other character either for that matter ('\0',
you name it, all goes).

The NPRuntime enumeration hook would need to return an array of NPIdentifiers,
null terminated, or we need an additional count out param.

r- until that's fixed.

Comment 6

[Alfred Peng]

Attachment: Patch v2

Thanks for you review, Johnny.
This is the modified one, please review it again.

Comment 7

[Alfred Peng]

(In reply to comment #5)
> (From update of attachment 188524 [details] [diff] [review] [edit])
> +#define JSJ_SLOT_COUNT (JSSLOT_PRIVATE+1)
> 
> What's up with this? Doesn't seem related to enumeration support...
> 
The MACRO defines the initial number of slots in JSObject in my patch. I had thought that proto, parent, class and private properties would occupy these slots, and 4 slots were enough. I also notice that there is another MACRO JS_INITIAL_NSLOTS defines the initial number. For iterator object, the iterator index will occupy another one. So maybe I can use JS_INITIAL_NSLOTS instead of defining a new one. Any comments? Johnny

Comment 8

[Alfred Peng]

Attachment: patch v3

The patch extends the NPClass API, implements the enumeration method of JSClass to support enumeration.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

doesn't this break API compatibility? (i.e. does this not need to check that the structVersion field is > 1?)

Comment 10

[Alfred Peng]

Yes, that patch will affect the API compatibility.
I'm not sure whether the NPRuntime API has already been marked as stable or still under development. jst, can you help me make this clear?
(In reply to comment #9)
> doesn't this break API compatibility? (i.e. does this not need to check that
> the structVersion field is > 1?)
> 

Comment 11

[Johnny Stenback (:jst)]

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

As far as the code you've written here so far this is definately going in the right direction, but this only implements one side of this problem, it makes NPObject's enumerable from JS, but it doesn't make JS objects enumerable from plugin code through this same API. To do that you'd need to implement the enumerate hook in nsJSObjWrapper::sJSObjWrapperNPClass, and add an enumerate entry to NPNetscapeFuncs for plugins to call when they want to enumerate properties on an NPObject.

Also, to add this new enumerate hook to NPClass, we need to update the version number of the class (NP_CLASS_STRUCT_VERSION in npruntime.h), and you'll probably want to add a #define NP_CLASS_STRUCT_VERSION_HAS_ENUM() macro or somesuch too). And to add to the NPNetscapeFuncs struct you'll need to update the version of the NPAPI (NP_VERSION_MINOR in npapi.h).

With the above added, and buy-in from the plugin-futures list this is really close to being ready for checkin. I'd suggest you email the plugin-futures list asap to give people some time to think about this.

Here's some comments on the patch so far (but I'm marking it r- due to the above missing parts and some problems with the code so far):

- In NPObjWrapper_newEnumerate():

+  static uint32_t i;
+  static uint32_t index;

These can't be static, this info needs to live in *statep while enumerating, or else nested enumerations etc won't work right. Generally speaking you'll need to create a new struct that can hold the current index and the array of NPIdentifiers and malloc one of those when initializing the enumeration operation, and then using this until we're done enumerating.

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

This is an unsafe check, npobj->_class only has an enumerate member if the class comes from a plugin with enumeration support. Here you'll need to check that npobj->_class->structVersion is the new version number (using the proposed NP_CLASS_STRUCT_VERSION_HAS_ENUM() macro) before looking at the enumerate member. If the plugin doesn't support enumeration, you'll need to return w/o causing an error, i.e. return JS_TRUE.

+  enum_value = (NPIdentifier *)JSVAL_TO_PRIVATE(*statep);
+  if (!enum_value) {
+    if (!npobj->_class->enumerate(npobj, &enum_value, &enum_count))
+      return JS_FALSE;
+  }

This code should be in the JSENUMERATE_INIT case.

+  case JSENUMERATE_INIT:
+    if (idp)
+      *idp = INT_TO_JSVAL(enum_count);
+      *statep = PRIVATE_TO_JSVAL(enum_value);
+      i = 0;
+      index = enum_count;
+      break;

The indentation here makes it look like all this code is in the if (idp) check, but it's not. Fix the indentation.

+  case JSENUMERATE_NEXT:
+    if (i < index) {

i and index here need to come from the new struct that holds the state of this enumeration...

+      ::JS_ValueToId(cx, (jsval)enum_value[i++], idp);
+      break;
+    }
+
+    *statep = JSVAL_NULL;

Why null out *statep here?

Comment 12

[Alfred Peng]

(In reply to comment #11)
> NP_CLASS_STRUCT_VERSION_HAS_ENUM() macro) before looking at the enumerate
> member. If the plugin doesn't support enumeration, you'll need to return w/o
> causing an error, i.e. return JS_TRUE.
If the version number of the plugin shows that it support enumeration, but the enumerate method of its NPClass struct is NULL, we need to return JS_TRUE also I think.

> 
> +  enum_value = (NPIdentifier *)JSVAL_TO_PRIVATE(*statep);
> +  if (!enum_value) {
> +    if (!npobj->_class->enumerate(npobj, &enum_value, &enum_count))
> +      return JS_FALSE;
> +  }
The return value of enum_value is an array of NPIdentifiers. The memory space is alloced by the enumerate method of NPClass, but it will be freed by NPObjWrapper_newEnumerate method of NPRuntime. The pairs of memory management isn't in the same function. Can we accept this? Or we should add some comments on this API?

For the other comments, I've fixed them in my new patch. And I want to do some test first and post it here.

Comment 13

[Johnny Stenback (:jst)]

(In reply to comment #12)
> If the version number of the plugin shows that it support enumeration, but the
> enumerate method of its NPClass struct is NULL, we need to return JS_TRUE also
> I think.

Yes, I agree.

> > +  enum_value = (NPIdentifier *)JSVAL_TO_PRIVATE(*statep);
> > +  if (!enum_value) {
> > +    if (!npobj->_class->enumerate(npobj, &enum_value, &enum_count))
> > +      return JS_FALSE;
> > +  }
> The return value of enum_value is an array of NPIdentifiers. The memory space
> is alloced by the enumerate method of NPClass, but it will be freed by
> NPObjWrapper_newEnumerate method of NPRuntime. The pairs of memory management
> isn't in the same function. Can we accept this? Or we should add some comments
> on this API?

We should add comments. The general memory management functions for plugins is to use memalloc amd memfree in NPNetscapeFuncs, and in Mozilla that maps to nsMemory::Free() so you could use that here.

Comment 14

[Alfred Peng]

Attachment: Patch v4

The new patch based on jst's last review.

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 208262 [details] [diff] [review]
Patch v4

- In modules/plugin/base/public/npruntime.h

+    The NPEnumerationFunctionPtr function may pass an array of
+    NPIdentifiers back to the caller. The callee allocs the memory of
+    the array, and it's the caller's responsibility to release it.

Make that:

  The NPEnumerationFunctionPtr function may pass an array of
  NPIdentifiers back to the caller. The callee allocs the memory of
  the array using NPN_MemAlloc(), and it's the caller's responsibility
  to release it using NPN_MemFree().

-#define NP_CLASS_STRUCT_VERSION 1
+#define NP_CLASS_STRUCT_VERSION 2
+
+#define NP_CLASS_STRUCT_VERSION_HAS_ENUM(npobj)   \
+        ((npobj)->_class->structVersion == NP_CLASS_STRUCT_VERSION)

To make life easier if there are more additions to this API in the future, hard code the new numer into the NP_CLASS_STRUCT_VERSION_HAS_ENUM macro and do a >= compare, i.e. either:

+#define NP_CLASS_STRUCT_VERSION_HAS_ENUM(npobj)   \
+        ((npobj)->_class->structVersion >= 2)

or add also a NP_CLASS_STRUCT_VERSION_ENUM that's defined to be the actual version (2) where enum support was added and use that in the NP_CLASS_STRUCT_VERSION_HAS_ENUM() macro. Oh, and it seems like it'd be cleaner to make this API take a NPClass * argument, not an NPObject * argument.

- In modules/plugin/base/src/ns4xPlugin.cpp

+bool NP_EXPORT
+_enumerate(NPP npp, NPObject *npobj, NPIdentifier **identifier,
+           uint32_t *count)
+{
+  if (!npp || !npobj || !npobj->_class || !npobj->_class->enumerate)
+    return false;

You'll need to use NP_CLASS_STRUCT_VERSION_HAS_ENUM here too before checking if npobj->_class->enumerate is non-null.

- In nsJSObjWrapper::NP_Enumerate():

+  ida = ::JS_Enumerate(cx, npjsobj->mJSObj);
[...]
+  for (i = 0; i < *count; i++) {
+    if (!::JS_IdToValue(cx, ida->vector[i], &v)) {
+      ::JS_DestroyIdArray(cx, ida);
+      PR_Free(*identifier);
+      return PR_FALSE;
+    }
+
+    (*identifier)[i] = JSVAL_TO_PRIVATE(v);
+  }

There's a memory ownership problem here (or two, actually, but only one that's at all likely to ever happen). The problem is that you're returning the jsval's in the loop as NPIdentifiers w/o making sure the jsval's won't get GC'd. To do that you should check if the jsval is a string (it it's either a string or a number), and if it is, call JS_InternUCStringN() to intern the JSString value in the jsval. That will guarantee that the NPIdentifier will remain valid as long as the plugin uses it (until mozilla is shut down, in fact).

+typedef struct NPObjectEnumerateState {
+  uint32_t      index;
+  uint32_t      length;
+  NPIdentifier *value;
+} NPObjectEnumerateState;

No need for the typedef here, this is C++ code.

- In NPObjWrapper_newEnumerate():

+  case JSENUMERATE_INIT:
+    if (!npobj->_class->enumerate(npobj, &enum_value, &length))
+      return JS_FALSE;
+
+    state = (NPObjectEnumerateState *)
+        PR_Malloc(sizeof(NPObjectEnumerateState));

You could simply use "state = new NPObjectEnumerateState();" here, and use delete to free this, of course.

+  case JSENUMERATE_NEXT:
+    state = (NPObjectEnumerateState *)JSVAL_TO_PRIVATE(*statep);
+    enum_value = state->value;
+    length = state->length;
+    if (state->index != length) {
+      ::JS_ValueToId(cx, (jsval)state->value[state->index++], idp);

Since you set enum_value above, use it here.

Other than that this is looking really good. Fix that, and I'll r+ it (or sr+ if someone else reviews, [e-mail] would be a good candidate).

Comment 16

[Alfred Peng]

Attachment: Patch v5

Another patch based on jst's last review.

jst, thanks for your careful review.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 208365 [details] [diff] [review]
Patch v5

Note, I'm taking much of this patch for granted, since I'm not terribly familiar with this plugin code.

Index: modules/plugin/base/src/nsJSNPRuntime.cpp
>+bool
>+nsJSObjWrapper::NP_Enumerate(NPObject *npobj, NPIdentifier **identifier,
>+                             uint32_t *count)
>+{

I'm not very familiar with this code, but I notice that this returns |bool| instead of |PRBool|. Is this intentional? If so, why return PR_* in the function?

>+JS_STATIC_DLL_CALLBACK(JSBool)
>+NPObjWrapper_newEnumerate(JSContext *cx, JSObject *obj, JSIterateOp enum_op,
>+                          jsval *statep, jsid *idp)
>+{
>...
>+  if (!NP_CLASS_STRUCT_VERSION_HAS_ENUM(npobj->_class) ||
>+      !npobj->_class->enumerate)
>+    return JS_TRUE;

Here *statep is left at JSVAL_NULL (the default) and we continue enumerating the first time around (since the JS engine doesn't check for statep nullness between the INIT and first NEXT cases)...

>+  NS_ENSURE_TRUE(statep, JS_FALSE);

This could probably be an assertion. As-is, it'll cause a silent error if this case ever does happen.

>+  case JSENUMERATE_NEXT:
>+    state = (NPObjectEnumerateState *)JSVAL_TO_PRIVATE(*statep);
>+    enum_value = state->value;

So if this class doesn't support enumeration, state will be NULL here, and you'll crash. Checking for |*statep == JSVAL_NULL| should save you.

>+    length = state->length;
>+    if (state->index != length) {
>+      ::JS_ValueToId(cx, (jsval)enum_value[state->index++], idp);

If this fails, you won't successfully propagate the failure. Easily fixed by making this: |return ::JS_ValueToId(...);| and removing the |break;|

>+      break;
>+    }
>+    /* FALL THROUGH */
>+
>+  case JSENUMERATE_DESTROY:
>+    state = (NPObjectEnumerateState *)JSVAL_TO_PRIVATE(*statep);
>+    PR_Free(enum_value);

If there was an error enumerating, then this frees enum_value before you assign to it (since you won't go through the JSENUMERATE_NEXT and be falling through, you need to set enum_value or just |PR_Free(state->value);|. Marking r- because of the crashes and such.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #17)
> So if this class doesn't support enumeration, state will be NULL here, and
> you'll crash. Checking for |*statep == JSVAL_NULL| should save you.

Or even better: when enumeration isn't supported, set statep to be an NPObjectEnumerateState with index == value == 0, and things will just take care of themselves.

Comment 19

[Alfred Peng]

Blake, I've got the following questions on some of your comments.

(In reply to comment #17)
> >+bool
> >+nsJSObjWrapper::NP_Enumerate(NPObject *npobj, NPIdentifier **identifier,
> >+                             uint32_t *count)
> >+{
> 
> I'm not very familiar with this code, but I notice that this returns |bool|
> instead of |PRBool|. Is this intentional? If so, why return PR_* in the
> function?

The other functions in the NPClass structure "sJSObjWrapperNPClass" do the same way. If that isn't reasonable, maybe I should make some small changes to the whole file.
jst, what's your opinion?

> >+JS_STATIC_DLL_CALLBACK(JSBool)
> >+NPObjWrapper_newEnumerate(JSContext *cx, JSObject *obj, JSIterateOp enum_op,
> >+                          jsval *statep, jsid *idp)
> >+{
> >...
> >+  if (!NP_CLASS_STRUCT_VERSION_HAS_ENUM(npobj->_class) ||
> >+      !npobj->_class->enumerate)
> >+    return JS_TRUE;
> 
> Here *statep is left at JSVAL_NULL (the default) and we continue enumerating
> the first time around (since the JS engine doesn't check for statep nullness
> between the INIT and first NEXT cases)...
> >+  case JSENUMERATE_NEXT:
> >+    state = (NPObjectEnumerateState *)JSVAL_TO_PRIVATE(*statep);
> >+    enum_value = state->value;
> 
> So if this class doesn't support enumeration, state will be NULL here, and
> you'll crash. Checking for |*statep == JSVAL_NULL| should save you.

I go through the code in function "js_Enumerate". The enumeration will get into INIT first. If some error happens in the INIT block, the function returns false. NEXT won't be called for this object I think.
If the NEXT can be called without invoking INIT correctly, that will be a big problem. Is that possible? If not, I'm not sure whether we should add the check for *statep.

> >+  NS_ENSURE_TRUE(statep, JS_FALSE);
> 
> This could probably be an assertion. As-is, it'll cause a silent error if this
> case ever does happen.
> 
> 

Assertion is useful for debugging. But mozilla will continue without DEBUG enable when the assertion fails. That will crash mozilla in the following code.
Maybe we should keep the "NS_ENSURE_TRUE" here and add another assertion above it.

For the other comments, I'll change accordingly in the next patch. Thanks for your review. 

Comment 20

[Alfred Peng]

I've discussed with Blake. Some new information is provided below.

(In reply to comment #19)
> (In reply to comment #17)
> > >+bool
> > >+nsJSObjWrapper::NP_Enumerate(NPObject *npobj, NPIdentifier **identifier,
> > >+                             uint32_t *count)
> > >+{
> > 
> > I'm not very familiar with this code, but I notice that this returns |bool|
> > instead of |PRBool|. Is this intentional? If so, why return PR_* in the
> > function?
> 
> The other functions in the NPClass structure "sJSObjWrapperNPClass" do the same
> way. If that isn't reasonable, maybe I should make some small changes to the
> whole file.
> jst, what's your opinion?

For the PRBool/bool issue, it won't be a problem.
 
> > >+JS_STATIC_DLL_CALLBACK(JSBool)
> > >+NPObjWrapper_newEnumerate(JSContext *cx, JSObject *obj, JSIterateOp enum_op,
> > >+                          jsval *statep, jsid *idp)
> > >+{
> > >...
> > >+  if (!NP_CLASS_STRUCT_VERSION_HAS_ENUM(npobj->_class) ||
> > >+      !npobj->_class->enumerate)
> > >+    return JS_TRUE;
> > 
> > Here *statep is left at JSVAL_NULL (the default) and we continue enumerating
> > the first time around (since the JS engine doesn't check for statep nullness
> > between the INIT and first NEXT cases)...

This enumeration check should be moved into INIT block.

> > >+  case JSENUMERATE_NEXT:
> > >+    state = (NPObjectEnumerateState *)JSVAL_TO_PRIVATE(*statep);
> > >+    enum_value = state->value;
> > 
> > So if this class doesn't support enumeration, state will be NULL here, and
> > you'll crash. Checking for |*statep == JSVAL_NULL| should save you.
> 
> I go through the code in function "js_Enumerate". The enumeration will get into
> INIT first. If some error happens in the INIT block, the function returns
> false. NEXT won't be called for this object I think.
> If the NEXT can be called without invoking INIT correctly, that will be a big
> problem. Is that possible? If not, I'm not sure whether we should add the check
> for *statep.

By returning true from the INIT block after moving the check into INIT, *statep can be JSVAL_NULL in the NEXT block. It will crash. Setting index == value == 0 can help on this.

> > >+  NS_ENSURE_TRUE(statep, JS_FALSE);
> > 
> > This could probably be an assertion. As-is, it'll cause a silent error if this
> > case ever does happen.
> > 
> > 
> 
> Assertion is useful for debugging. But mozilla will continue without DEBUG
> enable when the assertion fails. That will crash mozilla in the following code.
> Maybe we should keep the "NS_ENSURE_TRUE" here and add another assertion above
> it.

statep shouldn't be NULL except there is a bug in JS Engine. For future debugging usage, add the assertion here.

Comment 21

[Alfred Peng]

Attachment: Patch v6

New patch based on Blake's review.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 209126 [details] [diff] [review]
Patch v6

>+nsJSObjWrapper::NP_Enumerate(NPObject *npobj, NPIdentifier **identifier,
>+                             uint32_t *count)
>+{
...
>+  *identifier = (NPIdentifier *)PR_Malloc(*count * sizeof(NPIdentifier));
>+  if (!*identifier) {
>+    ::JS_DestroyIdArray(cx, ida);
>+    return PR_FALSE;
>+  }

The way you've written things here, these Enumerate callbacks are responsible for making sure that JS exceptions get thrown in failure cases. Please make sure this happens, or scripts will terminate without warning (and without being able to catch the exception). Other than that, this seems much better to me, so r=mrbkap.

Comment 23

[Alfred Peng]

Attachment: Patch v7

To compare with patch v6, this patch add the "ThrowJSException" function call to three error return places.

Comment 24

[Alfred Peng]

Attachment: Patch to throw JSException

There are some places in the code where JS exception should be thrown in failure cases. As Blake suggested, I make some changes to the whole file to throw JS exception.

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 209192 [details] [diff] [review]
Patch v7

- In _enumerate(NPP npp, NPObject *npobj, NPIdentifier **identifier,
+           uint32_t *count)

+  if (!NP_CLASS_STRUCT_VERSION_HAS_ENUM(npobj->_class) ||
+      !npobj->_class->enumerate)
+    return true;

Since this is a perfectly valid (though unlikely) case, I think we should make sure we set *count and *identifier to 0 here to ensure we don't have code assuming it got back some identifiers if it passed in uninitialized variables here (which is perfectly fine).

- In nsJSObjWrapper::NP_Enumerate():

+{
+  NPP npp = NPPStack::Peek();
+  JSContext *cx = GetJSContext(npp);
+  uint32_t i;
+  JSIdArray *ida;
+  jsval v;
+
+  if (!cx || !npobj)
+    return PR_FALSE;

Probably a good idea to initialize *ount and *identifiers to 0 at the beginning of this method too.

+    if (JSVAL_IS_STRING(v)) {
[...]
+    } else
+      NS_ASSERTION(JSVAL_IS_INT(v),
+                   "The element in ida must be either string or int!\n");

Please put braces around that multi-line else block.

+    (*identifier)[i] = JSVAL_TO_PRIVATE(v);

To match what's done in _getintidentifier() etc this should be:

+    (*identifier)[i] = (NPIdentifier)v;

sr=jst with that fixed.

Comment 26

[Alfred Peng]

(In reply to comment #25)
> (From update of attachment 209192 [details] [diff] [review] [edit])
> - In _enumerate(NPP npp, NPObject *npobj, NPIdentifier **identifier,
> +           uint32_t *count)
> 
> +  if (!NP_CLASS_STRUCT_VERSION_HAS_ENUM(npobj->_class) ||
> +      !npobj->_class->enumerate)
> +    return true;
> 
> Since this is a perfectly valid (though unlikely) case, I think we should make
> sure we set *count and *identifier to 0 here to ensure we don't have code
> assuming it got back some identifiers if it passed in uninitialized variables
> here (which is perfectly fine).
> 
Should we set *count and *identifier to 0 here or at the beginning of this method?

Comment 27

[Alfred Peng]

Attachment: Patch v8

The new patch based on jst's last review.

jst, I don't quite understand what's the difference between
    (*identifier)[i] = JSVAL_TO_PRIVATE(v);
and
    (*identifier)[i] = (NPIdentifier)v;

I change another two places where "JSVAL_TO_PRIVATE" is used to "(NPIdentifier)" in my patch. It will crash.

If the patch is ok for you, please help me to check it in. Thanks.

Comment 28

[Johnny Stenback (:jst)]

(In reply to comment #27)
> jst, I don't quite understand what's the difference between
>     (*identifier)[i] = JSVAL_TO_PRIVATE(v);
> and
>     (*identifier)[i] = (NPIdentifier)v;

JSVAL_TO_PRIVATE is a cast and an and operation to strip out the low bit in the value (JSVAL_INT). What we want here is to simply pass the jsval (v) as an NPIdentifier, which we need to do w/o loosing any bits. Actually, using JSVAL_TO_PRIVATE here would mean that int identifers would never come out right when enumerating, since the macro takes out the low bit in the jsval that makes the jsval an int value.

> I change another two places where "JSVAL_TO_PRIVATE" is used to
> "(NPIdentifier)" in my patch. It will crash.

I'm assuming those were the two remaing ones, the ones in NPObjWrapper_newEnumerate() where we get our own malloced NPObjectEnumerateState object out of *statep. In that case we *do* need to use JSVAL_TO_PRIVATE since the value when put into *statep was put in there with PRIVATE_TO_JSVAL, which does the opposite, it sets the low bit in the value. And if you try to use a pointer to an NPObjectEnumerateState object with the low bit set, you will crash (since the address you're using is off by one bit from the address you want).

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

+++ modules/plugin/base/public/npupp.h	26 Jan 2006 05:48:35 -0000
@@ -1710,6 +1739,7 @@
     NPN_RemovePropertyUPP removeproperty;
     NPN_HasPropertyUPP hasproperty;
     NPN_HasMethodUPP hasmethod;
+    NPN_EnumerateUPP enumerate;
     NPN_ReleaseVariantValueUPP releasevariantvalue;
     NPN_SetExceptionUPP setexception;
     NPN_PushPopupsEnabledStateUPP pushpopupsenabledstate;

Hm... shouldn't this be added at the end of the struct, to ensure binary compatibility?

Comment 30

[timeless]

yes please.

Comment 31

[Alfred Peng]

Attachment: Patch v9

Add the "enumerate" API to the end of struct "NPNetscapeFuncs" to keep binary
compatibility.

Comment 32

[Johnny Stenback (:jst)]

Attachment: Patch that was checked in.

I just checked this patch in. This is both of Alfred's patches combined into one, with some minor style tweaks that crept in.

With this, I believe we're done with this bug. Thank you Alfred for all the work to get this completed! And please close this bug unless there's something left to do here.

Comment 33

[Alfred Peng]

The tree is ok for the patch. Thanks again for your reviews.

=> FIXED

Comment 34

[Doug Shelton]

patch seems to have broken BeOS builds.  bug 328989 opened for regression.