574354 - Add wildcard support to OOP plugin disable prefs (e.g. for nspluginwrapper)

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

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

We should disable OOP for plug-ins which are already wrapped by nspluginwrapper and are running out of process.

A typical scenario on Fedora when nspluginwrapper is installed:

[komat@localhost ~]$ ll /usr/lib64/mozilla/plugins-wrapped/
IcedTeaPlugin.so -> /usr/lib64/mozilla/plugins/IcedTeaPlugin.so
libjavaplugin.so -> /usr/lib64/mozilla/plugins/libjavaplugin.so
libunixprintplugin.so -> /usr/lib64/mozilla/plugins/libunixprintplugin.so
npwrapper.so -> /usr/lib64/nspluginwrapper/npwrapper.so
nswrapper_32_64.libflashplayer.so
nswrapper_64_64.libdiamondx.so
nswrapper_64_64.libflashplayer.so
nswrapper_64_64.npbasicplugin.so
packagekit-plugin.so -> /usr/lib64/mozilla/plugins/packagekit-plugin.so

where nswrapper_X_X.* are plug-ins handled by nspluginwrapper and the rest are symlinks to regular plug-ins.

Maybe the RunPluginOOP() routine should use wildcards to match library names?

Comment 1

[Karl Tomlinson (:karlt)]

Using wildcards sounds reasonable to me.

CC'ing Josh and Benjamin to see whether long term plans are to continue supporting plugins pref'ed in-process.

Comment 2

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

Attachment: wildcard patch

The wildcard patch. Do we need any extra check for fnmatch.h presence?

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 454040 [details] [diff] [review]
wildcard patch

I think Josh would be a better reviewer for this.

Josh, one reason why people would use nspluginwrapper is to run Adobe's (32-bit only) Flash Player in a 64-bit browser.

Comment 4

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

Or Adobe Reader plug-in which is i386 only too.

Comment 5

[Josh Aas]

Comment on attachment 454040 [details] [diff] [review]
wildcard patch

>+  char** prefNames;
>+  rv = prefs->GetChildList(prefGroupKey.get(),
>+                           &prefCount, &prefNames);

I'm pretty sure you're responsible for cleaning up the result memory, thus you're leaking "prefNames". I think you want to use "NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY".

>+    PRUint32 currentPref = 0;
>+    for (; currentPref < prefCount; currentPref++)
>+    {

Please put the currentPref variable inside the loop declaration and put the opening brace on the same line.

>+      if(!fnmatch(maskStart, prefFile.get(), 0)) {

Put a space between "if" and "(".

Comment 6

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

Attachment: v2

Thanks! There's an updated version attached.

Comment 7

[Josh Aas]

Comment on attachment 461205 [details] [diff] [review]
v2

This is calling NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY even if prefNames is null and in that case the value of prefCount is not initialized so you could crash referencing an invalid address in the loop for NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY. Only free if "GetChildList" succeeds.

+  bool prefSet = false;

This needs to be PRBool, assignments to it PR_FALSE/PR_TRUE.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to comment #7)
> Comment on attachment 461205 [details] [diff] [review]
> v2
> 
> +  bool prefSet = false;
> 
> This needs to be PRBool, assignments to it PR_FALSE/PR_TRUE.

Really? <http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/27549f4e2fb4f51a/f058d235a0f68c60> says otherwise, and we're using bool all over the tree already.

Comment 9

[Josh Aas]

Please follow module style and use PRBool. At some point we'll probably move everything to bool via automated rewrite and until then consistency is the way to go.

Comment 10

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

Attachment: v3

Ahh, sorry for that. It should address it.

Comment 11

[timeless]

+  nsCAutoString prefGroupKey("dom.ipc.plugins.enabled.");

this should be an NS_LITERAL_CSTRING

Comment 12

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

Thanks!

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

You'll need to ask for approval2.0. Also, please address timeless' comment.

Comment 14

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

(In reply to comment #11)
> +  nsCAutoString prefGroupKey("dom.ipc.plugins.enabled.");
> 
> this should be an NS_LITERAL_CSTRING

All string constants from nsNPAPIPlugin.cpp are without it so I think it would be better to file a separate bug to convert all of them to NS_LITERAL_CSTRING, wouldn't it?

Comment 15

[Benjamin Smedberg]

Comment on attachment 461293 [details] [diff] [review]
v3

Either way: I tend to think the literal-string should be checked in now, but this has proper reviews and timeless is not a peer of this code.

Comment 16

[Karl Tomlinson (:karlt)]

Visual Studio doesn't seem to have fnmatch.h.
I guess NT isn't POSIX.2.
e:/builds/moz2_slave/tryserver-win32-debug/build/modules/plugin/base/src/nsNPAPIPlugin.cpp(124) : fatal error C1083: Cannot open include file: 'fnmatch.h': No such file or directory
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1280655143.1280658530.11685.gz#err0

Comment 17

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

Fnmatch is missing on windows generally, there seems to be some equivalent - PathMatchSpec() (http://msdn.microsoft.com/en-us/library/bb773727.aspx)

Comment 18

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

Attachment: possible windows patch

A patch with PathMatchSpec for windows, untested.

Comment 19

[Karl Tomlinson (:karlt)]

NS_WildCardMatch looks like it might provide an XP way to do this.
(I haven't checked exactly what makes a "valid shell expression".)
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsWildCard.h

Comment 20

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

Attachment: v4, with NS_WildCardMatch()

It calls NS_WildCardValid() for shell pattern validation and then calls strcasecmp() for normal strings and NS_WildCardMatch() for shell expressions.

Comment 21

[Josh Aas]

Comment on attachment 462757 [details] [diff] [review]
v4, with NS_WildCardMatch()

>+  nsCAutoString prefGroupKey("dom.ipc.plugins.enabled.");
>+        

There is a bunch of extra whitespace in that blank line.

>+  nsresult rv;
>+  PRUint32 prefCount;
>+  char** prefNames;
>+  rv = prefs->GetChildList(prefGroupKey.get(),
>+                           &prefCount, &prefNames);

Just declare rv where it is used a few lines down.

>+      if (valid == INVALID_SXP) {
>+         continue;
>+      }
>+      else if(valid == NON_SXP) { 
>+        // mask is not a shell pattern, compare it as normal string
>+        match = (strcasecmp(prefFile.get(), maskStart) == 0);
>+      } else {
>+        match = (NS_WildCardMatch(prefFile.get(), maskStart, 0) == MATCH);
>+      }

You're mixing style for putting "else if" and "else" on the same line as the ending brace for the last block. I don't care which way you do it but be consistent.

Also, for the NON_SXP case I don't think you need to be doing a case insensitive compare. The prefs are supposed to be lower case and you already converted prefFile to lower case, so since everything is already lower case you can do a case-sensitive compare which is faster.

Comment 22

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

Attachment: v5

Should address that, thanks.

Comment 23

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/75caf7ab0376

Comment 24

[LegNeato]

What is the user impact if this? Does making this change help the user in any way, or is it more a correctness issue? If the wrapper works fine currently I would not really be inclined to take this on branch.

Comment 25

[Karl Tomlinson (:karlt)]

This comment is not really a reply to comment 24.
The patch here only adds flexibility to preferences, and I hope does not actually change the effect of existing preferences.
However, the intention is to allow preferences to be set so that nspluginwrapper runs in-process.

There are some things that I would recommend checking before setting such preferences:

1) That script evaluation does not get reordered to happen during a windowless
   plugin paint event.
   (I don't know whether or not or how nspluginwrapper supports script
   execution.)

2) That right-clicking on a windowless Flash plugin to bring up a context menu
   does not hang the browser.

3) That text can be copied from browser and pasted to plugin and vice versa.

Comment 26

[LegNeato]

Comment on attachment 463091 [details] [diff] [review]
v5

We will not be taking this for 1.9.2.11. Those sound like great things to do and we'll re-review after this is tested a bit more.