155696 - MOZ_PLUGIN_PATH should support colon-delimited PATH syntax

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Dan Mosedale (:dmosedale, :dmose)]

The $MOZ_PLUGIN_PATH env var is currently expected to be just a single variable.
 It really should support the usual colon-delimited PATH syntax.  

I was reminded of this by the following comment in bug 13474:
 
------- Additional Comments From [e-mail]  2002-07-01 21:14 -------
Almost forgot.  There's some really dumb PATH-walking code in
uriloader/exthandler/unix/nsOSHelperAppService.cpp.  It may make sense to
prettify it some, make it a little more careful, and put it somewhere where this
code can use it too...

Comment 1

[rubydoo123]

assigning to serge

Comment 2

[rubydoo123]

*** Bug 151208 has been marked as a duplicate of this bug. ***

Comment 3

[Peter Lubczynski]

This is actually for ALL platforms, Windows uses this variable too.

With the current way we use the app directory service provider, this may be a
little difficult to fix. Perhaps we can move the MOZ_PLUGIN_PATH key into it's
own directory service provider?

Comment 4

[Conrad Carlen (not reading bugmail)]

It's not so hard. That env variable is used in a context where the provider is
being asked for a list of files, it is not a publicly available key.
http://lxr.mozilla.org/mozilla/source/xpcom/io/nsAppFileLocationProvider.cpp#509
In this code, if the environment var was parsed as a list of files, it could
return multiple files. It would make the iterator object a little more complex
though. 

Comment 5

[Frank Belew]

This is really bothering our (Ximian) users, its one of the most reported bugs
after shipping Mozilla 1.0.

Are there any patches floating around?

Comment 6

[serge (gone)]

Conrad, you are that code owner, would you take this bug, please?

Comment 7

[Conrad Carlen (not reading bugmail)]

Taking it.

Comment 8

[serge (gone)]

thanks.

Comment 9

[Frank Belew]

ping?

Comment 10

[Frank Belew]

So I'm guessing this won't be fixed in 1.2 either?

Comment 11

[Frank Belew]

Since this is breaking plugins on EVERY mozilla release, I'd like to see it
fixed for the next one.
Adding nomination for mozilla1.2

Comment 12

[timeless]

Attachment: create a special class which parses the first path by the os path delim

frb tested a version of this on linux

Comment 13

[Frank Belew]

Yes, this seems to work fine if the user/mozilla wrapper script sets MOZ_PLUGIN_PATH

Comment 14

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 103216 [details] [diff] [review]
create a special class which parses the first path by the os path delim

>+        static const char* keys[] = { nsnull, NS_USER_PLUGINS_DIR, NS_APP_PLUGINS_DIR, nsnull };
>+        if (!*keys)
>+            keys[0] = PR_GetEnv("MOZ_PLUGIN_PATH");

Why the condition?  You're guaranteeing that it's going to be true in the line
above it, no?

Comment 15

[timeless]

static const
char* keys[] = { nsnull, NS_USER_PLUGINS_DIR, NS_APP_PLUGINS_DIR, nsnull };
if (!*keys)
  keys[0] = PR_GetEnv("MOZ_PLUGIN_PATH");

> Why the condition?  You're guaranteeing that it's going to be true in the line
> above it, no?
no :), but perhaps if you saw it like this it'd make more sense:
if (!keys[0])
  keys[0] = PR_GetEnv("MOZ_PLUGIN_PATH");

Comment 16

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

OK, so maybe I'm a moron, but I'm still not seeing it.

You set char * keys to { nsnull, NS_USER_PLUGINS_DID, ... }.

That means that keys[0] is nsnull.

This in turn means that the condition
  (!*keys)
or, equivalently, the condition
  (!keys[0])
will always evaluate to true, since keys[0] will always be nsnull/0/false.

Did I lose my mind?

Comment 17

[Boris Zbarsky [:bzbarsky]]

shaver, that's a static.  So after the first call to that function it will not
be null anymore.

Comment 18

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 103216 [details] [diff] [review]
create a special class which parses the first path by the os path delim

None of that happened.

sr=shaver.

Comment 19

[Conrad Carlen (not reading bugmail)]

Comment on attachment 103216 [details] [diff] [review]
create a special class which parses the first path by the os path delim

>Index: mozilla/xpcom/io/nsAppFileLocationProvider.cpp
> 
>+#if defined(XP_WIN) || defined(XP_OS2)/* Win32, Win16, and OS/2 */
>+#define PATH_SEPARATOR ';'
>+#else /*if defined(XP_UNIX) || defined(XP_MAC) || defined(XP_BEOS)*/

Nice. Thanks for taking this. r=ccarlen

One nit: remove the XP_MAC from the above comment. This doesn't apply to Mac
and that definition, on 1st read, gave me a scare.

>+#define PATH_SEPARATOR ':'
>+#endif

Comment 20

[serge (gone)]

Comment on attachment 103216 [details] [diff] [review]
create a special class which parses the first path by the os path delim

>+        static const char* keys[] = { nsnull, NS_USER_PLUGINS_DIR, NS_APP_PLUGINS_DIR, nsnull };
>+        if (!*keys)
>+            keys[0] = PR_GetEnv("MOZ_PLUGIN_PATH");

to eliminate an expansive |PR_GetEnv| call I would change it to something like
this:
if (!keys[0] && !(keys[0] = PR_GetEnv("MOZ_PLUGIN_PATH"))) {
  static const char nullstr = 0;
  keys[0] = &nullstr;
}

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 103216 [details] [diff] [review]
create a special class which parses the first path by the os path delim

a=dbaron for trunk checkin

Comment 22

[timeless]

checked in

Comment 23

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

verified that this works:
chb@frodo:~/mozilla/opt/xpcom$ ls /tmp/p*
/tmp/p1:
nppdf.so

/tmp/p2:
libflashplayer.so
$ MOZ_PLUGIN_PATH=/tmp/p1:/tmp/p2 ~/mozilla/opt/dist/bin/mozilla

about:plugins then correctly showed both the PDF plugin and the Flash plugin.

marking VERIFIED on linux using latest CVS (of the changed file only, other
stuff is a bit older)