Plugin Package function pointers are set up wrong

• Component: Plug-ins
• Status: RESOLVED FIXED

[Mihnea Ovidenie]

Hello,

In file PluginPackageMac.cpp, function load, two function pointers are set in wrong way:

m_browserFuncs.hasproperty = _NPN_HasMethod; 
m_browserFuncs.hasmethod = _NPN_HasProperty;

The right way to set them should be:
m_browserFuncs.hasproperty = _NPN_HasProperty; 
m_browserFuncs.hasmethod = _NPN_HasMethod;

Also, the same problem can be found in PluginPackageGtk.cpp and PluginPackageQt.cpp.

Regards,
Mihnea

[Mihnea Ovidenie]

Created attachment 28483 [details]
First version of patch

Made a small patch by setting the function pointers in the right way. The patch does not contain any test for the moment. Is there any layout test necessary for such patch?

Regards,
Mihnea

[Darin Adler]

Comment on attachment 28483 [details]
First version of patch

We do have plug-in tests in our regression test suite. So you could indeed make a regression test for this, and I think it would be a good idea.

[Mihnea Ovidenie]

(In reply to comment #2)
> (From update of attachment 28483 [details] [review])
> We do have plug-in tests in our regression test suite. So you could indeed make
> a regression test for this, and I think it would be a good idea.
> 

This issue is already fixed in the mac version of WebKit. Currently it affects only Qt/Gtk builds. Can i build Qt/Gtk on Mac in order to test a possible unit test?

[Simon Hausmann]

Your patch looks great!

The Qt build currently does not build the test netsacpe plugin for the plugin layout tests, but the Gtk port does. (and they seem to all pass except for one test)

[Eric Seidel (no email)]

Comment on attachment 28483 [details]
First version of patch

This definitely needs a test case. I assume it should be possible to modify test_plugin in DumpRenderTree to test this?

[Holger Freyther]

I agree with Eric, I'm in the process of creating a test...

[Mihnea Ovidenie]

(In reply to comment #6)
> I agree with Eric, I'm in the process of creating a test...
> 

Thanks, time for me to learn :)

[Holger Freyther]

Created attachment 30332 [details]
Bugfix + TestCase

While doing the test case I found that _NPN_HasMethod is a bit too liberal in declaring things as a function. This was changed within the patch, but I'm not sure about the correctness.

[Anders Carlsson]

Comment on attachment 30332 [details]
Bugfix + TestCase

r=me on everything except the _NPN_HasMethod change. Let's file a separate bug about that.

[Holger Freyther]

I created bug #25891 for the NPN_HasMethod issue.

[Holger Freyther]

Landed in r43923.