939796 - Disable loading gtk2 plugins to gtk3 browser

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Plug-ins, defect)

Description

[Martin Stránský [:stransky] (ni? me)]

Attachment: disable_gtk2_plugins.patch

We can't load gtk2 plugin to gtk3 browser.

Comment 1

[Benjamin Smedberg]

Does this work because gtk3 plugins don't import the symbol gtk_major_version but gtk2 plugins do? I wasn't aware that PR_FindSymbol worked for imported dynamic symbols: I thought it only worked for exported symbols.

Comment 2

[Martin Stránský [:stransky] (ni? me)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> Does this work because gtk3 plugins don't import the symbol
> gtk_major_version but gtk2 plugins do? I wasn't aware that PR_FindSymbol
> worked for imported dynamic symbols: I thought it only worked for exported
> symbols.

Yes, gtk_major_version is imported by gtk2 plug-ins from gtk libraries. For instance is listed as "unresolved" by flash plugin:

$ nm -D /usr/lib64/mozilla/plugins/libflashplayer.so | grep gtk_major_version
                 U gtk_major_version

But I used it for gtk3/gtk3 plugin host switch and works as expected:

handle = dlopen(aFilePath, RTLD_LOCAL|RTLD_LAZY);
void *version = dlsym(handle, "gtk_major_version");

And the dlsym is also used by PR_FindSymbol:

http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/linking/prlink.c#1161

Comment 3

[Martin Stránský [:stransky] (ni? me)]

To be correct here the gtk_major_version is an integer, not function.

Comment 4

[Josh Aas]

Comment on attachment 8333855 [details] [diff] [review]
disable_gtk2_plugins.patch

Review of attachment 8333855 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/base/PluginPRLibrary.h
@@ +83,5 @@
> +        /* We can't load gtk2 plug-in in gtk3 browser.
> +         * The gtk_major_version symbol is only in gtk2 library. */
> +        if(PR_FindSymbol(mLibrary, "gtk_major_version"))
> +            return false;
> +#endif

This doesn't seem like the right place to be doing this check. Yeah, you're checking for a symbol, but it's not a required *NPAPI* symbol, which is the context for that code.

Your issue has to do with whether or not it's a suitable library to use. I think 'nsPluginsDirUnix.cpp' is a better place for that stuff, can you move your check there?

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Attachment: disable gtk2 plugins, v2

Thanks, so something like that?

Comment 6

[Josh Aas]

Comment on attachment 8341721 [details] [diff] [review]
disable gtk2 plugins, v2

Review of attachment 8341721 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/base/nsPluginsDirUnix.cpp
@@ +333,5 @@
> +     * The gtk_major_version symbol is only in gtk2 library. */
> +    if(PR_FindSymbol(pLibrary, "gtk_major_version")) {
> +        return NS_ERROR_FAILURE;
> +    }
> +#endif

Yes, but space between 'if' and '('.

Did you test this, and it works as you want it to? How about with a pluginreg.dat file created by a gtk2 browser?

Comment 7

[Mike Hommey [:glandium]]

I doubt this actually works.

Comment 8

[Martin Stránský [:stransky] (ni? me)]

(In reply to Josh Aas (Mozilla Corporation) from comment #6)
> Comment on attachment 8341721 [details] [diff] [review]
> disable gtk2 plugins, v2
> 
> Review of attachment 8341721 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/plugins/base/nsPluginsDirUnix.cpp
> @@ +333,5 @@
> > +     * The gtk_major_version symbol is only in gtk2 library. */
> > +    if(PR_FindSymbol(pLibrary, "gtk_major_version")) {
> > +        return NS_ERROR_FAILURE;
> > +    }
> > +#endif
> 
> Yes, but space between 'if' and '('.
> 
> Did you test this, and it works as you want it to? How about with a
> pluginreg.dat file created by a gtk2 browser?

The PR_FindSymbol() check works, but GetPluginInfo() is called only when the plugin cache is updated for new plugins.

OTOH If the check is in LoadPlugin() I have to close the library when the check fails, right?

Comment 9

[Benjamin Smedberg]

Resolving old bugs which are likely not relevant any more, since NPAPI plugins are deprecated.