467945 - npruntime NPN_HasMethod forwards to wrong plugin function (hasProperty)

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

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 in the report, I looked at the latest code in hg and the bug is still there.

When a plugin invokes the NPN_HasMethod() function on an NPObject, it is forwarded to that object's hasProperty() function instead of hasMethod(). I tracked the problem down to a function in modules/plugin/base/src/nsNPAPIPlugin.cpp. The function _hasMethod() reads:

bool NP_CALLBACK
_hasmethod(NPP npp, NPObject* npobj, NPIdentifier methodName)
{
  if (!NS_IsMainThread()) {
    NPN_PLUGIN_LOG(PLUGIN_LOG_ALWAYS,("NPN_hasmethod called from the wrong thread\n"));
    return false;
  }
  if (!npp || !npobj || !npobj->_class || !npobj->_class->hasMethod)
    return false;

  NPPExceptionAutoHolder nppExceptionHolder;
  NPPAutoPusher nppPusher(npp);

  NPN_PLUGIN_LOG(PLUGIN_LOG_NOISY,
                 ("NPN_HasMethod(npp %p, npobj %p, property %p) called\n",
                  npp, npobj, methodName));

  return npobj->_class->hasProperty(npobj, methodName);
}

Notice the last line forwards to hasProperty(). It should be:

  return npobj->_class->hasMethod(npobj, methodName);

If this is intentional (perhaps a workaround, but if so I hope there's a better way!), then there is still a bug. In that case, the test near the beginning of the function should be:

  if (!npp || !npobj || !npobj->_class || !npobj->_class->hasProperty)
    return false;


Reproducible: Always

Steps to Reproduce:
1. From a plugin using npruntime, create an NPObject with a class that implements hasProperty and hasMethod
2. Invoke NPN_HasMethod on that object

Actual Results:  
The object's hasMethod is not called. hasProperty is called instead.

Expected Results:  
Only hasProperty should be called.

Comment 1

[Josh Aas]

Attachment: fix v1.0

This is a disturbing bug, either this is on purpose and I don't see the logic or it is really strange we got away with this for so long.

Comment 2

[Johnny Stenback (:jst)]

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

I don't think this is anything other than a stupid mistake of mine. But a disturbing one at that indeed. I think we just need to get this in *now* and get it in for beta3 to get as much testing on this as we can. The later we fix this the harder it'll be to ever fix it, so I say let's get this in!

Comment 3

[Robert Sayre]

Can the test plugin check for this?

Comment 4

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/084a18e871f8

Comment 5

[Josh Aas]

Yes, the test plugin can check for this and it will once I check in a set of basic npruntime tests.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

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

(a+ not needed for blockers)

Comment 7

[Josh Aas]

pushed to mozilla-1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2c381e5c48f8