Closed Bug 467945 Opened 11 years ago Closed 11 years ago

npruntime NPN_HasMethod forwards to wrong plugin function (hasProperty)

Categories

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

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1b3

People

(Reporter: apatrick, Assigned: jaas)

Details

(Keywords: fixed1.9.1)

Attachments

(1 file)

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.
Component: General → Plug-ins
Product: Firefox → Core
QA Contact: general → plugins
Assignee: nobody → joshmoz
Attached patch fix v1.0Splinter Review
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.
Attachment #357174 - Flags: superreview?(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!
Attachment #357174 - Flags: superreview?(jst)
Attachment #357174 - Flags: superreview+
Attachment #357174 - Flags: review+
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: blocking1.9.1+
Priority: -- → P1
Target Milestone: --- → mozilla1.9.1b3
Attachment #357174 - Flags: approval1.9.1?
Can the test plugin check for this?
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/084a18e871f8
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Yes, the test plugin can check for this and it will once I check in a set of basic npruntime tests.
Comment on attachment 357174 [details] [diff] [review]
fix v1.0

(a+ not needed for blockers)
pushed to mozilla-1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2c381e5c48f8
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.