132430 - Real Jukebox Plugin destroys plugin finder service (default plugin) on branch

Status: VERIFIED FIXED

Description

[Arun Ranganathan]

On branch browsers based on 0.9.4, it appears that the default plugin completely
stops working when the file nprjplug.dll (the Real Jukebox plugin) is installed.
 Try the following HTML before and after the installation of the Real Jukebox
Plugin on builds based on 0.9.4: 

                <embed name="gregg" type="application/xyz" src="somefile.xyz"
width="50" height="50" hidden="false"></embed>

You'll see that the plugin finder service isn't invoked after nprjplug.dll is
installed.  This is a default installation with RealOne Player :-(

Comment 1

[Arun Ranganathan]

Created attachment 75284 [details]
The deadly Jukebox plugin that kills our Plugin Finder Service

Comment 2

[Arun Ranganathan]

[e-mail] diagnosed this problem like this:

"I don't think there is a code fix here unless we want to break the exisiting
semantics of the default pluign. This plugin does not return any errors during
initilization. Since this happens in 4.x too, I think it's an evangelism issue
for Real."

The Bugscape counterpart of this bug is
http:://bugscape.netscape.com/show_bug.cgi?id=12437

We've GOT to do something about this before we beta another Netscape product
with this problem in it.

Comment 3

[Peter Lubczynski]

Suggestion:

Black list the Real Jukebox plugin (nprjplug.dll).

Comment 4

[Arun Ranganathan]

The evangelism work is done.  We now need to stick to our part of the bargain
and fix this bug for versions of Real Jukebox that Real can't fix and that are
in existence already.

Comment 5

[Arun Ranganathan]

Setting target milestone.

Comment 6

[av (gone)]

I'll take it for now to see if I can come up with anything quick.

Comment 7

[av (gone)]

Arun, I have a couple of questions:

1. Do we need it on Windows only? 
2. Should there be a pref to turn it off?
3. Is dll name good enough to screen the other plugins? Maybe plugin name would
be better (the one which shows up in about:plugins in bold, we can make it the
same for all platforms if it is not already).

Comment 8

[av (gone)]

Created attachment 79034 [details] [diff] [review]
patch v.1

This is essentially one-liner, I rewrote the whole function code replacing
|while| loop with |for| loop but leaving it logically equivalent. The only real
change is this check for being our default plugin if '*' mime type is
encountered.

Please review.

Comment 9

[av (gone)]

Created attachment 79169 [details] [diff] [review]
patch v.2

Better patch. It allows to still use any third party "*" handler, but only in
case when there is no default plugin found.

Arun, do we want this behaviour?

Comment 10

[Peter Lubczynski]

Comment on attachment 79169 [details] [diff] [review]
patch v.2

r=peterl

Comment 11

[serge (gone)]

Comment on attachment 79169 [details] [diff] [review]
patch v.2

>+  nsPluginTag * tagStarHandler = nsnull;
>+  PRBool bStar = !PL_strcmp(aMimeType, "*");
>+
>+  // if we have a mimetype passed in, search the mPlugins linked list for a match
>+  for (nsPluginTag * tag = mPlugins; tag; tag = tag->mNext) {
>+
>+    // if we found the default plugin for the '*' mime type, return
>+    if (bStar && !PL_strcmp(tag->mName, NS_MOZILLA_DEFAULT_PLUGIN_NAME)) {
>+      aPlugin = tag;
>+      return NS_OK;
>+    }
>+
>+    PRInt32 variants = tag->mVariants;
>+    for (PRInt32 i = 0; i < variants; i++) {
>+      if (tag->mMimeTypeArray[i] && !PL_strcasecmp(tag->mMimeTypeArray[i], aMimeType)) {
>+
>+        // Skip any potential '*' mime type handlers which are not our own default
>+        // plugin as this breaks the plugin finder service, see Bugzilla bug 132430
>+        if (bStar) {
>+          // but remember it in case we don't have the default plugin
>+          if (!tagStarHandler)
>+            tagStarHandler = tag;
>+
what is the reason to continue this loop,
if you already found one '*'?
>+          continue;
>+        }
if tag->mMimeTypeArray[i + n] == aMimeType == "*",
I mean if there is more than one '*' in tag->mMimeTypeArray,
default '*' handler could be lost by return here
>+
>+        aPlugin = tag;
>+        return NS_OK;
>+      }
>+    }
>+  }
>+
>+  if (bStar && tagStarHandler) {
>+    aPlugin = tagStarHandler;
>+    return NS_OK;
>+  }
>+
>+  return NS_ERROR_FAILURE;
>+}

Comment 12

[av (gone)]

s> what is the reason to continue this loop,
s> if you already found one '*'?
>+          continue;
>+        }
s> if tag->mMimeTypeArray[i + n] == aMimeType == "*",
s> I mean if there is more than one '*' in tag->mMimeTypeArray,
s> default '*' handler could be lost by return here
>+
>+        aPlugin = tag;
>+        return NS_OK;

I don't see how it can be lost -- we always |continue| if |bStar| is true, no
matter what |tag->mMimeTypeArray[i + n]| is, thus not hitting that |return|. But
you are right, there no reason for |continue|, |break| will be better here.

Comment 13

[av (gone)]

Created attachment 79297 [details] [diff] [review]
patch v.3 --

Comment 14

[serge (gone)]

Comment on attachment 79297 [details] [diff] [review]
patch v.3 -- 

>we always |continue|
you are right, I overlooked if() braces
adding r=serge

Comment 15

[av (gone)]

Comment on attachment 79297 [details] [diff] [review]
patch v.3 -- 

Please hold, small modification needed: the default plugin returns different
strings on different platforms.

Comment 16

[av (gone)]

Created attachment 79301 [details] [diff] [review]
patch v.4 -- platform differences addressed

Comment 17

[av (gone)]

Comment on attachment 79301 [details] [diff] [review]
patch v.4 -- platform differences addressed

Carrying over serge's and peterl's r=.

Comment 18

[Patrick C. Beard]

Comment on attachment 79301 [details] [diff] [review]
patch v.4 -- platform differences addressed

It's really a bad idea to hard code the name of our default plugin in our
source code. What if somebody installs a plugin with the same name? What
prevents such a plugin from taking over from our default plugin?

Comment 19

[av (gone)]

Nothing. As nothing will prevent people to compile their own npnul32.dll which
would mimic the default plugin in all aspects. I voiced this at one of the aruns
meetings and we decided to proceed with this logic for the only primary reason
-- to fix this specific RealJukeBox problem and prevent _accidents_ like this.
Indeed, if somebody does it on purpose, there is not much we can do.

The bad hardcoding thing will also go away when we switch to XUL based "*" handler.

Comment 20

[Peter Lubczynski]

What about changing the handler to something like *.* or adding an extra field
to the version info to signify this is REALLY the default plugin?

What about completely black-listing nprjplug.dll? What about having code to
re-write nprjplug.dll's mime-type version table with something else than '*'?

Comment 21

[av (gone)]

Well, any of those solutions will involve hardcoding of some sort: JukeBox
plugin file name or default plugin name (or any other returnable string). Just
changing "*" to "$" or anything else will solve the immediate JukeBox problem
without hardcoding but will place us in exactly the same situation as we were
before this JukeBox release -- it will be just a matter of luck untill another
'JukeBox' with "$" handler appear. I don't see a clear advantage of any of these
approaches (including one we already have posted here).

I am fine with any approach, we just need to make a decision. If anybody thinks
that one solution is better than other, please speak up and explain.

Comment 22

[av (gone)]

Created attachment 79381 [details] [diff] [review]
patch v.5 -- different approach: black-listing JukeBox plugin

This patch adds more mess to the 'unwantedness' concept and just plainly
black-lists any plugin with "npjukebox.dll" file name on Windows.

Comment 23

[av (gone)]

First approach has an advantage of blocking only a specific mime type, not the
whole plugin. So, if somebody makes a plugin with multiple mime types and "*"
among them, not only will everything work fine but also this plugin will be
available to handle other mime types.

Comment 24

[serge (gone)]

can we use pref to specify black list?

Comment 25

[serge (gone)]

hardcoded "npjukebox.dll" seems inappropriate to me,
real will fix their bug in next release and a lot of hardcoded mozilla wont be 
able to use new npjukebox.dll

Comment 26

[av (gone)]

I don't like black-listing plugins at all. That patch was just an illustration
of one of the suggested approaches. I personally like skipping "*" mime type in
third party plugins better.

Comment 27

[av (gone)]

OK, looks like any solution to this bug is not going to satisfy everybody. But
we still need to fix it. Let's not try to achieve any perfection here as it is
barely possible. I spent a lot of time thinking it over and talking to Peter
analyzing different ways to fix it. The issue is not really that big, it _is_
easy to fix the originaly reported problem. What we can do.

1. Black list the plugin. Bad, because they will fix it at some point. This is
really bad.
  1-a. Use pref to be able to turn it off later. Bad, because it will block all 
  the other possible mime types of a given plugin.

2. Fall back if the star handler is not us. Bad, because this involves
hardcoding our default plugin info. But is this really that bad? Yes, the string
can be localized.
3. (Peter just came up with the idea.) Use Plugin ID we don't have yet but are
going to introduce. Bad? I don't know yet. Maybe risky.
4. As punkt 2 but using a special newly added numeric value from the version
info. It will still be hardcoded but at least will not be under a localization
danger.
Any other ideas?

Comment 28

[serge (gone)]

how about hardcoded or better pref defined default plugin file name?

Comment 29

[av (gone)]

This is a possibility I was also thinking of. But it is not essentialy different
from number 4. It will eliminate the need to add a new field to the version info
which is a plus, but it will be platform specific. From the other hand, there is
also an idea to hide all this code in nsPluginsDir<platform>.cpp so that the "*"
mime type of the alien plugin is ignored on the stage of creating
nsPluginTagInfo struct. If it will eventually be done this way, the file name
would make sense.

Comment 30

[serge (gone)]

embeders can use their own null plugin, so file name should be only pref 
defined.

Comment 31

[av (gone)]

Ah, interesting point, leaving the number 4 the best solution so far. Does
everybody agree?

Comment 32

[Peter Lubczynski]

Yeah, I think I like v.4 but with a pref. What if NS_MOZILLA_DEFAULT_PLUGIN_NAME
is localized?

Comment 33

[av (gone)]

No, Peter, I meant number four from the options in comment 27. Patrick, would
you agree with this approach plus prefs to turn it on/off?

Comment 34

[av (gone)]

This will involve changes to the default plugin itself on all platforms, so we
will need to make sure it gets into the distribution. Besides, we need to ask
embedding people to add analogous changes to their version of the default plugin.

Comment 35

[av (gone)]

Created attachment 79765 [details] [diff] [review]
patch v.6 (Windows only) -- implementation of option 4 from comment 27

This is yet another light-weight solution. Suspicious plugins get their "*"
mimetype replaced with the space character. Our own default plugin is
identified by new field in the version info -- 'DefaultPlugin' which is set to
1. This code is pref enabled (defaulted to not allowing alien plugins take
over) and all resides in nsPluginsDirWin.cpp.

Comment 36

[serge (gone)]

I don't understand how this code distinguishes suspicious plugins with "*" from 
our own old npnull32.dll?
by default for old npnull32.dll it will always call FixUpMimeType()
+    PRBool alien = defaultPlugin ? (PL_strcmp(defaultPlugin, "1")) : TRUE;
+    if (alien && !allowAlienStarHandler)
+      FixUpMimeType(mimeType);
Am I wrong?

Comment 37

[av (gone)]

You are right. This code will not distinguish. Why?

Comment 38

[serge (gone)]

+    if (alien && !allowAlienStarHandler)
is TRUE by default
+      FixUpMimeType(mimeType);
and this call will substitute '*' to ' ' for old npnull32.dll

Comment 39

[av (gone)]

Another idea came up on the aruner's meeting (thanks, Patrick). Let's call it:

Option 5:
Have the default plugin having a sort of signature, the simplest way being
adding an exported function the browser would try to find in the dll which poses
itself as "*" type handler. The downside of this approach is that we should
advertize it to the embedders who may write their own default plugin, and not to
the plugin developers.

Comment 40

[serge (gone)]

Created attachment 80052 [details] [diff] [review]
mime type black list proposal

when I was talking about pref specified black list, I was thinking about
something like this patch, it gives us a flexibility to block any xyz-mimetype
for any particular plugin shared lib, and this is XP.
any thoughts?

Comment 41

[av (gone)]

I like the idea of having the code in nsPluginHostImpl.cpp as it makes it
cross-platform (although slightly less efficient for we already looped through
the mime types in the according nsPluginsDir*.cpp files, but this is not
significant, I guess). What I am not sure about is that we need this mechanism
of blocking any arbitrary mime type implemented in such a way. Maybe we do, and
this will make Peter's fix for blocking mime types which are handled internally
less crusial. From the other hand, this functionality is supposed to be added
when we implement long awated plugin/mime type manager, and doing this now may
seem to be  an unnecessary overkill for the specific task.

The patch itself needs polishing (e.g. strings should be moved to bundles), but
we need to make a decision first, which probably means that at least everybody
on the cc list agrees.

Comment 42

[av (gone)]

Created attachment 80234 [details] [diff] [review]
patch v.8

This patch identifies the default plugin by presence of a special entry point.
Windows only.

Comment 43

[kmurray1115]

Can I get an update on the progress of this? Are we ready for reviews?

Comment 44

[av (gone)]

We are. And I am trying hard to get one. But more than just a review this bug is
about how we should approach it. Neither seems to be completely satisfying,
that's why people are somewhat reluctant to review.

Comment 45

[Peter Lubczynski]

Something's gota go in, but have we decided on which approach to take? If we
have, r=peterl, if not, perhaps we can decide in tomorrow morning's meeting.

Comment 46

[av (gone)]

Well, Patrick vocalized what I implemented in patch v.8. This will at least
guarantee we get an 'sr=' (less technical details of the implementation itself,
if any).

Comment 47

[Peter Lubczynski]

Comment on attachment 80234 [details] [diff] [review]
patch v.8

Loose the mimetype_reserved_message:

1. We've passed localization freeze
2. They may be looking for that description in JS
3. I don't see the point but I do see some possible confusion

Overall, I like this approach, however, perhaps NP_GetPrivateData could return
something more usefull like a struct or enum and we could actually check for
it. Ahh, what about using NP_GetMIMEDescription?

Comment 48

[av (gone)]

The string can be just hardcoded. I don't think it can be localized anyway, it
is simple |char *| everywhere.

|GetPrivateData| returns |void *|. I did this way specifically because it can be
filled with anything we may want in the future.

If changing the mime description line can cause problems, how about not touching
it at all?

Comment 49

[av (gone)]

Created attachment 81170 [details] [diff] [review]
patch v.8.1

This patch is essentially v.8 less mime description change. Given we passed
localization freeze this will be more appropriate for the branch. But for the
trunk we should condider something like v.8.

Comment 50

[av (gone)]

Created attachment 81197 [details] [diff] [review]
patch v.8.2

As per peterl's suggestion the new entry point name is |NP_GetMIMEDescription|
now. It is prototyped to match the Unix version which already has it.

Comment 51

[Jaime Rodriguez, Jr.]

What are the chances this is gonna be ready by tonight? If not tonight, can we
get a bets guesstimate on when this might be resolved?

Comment 52

[Peter Lubczynski]

Comment on attachment 81197 [details] [diff] [review]
patch v.8.2

r=peterl

Comment 53

[Marc Attinasi]

Comment on attachment 81197 [details] [diff] [review]
patch v.8.2

sr=attinasi

Comment 54

[av (gone)]

Patch checked in to the trunk. Nominating for 1.0.0.

Comment 55

[scottputterman]

adding adt1.0.0+ keyword.  Please check this into the branch as soon as possible
and add the fixed1.0.0 keyword after getting drivers approval.

Comment 56

[Asa Dotzler [:asa]]

Comment on attachment 81197 [details] [diff] [review]
patch v.8.2

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 57

[av (gone)]

Updating keyword business and marking FIXED.

Comment 58

[Arun Ranganathan]

yay!  Changing QA contact from [e-mail] to [e-mail] .
 Shrir, the best way to verify/test this bug is:
1. Install a branch build, preferably of the "Netscape" branded browser (so as
to benefit from plugin installer recognition).
2. Install RealOne Player.  After completing install, ensure that the branch
build has the nprjplug.dll , or the NS RealJukebox Plugin.
3. Now, test standard EMBED tag invocations to the Plugin Finder Service.  If
you need help, ask me.  Essentially, the Plugin Finder Service should be
correctly invoked.  This is "proof of the pudding" in terms of this fix.  Let's
verify this one immediately.

Comment 59

[shrirang khanzode]

this fix is good on branch 0430 win98/nt. default plugin now works, pfs is not 
overrided or anything, works nicely. Things are back to normal.

Comment 60

[R.K.Aa.]

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

Comment 61

[shrirang khanzode]

verified fixed on trunk as well .