Closed Bug 344425 Opened 13 years ago Closed 13 years ago

support NP_GetEntryPoints as a way to get plugin function pointers

Categories

(Core :: Plug-ins, enhancement)

PowerPC
macOS
enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jaas, Assigned: jaas)

Details

Attachments

(1 file, 1 obsolete file)

5.82 KB, patch
sayrer
: review+
mikepinkerton
: superreview+
Details | Diff | Splinter Review
When we load a plugin, we get function pointers from it by calling its "main()" function. This is a bit antiquated, WebKit has support for getting function pointers via a function called NP_GetEntryPoints. We should try to get function pointers from there instead of main() if possible, that way it'll be easier for plugin authors to support both browsers.
Summary: support NP_GetEntryPoints as a way to get plugin funtion pointers → support NP_GetEntryPoints as a way to get plugin function pointers
Attached patch fix v1.0 (obsolete) — Splinter Review
This patch adds support for NP_Initialize/NP_GetEntryPoints for sending/getting function pointers. However, support for this is #ifdef'd so that it is Intel-only. The reason for this is that on PPC we run into the possibility of the plugin expecting or sending tvector-wrapped function pointers. Since there was no protocol for this, some plugins expect tvectors via NP_Initialize, others expect function pointers, and we have the same problem with not knowing what we're getting from the plugin.

There are two possible solutions to this problem if we want to move towards API compatibility with WebKit. First we could take a patch like this and as we move towards an Intel-Mac-Only world we gain API compatibility. The other possible solution is to add a plugin-side call to npapi - if that function exists then we can call it and find out whether the plugin wants tvectors or function pointers. Actually, if the function exists then the author will probably be aware of our situation and we can just have that author follow a certain protocol and we don't even really need to ask.
Attachment #229678 - Flags: review?(mark)
Comment on attachment 229678 [details] [diff] [review]
fix v1.0

+  NP_PLUGINSHUTDOWN pfnShutdown = (NP_PLUGINSHUTDOWN)PR_FindSymbol(aLibrary, "NP_Shutdown");

I think this should be inside the #ifdef, but this patch does other confusing things with pfnShutdown and pfnMainShutdown, so I don't really know what you intended.

>+#ifdef MACOSX_GETENTRYPOINT_SUPPORT
>+  NP_GETENTRYPOINTS pfnGetEntryPoints = (NP_GETENTRYPOINTS)PR_FindSymbol(aLibrary, "NP_GetEntryPoints");
>+  NP_PLUGININIT pfnInitialize = (NP_PLUGININIT)PR_FindSymbol(aLibrary, "NP_Initialize");
>+  usesGetEntryPoints = (pfnGetEntryPoints && pfnInitialize && pfnShutdown);
>+
>+  if (usesGetEntryPoints) {
>+    // we call NP_Initialize before getting function pointers to match
>+    // WebKit's behavior. They implemented this first on Mac OS X.
>+    if (pfnInitialize(&(ns4xPlugin::CALLBACKS)) != NS_OK)
>+      return;
>+    if (pfnGetEntryPoints(&np_callbacks) != NS_OK)
>+      return;
>+  }
>+  else {
>+#endif

End the block like this:
  }
  else
#endif
  {

Then you can get rid of the maintenance-nightmare #ifdef at the end of the next block, and you ensure that the block always has the same scoping, regardless of whether the #ifdef is true or false.

>+    NPError error;
>+    NPP_ShutdownUPP pfnMainShutdown;
>+    
>+    // call into the entry point
>+    NP_MAIN pfnMain = (NP_MAIN)PR_FindSymbol(aLibrary, "main");
>+    
>+    if (pfnMain == NULL)
>+      return;
>+    
>+    NS_TRY_SAFE_CALL_RETURN(error,
>+                            CallNPP_MainEntryProc(pfnMain,
>+                                                  &(ns4xPlugin::CALLBACKS),
>+                                                  &np_callbacks,
>+                                                  &pfnMainShutdown),

What's pfnMainShutdown?  You never use it, is an API change.  I assume you meant to leave pfnShutdown?

>+                            aLibrary,
>+                            nsnull);
>+    
>+    NPP_PLUGIN_LOG(PLUGIN_LOG_BASIC,
>+                   ("NPP MainEntryProc called: return=%d\n",error));
>+    
>+    if (error != NPERR_NO_ERROR)
>+      return;
>+    
>+    fShutdownEntry = (NP_PLUGINSHUTDOWN) TV2FP(pfnShutdown);

pfnShutdown was already a function pointer.

>-  fShutdownEntry = (NP_PLUGINSHUTDOWN) TV2FP(pfnShutdown);
>+  fShutdownEntry = pfnShutdown;

I think fShutdownEntry should be what we got back from CallNPP_MainEntryProc if we're using main.
Attachment #229678 - Flags: review?(mark) → review-
Attached patch fix v1.1Splinter Review
Addresses comments
Attachment #229678 - Attachment is obsolete: true
Attachment #236983 - Flags: review?(mark)
Attachment #236983 - Flags: review?(mark) → review?(sayrer)
Comment on attachment 236983 [details] [diff] [review]
fix v1.1

>
>+    NPP_ShutdownUPP pfnMainShutdown;

pfnMainShutdown lives on. doesn't bother me though. r=sayrer
Attachment #236983 - Flags: review?(sayrer) → review+
Attachment #236983 - Flags: superreview?(mikepinkerton)
(In reply to comment #4)
> (From update of attachment 236983 [details] [diff] [review] [edit])
> >
> >+    NPP_ShutdownUPP pfnMainShutdown;

on IRC, Josh says pfnMainShutdown was the name of a different variable in the old patch. No issue here.
Comment on attachment 236983 [details] [diff] [review]
fix v1.1

sr=pink
Attachment #236983 - Flags: superreview?(mikepinkerton) → superreview+
Comment on attachment 236983 [details] [diff] [review]
fix v1.1

>Index: ns4xPlugin.cpp
>===================================================================

>+#ifdef MACOSX_GETENTRYPOINT_SUPPORT
>+  fShutdownEntry = (NP_PLUGINSHUTDOWN)PR_FindSymbol(aLibrary, "NP_Shutdown");
>+  NP_GETENTRYPOINTS pfnGetEntryPoints = (NP_GETENTRYPOINTS)PR_FindSymbol(aLibrary, "NP_GetEntryPoints");
>+  NP_PLUGININIT pfnInitialize = (NP_PLUGININIT)PR_FindSymbol(aLibrary, "NP_Initialize");
>+  usesGetEntryPoints = (pfnGetEntryPoints && pfnInitialize && fShutdownEntry);
>+
>+  if (usesGetEntryPoints) {
>+    // we call NP_Initialize before getting function pointers to match
>+    // WebKit's behavior. They implemented this first on Mac OS X.
>+    if (pfnInitialize(&(ns4xPlugin::CALLBACKS)) != NS_OK)

Should this be testing against NS_OK, or NPERR_NO_ERROR?

>+      return;
>+    if (pfnGetEntryPoints(&np_callbacks) != NS_OK)
>+      return;

Ditto.
Simon's comment #7 fixed on trunk. Nice catch!
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.