108246 - A special character in plugin info causes caching mechanism fail

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Zac Jacobson]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.5+)
Gecko/20011101
BuildID:    2001110103

I copied plugin files from the Windows Media Player into Mozilla's plugins
directory, and about:plugins fails to work entirely.

Reproducible: Always
Steps to Reproduce:
1. I did this with Windows Media Player 7.01.00.3055
2. Attempt to enable WMP plugin in Mozilla by copying <WMP>\np* into Mozilla's
plugins diretory.
3. go to about:plugins

Actual Results:  The page displayed says only:

Installed plug-ins
Find more information about browser plug-ins at Netscape.com.
----- <horizontal line> -----


The JavaScript Console reports:

Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIDOMPlugin.name]"  nsresult: "0x80004005
(NS_ERROR_FAILURE)"  location: "JS frame ::
jar:resource:///chrome/toolkit.jar!/content/global/plugins.html :: <TOP_LEVEL>
:: line 95"  data: no]

Expected Results:  I would expect that this exception would be handled, and the
rest of the plugins would be displayed. In fact, it'd be nice if a little
warning block appeared on the about:plugins page saying such-and-such plugin is
invalid.

The offending plugin file is npwmsdrm.dll
When I take it out of the plugins directory, everything is wonderful again.

Comment 1

[Olivier Cahagne]

confirming with build 2001110103 on Win2k and WM 7.1.

Comment 2

[sairuh (rarely reading bugmail)]

->plug-ins? punt as needed.

Comment 3

[av (gone)]

This works for me with yesterday's build on NT40.

Shrirang? 

Files I copied are: npdrmv2.dll, npdrmv2.zip, npds.zip, npdsplay.dll.

Comment 4

[shrirang khanzode]

will chek this asap today..am having some problem with my NT..

Comment 5

[av (gone)]

OK, I see it now. The problematic file is npwmsdrm.dll.

Comment 6

[av (gone)]

dp, I think this is yours. I need your advice. When we get the plugin tag info 
and cache it in the registry your code dose something like

  registry->SetStringUTF8(pluginKey, kPluginsNameKey, tag->mName);

The problem here is that this specific plugin name contains a special character 
(the trademark -- (R)), and the above line of code fails and writes null in the 
registry, I beleive. Then when JavaScripts asks for a name of the plugin it gets 
an error and fails to execute.

I can see differents ways to fix this, mostly just bulletproofing which maybe is 
a good idea anyway, but the real problem is that the plugin info is _not_ cached 
in such a case, and I beleive we should fix that.

Comment 7

[Suresh Duddi (gone)]

Yup. You are right.  It is unfortunate that these names can have non-UTF8
characters in them. Which character code is this ?

PluginName and all other cached fields that come from the plugin could use
    nsIRegistry::SetBytesUTF8()
instead of SetStringUTF8(). Dan, would that work ?

The other option is to escape/unescape them while writing/reading from registry.

Dan, what is the best approach here.

Comment 8

[Daniel Veditz [:dveditz]]

The "UTF8" registry APIs expect UTF8, validate the UTF8, and return a specific
error indicating not-UTF8.

If you had Unicode you could use the SetString() API, but then you have to worry
about what charset the plugin is in in order to convert correctly. You can, as
dp suggests, use SetBytes or SetBytesUTF8 (which in this case merely refers to
the character coding of the name arg) to preserve the random bytes passed in,
but that merely moves the problem since at some point to display this string
you're still going to have to figure out what charset it's in and convert to
Unicode.

Comment 9

[av (gone)]

Attachment: not fully working patch

I did not use string stuff much so far. This patch attempts to address the
presence of spec chars in plugin name and description. And it sort of work. 

dp and dveditz, please advise on how to make it better.

Comment 10

[av (gone)]

Attachment: screen shot of how it looks with the patch applied

Please note an extra character just before the trademark symbol. There should
be nothing, not even a space char. When I look at it in the debugger, the '(R)'
symbol value is -82 (negative eighty two).

Comment 11

[Martin Poirier]

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

Comment 12

[Daniel Veditz [:dveditz]]

The default three lines of context is pretty useless for people unfamiliar with
code except for the most trivial patches; please use more context for reviews.

The deprecated "WithConversion" string methods or explicit conversions from or
to ASCII are a big red flag -- do you really have ASCII text? (by ascii we mean
the strict 7-bit charset, not the various 8-bit "ascii-plus" national sets
people sometimes call ascii, especially here in the US.) If you did then the
UTF8 registry methods wouldn't have given you any problems since ascii is a
subset of UTF8. Most likely you have Latin-1 data, which means it'll be
something completely different on Asian systems and the ConvertASCIItoUCS2()
functions will *really* mess you up.

Who calls nsPluginsHostImpl::RegisterPlugin()? The only calls I could find were
under module/plugin/samples and in the oji/MRJ directories, neither of which
seemed right. Who calls this will help determine which charset we should
use/expect. Or instead of the ASCII->Unicode conversion just store the text as
bytes and let the conversion errors be dealt with somewhere else.

Chopped off your patch in the RegisterPlugin routine was a for-loop that also
uses SetStringUTF8 for the mime types, descriptions and extensions. The
descriptions in particular might be translated into a non-ascii charset so I
think the UTF8 routines here are problematic.

Comment 13

[av (gone)]

dveditz, thanks for your thoughts. RegisterPlugin is supposed to be called by 
xpcom plugins which are now deprecated. MRJ plugin uses it on Mac. A couple of 
commersial products are on the market already, e.g. RealPlayer.

I'll try SetBytes approach and see how it works.

Comment 14

[av (gone)]

Attachment: patch to fix the bug -- now using SetBytesUTF8 instead of SetString

This patch works with no visible problems so far.

Comment 15

[av (gone)]

Attachment: the same patch as above, but manually modified to include more surrounding context

Comment 16

[av (gone)]

If this way to store string as bytes seems to be acceptable, should I 
explicitly terminate the array with null after |GetBytesUTF8(...)| before I cast 
it to |const char *|?

Comment 17

[Suresh Duddi (gone)]

Since you are writing strlen()+1 I dont think you need to try null terminate it.
It should happen automatically. Patch looks good to me.

Comment 18

[av (gone)]

Formal r=/sr= please?

Comment 19

[Daniel Veditz [:dveditz]]

You don't need to manually copy in more context, just pass the argument to the
cvs diff command so the result is a valid patch that can be applied and tested.
Thanks for the context though -- much appreciated! -- I'm just noting that the
cvs diff argument is easier and results in a patch that can be used as the main
review attachment so you don't have to make two attachments.

You did not address my concern about the remaining three SetStringUTF8 calls in
nsPluginHostImpl::RegisterPlugin. I'll grant that most likely the MIME type and
extension are 7-bit ascii (maybe even required by the spec?), but I don't trust
the description. You're not going to bail on error, that's good, but what
happens later if there's an uninitialized description?
Other than that though the rest looks good and I'll happily give sr= if you
reassure me about that one point.

Comment 20

[av (gone)]

I know about cvs diff -uN thing, just thought that including the whole C
function is better. Peter L. told me that there is such cvs option on Mac, but I
am not aware of the Win analogue.

I will gladly change the the remaining three cases along the same line if dp
does not object. dp, do you?

Comment 21

[Suresh Duddi (gone)]

The other three calls are mimetype, mimetype description and mimetype extension.
I can see description might need it but not the other two.

Comment 22

[Daniel Veditz [:dveditz]]

dp: isn't that what I said? 

I have more concerns than I did about MIME type and extension, though. It looks
like those values are passed in from the plugin itself, and even if legal values
only contain 7-bit ascii (and no-one has asserted that, it's an optimistic guess
on my part) there's no guarantee the calling program won't screw up.  These reg
entries won't get created and this routine has no problem with that, but what
happens later when these values aren't found?

av: including the whole function is much appreciated. You can usually jigger the
context value if you have to make sure its included. -u20 would have gotten
nearly all of it (-u30 if you wanted overkill) and has the advantage of 1) being
easier to produce for you than hand editing and 2) results in a valid patch that
can be reviewed and applied

Comment 23

[Suresh Duddi (gone)]

Dan, yeah. Thats what I get for fastreading...av it is up to you what you wanna
do here.

Comment 24

[av (gone)]

Attachment: patch -- now including mimedescriptions to SetBytesUTF8 business

Please review/superreview. I guess we are fine expecting lower ASCII chars only
for mimetypes and file extensions. If this whenever happens we will fail to
launch a plugin as the worst scenario. Evangelism issue, if any. I'll talk
about it on the next group gathering, perhaps the best way is just to mention
this fact in the plugin documentation.

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 58818 [details] [diff] [review]
patch -- now including mimedescriptions to SetBytesUTF8 business

sr=dveditz, thanks for making these changes

Comment 26

[serge (gone)]

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

Comment 27

[serge (gone)]

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

Comment 28

[av (gone)]

I checked this in to the trunk. dp, I guess we should also get it into the 
branch, should we? What branches will benefit?

Comment 29

[Suresh Duddi (gone)]

I dont think plugin caching is on the 0.9.4 branch. So this wont matter for that
branch.

Comment 30

[Jacob Kjome]

I have a similar problem.  Before this patch, about:plugins didn't work at all
(at least recently before this patch).  Now it works, but stops before
displaying info on all my plugins.  I also have a script that is similar to
about:plugins.  It does a loop through the plugins and pipes the output to a new
window.  The script does nothing right now because it fails before it can get to
the point where it pipes the data to the new window.  Is this going to be fixed
in full soon?

Here is the error in my script:

Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIDOMPlugin.description]"  nsresult: "0x80004005
(NS_ERROR_FAILURE)"  location: "JS frame ::
http://www.visi.com/~hoju/assets/scripts/browserinfo.js ::
getExtendedBrowserInfo :: line 88"  data: no]

To reproduce this, visit:
http://www.visi.com/~hoju/indextest.html

Then, click on "browser info extended browser info (NS only)" on the left side
of the screen.  You will see nothing happen.  Now check your javascript console
to see the error above.

jake

Comment 31

[shrirang khanzode]

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

Comment 32

[shrirang khanzode]

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

Comment 33

[av (gone)]

Marking fixed. Please file a separate bug for other issues mentioned if needed.

For verification purposes please use npwmsdrm.dll on Windows or plugger on 
Linux.

Comment 34

[Martin Poirier]

Sorry, still doesn`t work for me with 20011127 and 20011128 trunk builds, you
should verify the code has actually been checked in or reopen.

Comment 35

[av (gone)]

Martin, what exactly do you see? The best way to see the original problem (which 
is I beleive fixed) is to place npwmsdrm.dll in Mozilla plugins folder (not 
Netscape4's) and on about:plugins page see how it looks. Can you please try 
this?

Comment 36

[Jacob Kjome]

I'm using build 2001112809 on Win2k (SP2).

I just removed all plugins except for npnull.dll from my Netscape4 plugins 
directory and and have been testing various plugins on their own in Mozilla's 
plugins directory.  I haven't found one plugin yet that *doesn't* cause that 
error to be thrown.  These include:

npwmsdrm.dll (Windows Media Player)
nppl3260.dll (Real Player)
npswf32.dll  (flash)
np32dsw.dll (shockwave)
NPAXDLPI.dll (not sure what plugin)
All the Java plugins files
etc...

This build must have the fix that was checked into the trunk.  Did it do more 
harm than good?

Jake

Comment 37

[Martin Poirier]

See my description in the Bug 110011 I filed (duplicate) for a description of my
problem and the list of plugins I have (I have npwmsdrm.dll and I still see the
same problem as before).

Comment 38

[Martin Poirier]

I don't have netscape 4.x installed too, must I add. Netscape 6.x and Internet
explorer are not on my system too. The only browser I have is mozilla.

Comment 39

[Kriton Kyrimis]

The problem has become worse under Solaris/x86 2.8 using a home-made build
from the latest source. (The source tarball had a 28-Nov-2001 09:52
timestamp, so this is probably 20011128).

about:plugins now simply reports

Installed plug-ins
Find more information about browser plug-ins at Netscape.com.

and the following error appears in the JavaScript console:

Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIDOMPlugin.name]"  nsresult: "0x80004005
(NS_ERROR_FAILURE)"  location: "JS frame ::
jar:resource:///chrome/toolkit.jar!/content/global/plugins.html :: <TOP_LEVEL>
:: line 95"  data: no]

This is with just libnullplugin.so and plugger.so in the plugins
directory.

Comment 40

[Boris Zbarsky [:bzbarsky]]

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

Comment 41

[Kriton Kyrimis]

I built Mozilla for Solaris/x86 again from the 29 Nov sources, and this
time about:plugin works perfectly. Either I did something wrong yesterday,
or the bug was finally squashed during the last 24 hours.

Comment 42

[Zac Jacobson]

Well, the *was* fixed, but I just got Build 2001120303, and now I get the
original symptoms, though in more situations.

Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIDOMPlugin.name]"  nsresult: "0x80004005
(NS_ERROR_FAILURE)"  location: "JS frame ::
jar:resource:///chrome/toolkit.jar!/content/global/plugins.html :: <TOP_LEVEL>
:: line 95"  data: no]

So I put only npnul32.dll in the plugins dir, and I get the SAME EXCEPTION, but
at leas a line does display...

So I added the NPJava11.dll, only the default plugin is displayed, the exception
is thrown.
NPJava12.dll, only the default plugin is displayed, the exception is thrown.
NPJava131.dll, only the default plugin is displayed, the exception is thrown.
NPJava32.dll, only the default plugin is displayed, the exception is thrown.
any other plugin dll, now no plugins are displayed, and the exception is thrown.

Zac (original reporter)

Comment 43

[av (gone)]

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

Comment 44

[shrirang khanzode]

.