470291 - JavaScript delete operator does not invoke npruntime removeProperty on NPObject

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Al Patrick]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.4) Gecko/2008102920 Firefox/3.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.4) Gecko/2008102920 Firefox/3.0.4

In addition to the build identified above, I have checked the latest code in source control and this bug is still present.

The implementation of NPObjWrapper_DelProperty() in modules/plugin/base/src/nsJSNPRuntime.cpp is incomplete.

static JSBool
NPObjWrapper_DelProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
{
  NPObject *npobj = GetNPObject(cx, obj);

  if (!npobj || !npobj->_class || !npobj->_class->hasProperty) {
    ThrowJSException(cx, "Bad NPObject as private data!");

    return JS_FALSE;
  }

  if (!npobj->_class->hasProperty(npobj, (NPIdentifier)id)) {
    ThrowJSException(cx, "Trying to remove unsupported property on scriptable "
                     "plugin object!");

    return JS_FALSE;
  }

  return ReportExceptionIfPending(cx);
}

Notice that it does not call npobj->_class->removeProperty. Before checking for the pending exception, it should do something like this:

  JSBool ok = npobj->_class->removeProperty &&
              npobj->_class->removeProperty(npobj, (NPIdentifier)id);
  if (!ok) {
    // throw exception
  }

Compare with the implementation of NPObjWrapper_SetProperty, which calls the setProperty callback or NPObjWrapper_GetProperty which calls the getProperty callback.

Reproducible: Always

Steps to Reproduce:
1. In an npruntime plugin, create an NPObject implementing at least the hasProperty and removeProperty callbacks in its NPClass. The hasProperty function should return true for 'p'.
2. From JavaScript, use the delete operator to remove the property 'p' from the object:

var obj = plugin.getObjectImplementingRemoveProperty();
delete obj.p;
Actual Results:  
The removeProperty callback in not invoked. Therefore the plugin cannot remove the property. The property still exists on the object.

Expected Results:  
The removeProperty callback should be invoked, allowing the plugin to remove the property from the NPObject.

This bug prevents plugins that rely on the removeProperty callback from functioning correctly.

Comment 1

[Josh Aas]

Attachment: fix v1.0

Comment 2

[Al Patrick]

With regard to fix v1.0, I believe the semantics of JavaScript is to have the delete operator return false if a property cannot be removed rather than throwing an exception. It isn't clear to me whether returning false from NPClass::removeProperty should signify an error or simply that the property does not exist. From my point of view, I would prefer the latter (just return the result of NPClass::removeProperty as the result of the JavaScript delete operator). Likewise, in this case, if NPClass::hasProperty returns false, I would prefer that to simply be returned as the result of the JavaScript delete operator rather than throw an exception.

I think the only cases where an exception should be thrown is if removeProperty (or maybe hasProperty) is null or if the plugin sets an exception. But is there any need to call hasProperty at all?

Of course, my preference may not reflect the best general approach for other npruntime plugins.

Comment 3

[Josh Aas]

Interesting, I don't know what the right thing to do is. Johnny?

Comment 4

[Johnny Stenback (:jst)]

Not blocking on this given how rarely delete is used in JS, and nobody apparently noticed before now.

Comment 5

[Nochum Sossonko [:Natch]]

delete is rare in JS? I've seen it being used all over (especially for pseudo-arrays or array-like objects, instead of pop you use delete). Probably doesn't change the blocking- decision though...

Comment 6

[Johnny Stenback (:jst)]

Yeah, I'm not saying it's not used, but I've never seen anyone use delete on a *plugin* object. Either way, I think we'll end up fixing this for Firefox 3.1, just sayin' it's not something we'd block the release for.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 357232 [details] [diff] [review]
fix v1.0

   if (!npobj->_class->hasProperty(npobj, (NPIdentifier)id)) {
     ThrowJSException(cx, "Trying to remove unsupported property on scriptable "
                      "plugin object!");
 
     return JS_FALSE;
   }

Al is right, we shouldn't throw here, we should just return JS_TRUE since that's just how JS works.

+  if (!npobj->_class->removeProperty(npobj, (NPIdentifier)id)) {
+    ThrowJSException(cx, "Removing property failed on scriptable plugin "
+                     "object!");
+
+    return JS_FALSE;
+  }

And here we should just set *rval to BOOLEAN_TO_JSVAL(npobj->_class->removeProperty(...)) so that if removeProperty returns false for some reason, we'll return false to the JS code, and then we'll only throw below if the plugin reported an exception. Fix that, and I'll r+sr...

Comment 8

[Josh Aas]

Attachment: fix v1.1

Johnny - I think this is what you're suggesting, though I'm not sure what "*rval" you're referring to. This makes sense to me.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 357309 [details] [diff] [review]
fix v1.1

+  if (!npobj->_class->hasProperty(npobj, (NPIdentifier)id))
+    return JS_TRUE;

That's all good...

+  if (!npobj->_class->removeProperty(npobj, (NPIdentifier)id))
     return JS_FALSE;

But that return line need to be replaced by this (i.e. no more early return here):

  *rval = JSVAL_FALSE;

(*rval is the return value that the running *JS* sees from the delete operation. The value returned here in C, is the success code to the JS engine, which is different from the return value seen by the running JS.)

r+sr+a=jst with that changed.

Comment 10

[Josh Aas]

My previous confusion stems from the fact that there is no *rval in the function we're talking about. Perhaps you mean *vp?

Comment 11

[Josh Aas]

Attachment: fix v1.2

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 357352 [details] [diff] [review]
fix v1.2

Yes, *vp was of course what I meant. I was reading JS engine code to verify what I was saying here, and it uses rval instead of vp. Sorry about that, and thanks for fixing!

Comment 13

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/5c93998e6f75

Comment 14

[Al Patrick]

Perhaps I'm not quite following here but could it be fixed like this:

  if (!npobj->_class->hasProperty(npobj, (NPIdentifier)id)) {
    *vp = JSVAL_TRUE; // return true to JS
    return ReportExceptionIfPending(cx); // plugin may have set an exception
  }

  if (npobj->_class->removeProperty(npobj, (NPIdentifier)id))
    *vp = JSVAL_TRUE; // return true to JS
  else
    *vp = JSVAL_FALSE; // return false to JS
 
  // plugin may have set an exception
  return ReportExceptionIfPending(cx);

I think that unless the plugin throws an exception, this function should always set the result returned to the JS language. As fix v1.2 stands, in some cases it will either be a default value or not defined (haven't checked the callers to see which). Also, if hasProperty sets an exception (it would be unusual but it's possible), then it should it not be immediately be thrown into JS, rather than returning successfully?

Comment 15

[Josh Aas]

pushed to mozilla-1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bfe1fd8294b9

Comment 16

[Josh Aas]

Al - it seems like you're right about *vp not being set consistently, Johnny? If you are right then you should file another bug. Thanks for the feedback.

Comment 17

[Jeff Walden [:Waldo]]

We can test this now!!!1!!cos(0)!!!

Comment 18

[Johnny Stenback (:jst)]

(In reply to comment #16)
> Al - it seems like you're right about *vp not being set consistently, Johnny?
> If you are right then you should file another bug. Thanks for the feedback.

Al, *rval always comes in set to JSVAL_TRUE, so no need to ever set it to that (see http://hg.mozilla.org/mozilla-central/file/3c7610653ad2/js/src/jsobj.cpp#l4178).

But yeah, we should probably report exceptions if hasProperty() ever sets one. Can you please file a new bug on that? Thanks!

Comment 19

[Al Patrick]

I filed a new bug (a minor one) as you requested. See https://bugzilla.mozilla.org/show_bug.cgi?id=474157