420285 - Internationalize plugin tag and plugin host

Status: RESOLVED FIXED

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

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

Quick time description has Japanese locale on MacOSX.

However, it is garbage text on addon manager. (This is same as bug 420280).
And on about:plugins, it is empty string.

Because the encoding is Shift_JIS. However, we assume that it is UTF-8 which *file system* encoding. Therefore, We need to internationalize the plugin tab/host.

The bug's causes are:

1. nsPluginTag is not internationalized.
2. Using wrong charset at encoding in nsPluginHostImpl.

We need following fix:

1. nsPluginTag should have UTF-8 file name/full path/plugin name/plugin description/mime descriptions.
2. file name/full patch should be file system charset, however others are plain text charset on the system or UTF-8.
3. plugin cache in profile should be UTF-8 and it should have new file name for compatibility with older fx.

Note that this apporach is adhoc, but this is first step of the internationalization of plugins. Now, we don't have much time for Gecko1.9. So, this is limitation of us now.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> Quick time description has Japanese locale on MacOSX.

Quick time description has _Japanese characters on_ Japanese locale on MacOSX.

Sorry.

Comment 2

[Johnny Stenback (:jst)]

*I* don't think this is a blocker, but I'd love to get a fix for this in. As for the patch, I really don't like taking a change this size to this code this late in the release :(

At the very least, I really don't think we should change the name of the pluginreg.dat file. If need be (and we probably do need to) we could bump the version number in that file, but changing the name doesn't seem like a good idea to me.

If you disagree with blocking- here, please renominate, but please note that that doesn't mean we can't fix this. If we get a fix that's ideally smaller and doesn't change the file name, we could request approval on that and get it in, but past beta5 I doubt it would be accepted :(

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 306504 [details] [diff] [review]
Patch v1.0

r- based on the above comment.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #2)
> *I* don't think this is a blocker, but I'd love to get a fix for this in. As
> for the patch, I really don't like taking a change this size to this code this
> late in the release :(

I believe that this should be blocker. Because we cannot read the plugin descriptions. (They are garbage text! See bug 420280) This is not a critical issue for Gecko. However, Japanese users hate such garbage text bug.

I agree to your worry, because I also worry. However, this is important for non-ASCII users. I hope that you imagine you cannot read a part of UI that displays some information.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.1

using the registry version.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 307518 [details] [diff] [review]
Patch v1.1

@@ -831,168 +825,220 @@ nsPluginTag::nsPluginTag(nsPluginInfo* a
           pre = *(p - 1);
           *(p - 1) = '\0';
         } else {
           cur = *p;
           *p = '\0';
         }
 
       }
-      mMimeDescriptionArray[i] = new_str(aPluginInfo->fMimeDescriptionArray[i]);
+      mMimeDescriptionArray.AppendCString(
+        nsDependentCString(aPluginInfo->fMimeDescriptionArray[i]));

Do we know aPluginInfo->fMimeDescriptionArray[i] always is non-null here? Might need a null check (and an append of an empty string as you've done in other places), as nsDependentCString should not be used on a null pointer.

- In nsPluginTag::nsPluginTag(const char* aName,
                          const char* aDescription,
                          const char* aFileName,
                          const char* aFullPath,
                          const char* const* aMimeTypes,
                          const char* const* aMimeDescriptions,
                          const char* const* aExtensions,
                          PRInt32 aVariants,
                          PRInt64 aLastModifiedTime,
                          PRBool aCanUnload)
...
+      mMimeDescriptionArray.AppendCString(
+                              nsDependentCString(aMimeDescriptions[i]));

Same thing here.

With that fixed this looks good, it still scares me a bit though, so the sooner we get this in the better. I'd like bz to give this a once over too. bz, let me know if you won't get to this in time for beta5.

Comment 7

[Johnny Stenback (:jst)]

Marking blocker/P1, this needs a beta for sure.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.2

Thank you, Johnny.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 307687 [details] [diff] [review]
Patch v1.2

>Index: modules/plugin/base/src/nsPluginHostImpl.h
>+  nsCStringArray mMimeDescriptionArray; // UTF-8

I think it would be better (less malloc-y, etc) to use nsTArray<nsCString> here.That needs corresponding changes to the code that uses this array, of course.  All those places where you have to do a ?: with nsDependentCString could become AppendElement() calls, with the char* passed to AppendElement.

>Index: modules/plugin/base/src/nsPluginHostImpl.cpp


>@@ -831,168 +827,220 @@ nsPluginTag::nsPluginTag(nsPluginInfo* a
>+      mMimeDescriptionArray.AppendCString(
>+        nsDependentCString(aPluginInfo->fMimeDescriptionArray[i]));

Are we sure that pointer is never null?  Where is that enforced?

>+  } else {
>+    for (int i = 0; i < mVariants; i++)
>+      mMimeDescriptionArray.AppendCString(EmptyCString());

This could just become a SetLength() call on the array, if we used nsTArray.

>+static nsresult ConvertToUTF8(nsIUnicodeDecoder *aUnicodeDecoder,
>+  // If the member is already UTF-8, we must not reconvert it.
>+  // (Such case is initialized from plugin cache)
>+  if (IsUTF8(aNativeString)) {

Is there any way to store that information in the plugin tag instead of guessing it based on how the string's bytes happen to look?

>+nsresult nsPluginTag::EnsureMembersAreUTF8()
>+  if (!charset.Equals("utf-8", nsCaseInsensitiveCStringComparator())) {

  if (!charset.LowerCaseEqualsLiteral("utf-8")) {

>+    ConvertToUTF8(decoder, mFileName, mFileName);
>+    ConvertToUTF8(decoder, mFullPath, mFullPath);

So where do mFileName and mFullPath normally come from, other than the plug-in registry?  Why is assuming that they're in the platform filename charset a good assumption?  I guess that's sort of what this code did anyway... OK.
 
>+  // The resources of the plugins should same the standard plain text file
>+  // encoding on the system.

Perhaps:

  // The description of the plug-in and the various MIME type descriptions
  // should be encoded in the standard plain text file encoding for this system.

?  Is that generally true for plug-ins?  They don't encode in UTF-8 or some such, or there is just one particular plug-in being an issue here?  How does this whole thing work if the same profile (in a network directory) is used across multiple systems (with possibly different default encodings)?

>+  if (!charset.Equals("utf-8", nsCaseInsensitiveCStringComparator())) {

Again, LowerCaseEqualsLiteral().

Since ConvertToUTF8 is always called with the in and out param the same string, perhaps this should be made clear by just making it a single inout param?  Otherwise it woudl be easy for local changes to ConvertToUTF8 to screw things up.

> class DOMMimeTypeImpl : public nsIDOMMimeType {
>+    if (aTag->mExtensionsArray)
>+      mSuffixes.AssignWithConversion(aTag->mExtensionsArray[aMimeTypeIndex]);
>+    if (aTag->mMimeTypeArray)
>+      mType.AssignWithConversion(aTag->mMimeTypeArray[aMimeTypeIndex]);

I think this should be using CopyUTF8toUTF16, since we know those are UTF8.  No reason to do a lossy conversion.

>+static nsresult Create4xPlugin(nsIServiceManagerObsolete* aServiceManager,
>+  if (!charset.Equals("utf-8", nsCaseInsensitiveCStringComparator())) {

LowerCaseEqualsLiteral().

>@@ -4790,84 +4839,81 @@ NS_IMETHODIMP nsPluginHostImpl::GetPlugi
>+    if (!pluginTag->mDescription.IsEmpty() &&
>+        StringBeginsWith(pluginTag->mDescription,
>+                         nsDependentCString("Shockwave Flash 6.0"),

No need for the IsEmpty() check; if mDescription is empty, StringBeginsWith will just return false.  And NS_LITERAL_CSTRING, please, not nsDependentCString.

>+       int ver = atoi(pluginTag->mDescription.get() + 21);

Toss in a comment about why 21 if you happen to know?  And perhaps even a check that 21 < pluginTag->mDescription.Length()?

>-  if (PL_strcasestr(tag->mFileName,"npqtplugin") != nsnull)
>+  if (tag->mFileName.Find("npqtplugin", PR_TRUE) >= 0)

Are you sure this is picking up the right overload?  I seem to recall this being a problem...  Might be worth just passing all the args.  I'd compare != kNotFound rather than >= 0.

>@@ -5590,50 +5636,50 @@ nsPluginHostImpl::WritePluginInfo()
>-        (tag->mFileName ? tag->mFileName : ""),
>+        (!tag->mFileName.IsEmpty() ? tag->mFileName.get() : ""),

Might be able to just unconditionally use .get() here, but I suppose this is safer...

>@@ -6544,30 +6589,29 @@ nsPluginHostImpl::HandleBadPlugin(PRLibr
>+    nsCAutoString pluginname;

nsCString.  You're about to assign nsCStrings into it, so the stack buffer will just get ignored anyway.

The rest looks great, but I'd like the above comments addressed, especially the nsTArray thing.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.3

(In reply to comment #9)
> >+static nsresult ConvertToUTF8(nsIUnicodeDecoder *aUnicodeDecoder,
> >+  // If the member is already UTF-8, we must not reconvert it.
> >+  // (Such case is initialized from plugin cache)
> >+  if (IsUTF8(aNativeString)) {
> 
> Is there any way to store that information in the plugin tag instead of
> guessing it based on how the string's bytes happen to look?

the plugin tag should always have UTF-8 str. And when plugin tag created from another plug in tag, the conversion is not called.

> >+    ConvertToUTF8(decoder, mFileName, mFileName);
> >+    ConvertToUTF8(decoder, mFullPath, mFullPath);
> 
> So where do mFileName and mFullPath normally come from, other than the plug-in
> registry?  Why is assuming that they're in the platform filename charset a good
> assumption?  I guess that's sort of what this code did anyway... OK.

The current code always assumes that they are native encoding. If that is wrong, we have some bugs :-(

> Is that generally true for plug-ins?  They don't encode in UTF-8 or some
> such, or there is just one particular plug-in being an issue here?  How does
> this whole thing work if the same profile (in a network directory) is used
> across multiple systems (with possibly different default encodings)?

No. E.g., on mac, the resource encoding depends on the <locale> of |Minefield.app/Contents/Resources/<locale>.lproj|. If the <locale> and system locale are not same, the garbage text is still there. However, it should be rare case, and we cannot fix it with internationalizing plugin tag and plugin host. I said that "this is first step of the internationalization of plugins." in comment 0. So, I need more works for the issue. But it is after Gecko 1.9.

Thank you for your review!

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 308194 [details] [diff] [review]
Patch v1.3

> the plugin tag should always have UTF-8 str.

Yes, I understand that.  My question was whether we could flag tags initialized with UTF-8 from the plug-in cache so that we don't have to use the IsUTF8 guessing mechanism here....

>Index: modules/plugin/base/src/nsPluginHostImpl.cpp
> nsPluginTag::nsPluginTag(nsPluginTag* aPluginTag)
>-    mMimeDescriptionArray(nsnull),
...
>+  for (int i = 0; i < mVariants; i++)
>+    mMimeDescriptionArray.AppendElement(aPluginTag->mMimeDescriptionArray[i]);

I think it would be better to put:

  mMimeDescriptionArray(aPluginTag->mMimeDescriptionArray),

in the initializer instead of doing the for loop by hand.

>-  if (PL_strcasestr(tag->mFileName,"npqtplugin") != nsnull)
>+  if (tag->mFileName.Find("npqtplugin", 0, PR_TRUE) != kNotFound)

That doesn't seem right...  Which Find() signature are you looking at?  The one I've found so far is in nsTString.h:

158       NS_COM PRInt32 Find( const char* aString, PRBool aIgnoreCase=PR_FALSE, PRInt32 aOffset=0, PRInt32 aCount=-1 ) const;

So I'd think you want (PR_TRUE, 0, -1) for the 2-4 args?  Or are you calling some other Find()?

With those two issues fixed and an aswer to the "tag from plug-in cache" issue, sr=bzbarsky

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.4

(In reply to comment #11)
> (From update of attachment 308194 [details] [diff] [review])
> > the plugin tag should always have UTF-8 str.
> 
> Yes, I understand that.  My question was whether we could flag tags initialized
> with UTF-8 from the plug-in cache so that we don't have to use the IsUTF8
> guessing mechanism here....

O.K. I added a param that is |PRBool aArgsAreUTF8 = PR_FALSE| to a constructor of nsPluginTag. If that is true, EnsureMembersAreUTF8 is not called. And the IsUTF8 checking is removed now.

> >-  if (PL_strcasestr(tag->mFileName,"npqtplugin") != nsnull)
> >+  if (tag->mFileName.Find("npqtplugin", 0, PR_TRUE) != kNotFound)
> 
> That doesn't seem right...  Which Find() signature are you looking at?  The one
> I've found so far is in nsTString.h:
> 
> 158       NS_COM PRInt32 Find( const char* aString, PRBool
> aIgnoreCase=PR_FALSE, PRInt32 aOffset=0, PRInt32 aCount=-1 ) const;
> 
> So I'd think you want (PR_TRUE, 0, -1) for the 2-4 args?  Or are you calling
> some other Find()?

Wow, sorry for the mistake. I looked nsAString::Find()

http://mxr.mozilla.org/mozilla/source/xpcom/glue/nsStringAPI.h#294
>  294   NS_HIDDEN_(PRInt32) Find(const char *aStr, PRBool aIgnoreCase = PR_FALSE) const
>  297   NS_HIDDEN_(PRInt32) Find(const char *aStr, PRUint32 aOffset, PRBool aIgnoreCase = PR_FALSE) const;

So, I looked wrong class (nsACString doesn't have same args |Find()|)...

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'll land the latest patch several hours later.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in. thank you!

Comment 15

[[e-mail]]

Comment on attachment 308330 [details] [diff] [review]
Patch v1.4

>+  readonly attribute AUTF8String name;
So, at least on my system, nsPluginTag::mName is "Microsoft® DRM." In ISO-8859-1. Oops.

Comment 16

[[e-mail]]

In fact it looks like nsPluginsDirWin.cpp isn't using Unicode when it should.

Comment 17

[Tuukka Tolvanen (sp3000)]

In member function ‘nsresult nsPluginHostImpl::ReadPluginInfo()’:
http://mxr.mozilla.org/mozilla/source/modules/plugin/base/src/nsPluginHostImpl.cpp#5871
warning: cannot pass objects of non-POD type ‘class nsCString’ through ‘...’; call will abort at runtime

5871     PR_LOG(nsPluginLogging::gPluginLog, PLUGIN_LOG_BASIC,
5872       ("LoadCachedPluginsInfo : Loading Cached plugininfo for %s\n", tag->mFileName));

-  char          *mFileName;
+  nsCString     mFileName; // UTF-8

Comment 18

[Johnny Stenback (:jst)]

That's covered in bug 421992.