345541 - plugins linked to libxul.so do not load

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Steve Krulewitz]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.0.4) Gecko/20060608 Ubuntu/dapper-security Firefox/1.5.0.4
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.0.4) Gecko/20060608 Ubuntu/dapper-security Firefox/1.5.0.4

When loading a plugin, nsPluginFile::GetPluginInfo (http://lxr.mozilla.org/mozilla1.8/source/modules/plugin/base/src/nsPluginsDirUnix.cpp) is called to get details about the plugin.  It looks for a symbol named "NSGetFactory" to determine if it is an XPCOM or NSAPI plugin.  However, if the plugin is linked to libxul.so, it will find the NSGetFactory defined in the live connect stuff (http://lxr.mozilla.org/mozilla1.8/source/js/src/liveconnect/nsCLiveconnectFactory.cpp#67) and it will treat the plugin as an XPCOM plugin and fail to load.  This happens for both the null and the demo print plugins.

The attached patch works around the problem by trying to use the plugin as an NSAPI plugin if the call to NSGetFactory fails.  I don't think it is a good solution but it works :)


Reproducible: Always

Comment 1

[Steve Krulewitz]

Attachment: Work around patch

Comment 2

[Benjamin Smedberg]

Urgh. We really ought to fix this by fixing liveconnect to not export NSGetModule. But I don't understand how liveconnect actually works, so I'm going to need to poke around and see if I can find an expert.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 230213 [details] [diff] [review]
Work around patch

Any chance of getting this reviewed and on the branch? We've been running with it for a while now and it's the only way we can get our plugins loading properly from XULRunner land...

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 230213 [details] [diff] [review]
Work around patch

+        if (NS_FAILED(rv)) {
+            // HACK: The symbol lookup for "NSGetFactory" mistakenly returns
+            // a reference to an unrelated function when we have an NSAPI
+            // plugin linked to libxul.so.  Give this plugin another shot as
+            // an NSAPI plugin

NSAPI in both cases should be NPAPI.

r+sr=jst with that changed.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 230213 [details] [diff] [review]
Work around patch

oops, didn't mean to request 1.8 approval yet.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 230213 [details] [diff] [review]
Work around patch

Fixed on trunk.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

please file a bug for doing what comment 2 suggests

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #7)
> please file a bug for doing what comment 2 suggests
> 

Filed bug 350550. Thanks for reminding me.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 230213 [details] [diff] [review]
Work around patch

Requesting branch approval for this one. This has baked for a few days and no regressions have popped up that I'm aware of. This patch basically catches a plugin load failure and tries one last thing to get it initialized properly. It seems therefore very low risk. This is the only way we have been able to load plugins properly in XULRunner so the benefit is large.

A better fix for this bug has landed on the trunk (see bug 350550) but it seems highly unlikely that that fix will ever go on the branch.

Comment 10

[Mike Schroepfer]

Is there a way to fix this without affecting plug-in code for FF/TBird.  We do not have much time left for final release -and no good automated plugin regression tests.  This area is generally fragile so we are not sure how this will impact the core release without months/weeks of baking...

Comment 11

[Benjamin Smedberg]

We could take bug 350550 which is actually probably safer than this fix.

Comment 12

[Mike Schroepfer]

Comment on attachment 230213 [details] [diff] [review]
Work around patch

Per comments 10 and 11 we are going to try and take bug 350550 instead.