380984 - NPAPI symbols hidden in libnullplugin.so and libunixprintplugin.so

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Karl Tomlinson (:karlt back Feb 10)]

NPAPI symbols hidden in libnullplugin.so and libunixprintplugin.so

The code in modules/plugin/samples/default/unix and
modules/plugin/samples/unixprinting is compiled with
VISIBILITY_FLAGS = -fvisibility=hidden
and so no symbols are in the dynamic symbol table.

This results in the plugins not loading, as (not) seen in about:plugins.

Adding
  VISIBILITY_FLAGS =

before
  include $(topsrcdir)/config/rules.mk

in each of the respective Makefile.in works around the issue.

Comment 1

[Karl Tomlinson (:karlt back Feb 10)]

What is the correct solution to this?

Something like NS_EXPORT in npapi.h seems appealing except I assume we don't
want to depend on nscore.h.

As the API is not changing frequently a version script might be suitable, but
it may be easier to duplicate the NS_EXPORT logic in nscore.h?

Comment 2

[Benjamin Smedberg]

You probably want to duplicate http://mxr.mozilla.org/mozilla/source/nsprpub/pr/include/prtypes.h#197

Comment 3

[Karl Tomlinson (:karlt back Feb 10)]

Attachment: patch using PR_EXTERN

Thanks, Ben.  I see now that npapi.h (and so npupp.h) already depends on
prtypes.h, so it seems reasonable to use what's provided by NSPR.

PR_EXPORT looks good but PR_EXTERN is documented so seems the most
appealing option.

http://www.mozilla.org/projects/nspr/reference/html/Prtyp.html#PR_EXTERN

With gcc at least PR_IMPLEMENT doesn't seem necessary for the implementation
when the prototype has PR_EXTERN.

Comment 4

[Karl Tomlinson (:karlt back Feb 10)]

Changing to component to plug-ins
(as this can be fixed without changing the build).

Comment 5

[Karl Tomlinson (:karlt back Feb 10)]

Windows uses a DEFFILE, so there shouldn't be a problem on that platform.

OS2 doesn't have a DEFFILE, so I don't know how that works but I don't have an OS to test.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 265249 [details] [diff] [review]
patch using PR_EXTERN

Hmm, odd. I never realized that npapi.h depends on nspr. It really shouldn't. Plugins should be buildable using our headers w/o needing to include nspr stuff, ideally.

Therefore, I'd rather see us duplicating the definition of PR_EXTERN (and calling it NP_EXTERN or something) and making it do only what we need for plugins for now.

With that change we'd get the same functionality w/o introducing more dependencies on nspr (of which I don't actually see any right now). And one day we should even remove that dependency, ideally.

Comment 7

[Karl Tomlinson (:karlt back Feb 10)]

Thanks for your explanation, Johnny.

It looks like NP_EXPORT is the macro to use but it has already been used for 2
or 3 different purposes.  Here's the current situation:

It is used (appropriately I believe) for definitions of functions that need to
be exported (NP_GetEntryPoints NP_Initialize NP_Shutdown) from the plugin.

But in this use it is often defined empty and another method of exporting is used.

modules/plugin/samples/default/os2/npos2.cpp and
modules/plugin/samples/default/windows/npwin.cpp:

  #define NP_EXPORT

embedding/browser/activex/src/plugin/npwin.cpp:

  #ifdef WIN32
      #define NP_EXPORT
  #else
      #define NP_EXPORT _export
  #endif

---

The second use is in the definition of static and non-static callback functions
defined in ns4xPlugin.cpp, which don't need to be exported (in the linker
sense).  This is about calling convention not symbol visibility.

ns4xPlugin.h:

   * Use this macro before each exported function
   * (between the return address and the function
   * itself), to ensure that the function has the
   * right calling conventions on Win16.
   */
  
  #ifdef XP_OS2
  #define NP_EXPORT _System
  #else
  #define NP_EXPORT
  #endif

The functions defined with this definition have their corresponding function
pointer types (NPN_*UPP) defined in npupp.h using NP_LOADDS (not NP_EXPORT).

NP_LOADDS is defined slightly differently to NP_EXPORT in npapi.h:

  #if defined(_WINDOWS) && !defined(WIN32)
  #define NP_LOADDS  _loadds
  #else
  #if defined(__OS2__)
  #define NP_LOADDS _System
  #else
  #define NP_LOADDS
  #endif
  #endif

--

The third use is for NPN functions defined in npn.cpp, in place of the
NP_LOADDS used in npapi.h.  In this case it is defined empty.

embedding/browser/activex/src/pluginhostctrl/npn.h

  #define NP_EXPORT

Comment 8

[Karl Tomlinson (:karlt back Feb 10)]

The ideal situation I think would have been to have

* NP_EXPORT(__type) for indicating external visibility,

* NP_CALLBACK for indicating calling convention of callbacks,

* Something like NPN_API (and NPP_API I guess) for indicating calling
  convention of NPN and NPP functions.  This currently is the same as
  NP_CALLBACK but need not have been.

However, in the interests of minimizing changes to the files that have long been
used in the NPAPI, I propose the following.

* Define NP_EXPORT(__type) in npupp.h only for XP_UNIX

* Change ns4xPlugin.h/.cpp to use NP_CALLBACK (which probably should be set to
  NP_LOADDS, but, in the interests of not fixing what isn't broken, I'd leave
  the definition the same as what is currently in ns4xPlugin.h.

I assume embedding/browser/activex is not used on XP_UNIX so the new
definition should not conflict with either of the existing definitions there,
and the sample plugins at modules/plugin/samples/default/os2|windows are still
free to define their own NP_EXPORT.  (These situations can be changed in the
future if desired.)

Comment 9

[Karl Tomlinson (:karlt back Feb 10)]

Attachment: Change ns4xPlugin.h/.cpp to use NP_CALLBACK instead of NP_EXPORT

Comment 10

[Karl Tomlinson (:karlt back Feb 10)]

Attachment: define NP_EXPORT(__type) in npupp.h only for XP_UNIX and use it

Comment 11

[Michael Wu [:mwu]]

Checking in modules/plugin/base/public/npupp.h;
/cvsroot/mozilla/modules/plugin/base/public/npupp.h,v  <--  npupp.h
new revision: 3.24; previous revision: 3.23
done
Checking in modules/plugin/base/src/ns4xPlugin.cpp;
/cvsroot/mozilla/modules/plugin/base/src/ns4xPlugin.cpp,v  <--  ns4xPlugin.cpp
new revision: 1.149; previous revision: 1.148
done
Checking in modules/plugin/base/src/ns4xPlugin.h;
/cvsroot/mozilla/modules/plugin/base/src/ns4xPlugin.h,v  <--  ns4xPlugin.h
new revision: 1.42; previous revision: 1.41
done