607121 - The Windows Live Photo Gallery plugin has a strange character in the add-ons manager

Status: RESOLVED WONTFIX

(Core :: Layout: Text and Fonts, defect)

Description

[JK]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6) Gecko/20100101 Firefox/4.0b6
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101025 Firefox/4.0b8pre

The Windows Live Photo Gallery plugin has a strange character ("00 99") in the add-ons manager.

Reproducible: Always

Comment 1

[JK]

Attachment: Picture.

Comment 2

[Dave Townsend [:mossop]]

Please attach a copy of pluginreg.dat from your profile folder to this bug report.

http://support.mozilla.com/en-US/kb/Profiles

Comment 3

[JK]

Attachment: pluginreg.dat

Comment 4

[Dave Townsend [:mossop]]

The plugin seems to include symbol 99 (Trademark) in its name and for some reason we can't display it. It may be that the font in use just doesn't support that glyph, not sure what other conditions trigger this.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That is weird. We should find a glyph for that string in some other font in the system.

spammaaja: can you try loading this in your browser? data:text/html,™
Does that show a "TM"?

Comment 6

[Dave Townsend [:mossop]]

(In reply to comment #5)
> That is weird. We should find a glyph for that string in some other font in the
> system.
> 
> spammaaja: can you try loading this in your browser? data:text/html,™
> Does that show a "TM"?

I can now reproduce the problem on my windows machine and this data URL shows the TM symbol for me.

Comment 7

[Dave Townsend [:mossop]]

The steps to reproduce this are basically to install Windows Live Essentials (http://explore.live.com/windows-live-essentials?os=other) and then look in the add-ons manager for the new plugin.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Seems like some kind of regression in font fallback. Over to Jonathan

Comment 9

[JK]

(In reply to comment #5)
> That is weird. We should find a glyph for that string in some other font in the
> system.
> 
> spammaaja: can you try loading this in your browser? data:text/html,™
> Does that show a "TM"?

Yes, it shows a "TM".

Comment 10

[Dave Townsend [:mossop]]

Sounds like this is a core issue, please move back if you think it is something specific to the add-ons manager UI

Comment 11

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

undoing reassignment to nobody

Comment 12

[Asa Dotzler [:asa]]

Segoe UI, the font for Windows Vista and Windows 7 interfaces, simply doesn't have a TM glyph. That's the trigger for this specific problem.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, but we should be falling back to a font that does have the TM glyph.

Comment 14

[Jonathan Kew [:jfkthame]]

(In reply to comment #4)
> The plugin seems to include symbol 99 (Trademark) in its name and for some
> reason we can't display it. It may be that the font in use just doesn't support
> that glyph, not sure what other conditions trigger this.

If the plugin uses character code (hex) 99 -- I assume you can't mean decimal 99, because that would be "c" -- then the plugin is apparently using something other than Unicode, so it's not surprising we fail. The Unicode value U+0099 is in the C1 Controls block, it is not a printing character.

The correct Unicode value for the trademark symbol would be U+2122.

The byte value 0x99 is used in Windows codepage 1252 (but not ISO-8859-1) for the trademark symbol; presumably that's where this plugin is getting it from. What kind of string is this, and does the plugin API say about the encoding of the name? If it's supposed to be a Unicode string, then this looks like an error in the plugin.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In nsPluginTag::nsPluginTag we copy the string into nsPluginTag::mMimeDescriptionArray, which expects UTF8.

It looks like we get that string from the plugin DLL. See nsPluginFile::GetPluginInfo. GetKeyValue calls VerQueryValueW and expects to get UTF16, but since the codepage is specified as part of the lpSubBlock parameter, I guess we're getting back 1252 because that's what we asked for. The docs say "The function retrieves a string value specific to the language and code page indicated."

I'm guessing we should call VerQueryValueA and interpret the result as 1252.

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: patch, convert plugin info strings from cp1252 to unicode

(In reply to comment #15)
> It looks like we get that string from the plugin DLL. See
> nsPluginFile::GetPluginInfo. GetKeyValue calls VerQueryValueW and expects to
> get UTF16, but since the codepage is specified as part of the lpSubBlock
> parameter, I guess we're getting back 1252 because that's what we asked for.
> The docs say "The function retrieves a string value specific to the language
> and code page indicated."

It seems a bit weird, because the string is returned in WCHARs when calling VerQueryValueW, but it is indeed returning codepage 1252 values in those 16-bit code units.

> I'm guessing we should call VerQueryValueA and interpret the result as 1252.

Yes, that seems to work. I was a bit dubious whether we could mix that with the use of GetFileVersionInfoW to read the version info, but it's fine: note that the version record is an opaque block passed as a void*, not a TCHAR*, so it's interoperable between the W and A functions.

With this patch, the Windows Live™ Photo Gallery plugin shows up as expected in the addons manager.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We can actually write a test for this pretty easily (and should). Just add some CP1252 stuff (like "TM") to the name and description in nptest.cpp. Then we can read them in a test using the same code that about:plugins uses.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, no I guess that's different.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, add the CP1252 stuff to nptest.rc. And add the UTF8-encoded version to Info.plist, and to PLUGIN_NAME and PLUGIN_DESCRIPTION in nptest.cpp.

Comment 20

[Simon Montagu :smontagu]

Comment on attachment 492960 [details] [diff] [review]
patch, convert plugin info strings from cp1252 to unicode

I think this should be using the native ::MultiByteToWideChar with CP_ACP instead of a UnicodeDecoder with hardcoded windows-1252.

Comment 21

[Simon Montagu :smontagu]

Wait, we already have a hardcoded 0x04e4 = 1252 in the string we pass to VerQueryValue? Ugh. We should really be making some effort to get localized version info here. Is there already a bug about that?

Even if that's out of scope for this fix, I suggest at least using a codepage variable and passing it to both VerQueryValue and MultiByteToWideChar.

Comment 22

[Jonathan Kew [:jfkthame]]

(In reply to comment #21)
> Wait, we already have a hardcoded 0x04e4 = 1252 in the string we pass to
> VerQueryValue? Ugh. We should really be making some effort to get localized
> version info here. Is there already a bug about that?

I wondered about that aspect (are plugins expected to provide localized info?), but I'd suggest filing a separate bug; this one is about correctly interpreting the (non-localized) info we currently get.

Comment 23

[Jonathan Kew [:jfkthame]]

Attachment: patch, v2 - convert plugin info strings to Unicode

Updated based on smontagu's comments.

Note that it turns out we *can't* use VerQueryValueA to get the info strings from the version record, as that doesn't handle the ™ character correctly - it's replaced by ? in the resulting byte string. (My previous testing missed this because of cached plugin info; an earlier version of the patch had put the right string into our cache, so then I didn't realize it was being corrupted by the "ANSI" variant of the function.)

Comment 24

[Jonathan Kew [:jfkthame]]

Attachment: patch, v3 - convert plugin info strings to Unicode

For unknown reasons, linking libxul failed on tryserver Win Opt build, complaining it is unable to find swprintf (although it builds fine on Debug, and both ways locally).

Updating the patch to use the "secure" variant _snwprintf_s instead resolves this problem.

Comment 25

[Benjamin Smedberg]

Comment on attachment 495655 [details] [diff] [review]
patch, v3 - convert plugin info strings to Unicode

I am so not the guy you want for this ;-)

If the question was about snwprintf, you probably wanted _snwprintf, but the _s version is fine also.

Comment 26

[Jim Mathies [:jimm]]

Comment on attachment 495655 [details] [diff] [review]
patch, v3 - convert plugin info strings to Unicode

>diff --git a/modules/plugin/base/src/nsPluginsDirWin.cpp b/modules/plugin/base/src/nsPluginsDirWin.cpp
>+#define KEYBUFSIZE  64 // more than enough for any of the keys used here
>+  WCHAR keybuf[KEYBUFSIZE];
>+  const WCHAR keyFormat[] = L"\\StringFileInfo\\%04X%04X\\%s";
>   WCHAR *buf = NULL;
>   UINT blen;
> 
>-  ::VerQueryValueW(verbuf, key, (void **)&buf, &blen);
>+  if (_snwprintf_s(keybuf, KEYBUFSIZE, KEYBUFSIZE-1,


I think we can get rid of KEYBUFSIZE and just use NS_ARRAY_LENGTH. PRUnichar should work here as well.


>+                   keyFormat, language, codepage, key) < 0) {
>+    NS_NOTREACHED("plugin info key too long for buffer!");
>+    return nsnull;
>+  }

>+  ::VerQueryValueW(verbuf, keybuf, (void **)&buf, &blen);

Let's wrap all our check up here: |if (!VerQueryValue || !*blen || !buf)| and return if this fails.

>-    return PL_strdup(NS_ConvertUTF16toUTF8(buf).get());
>+    // copy the WCHAR cp1252 output from VerQueryValueW into a
>+    // byte buffer so that we can use MultiByteToWideChar on it
>+    nsCString byteString;
>+    for (UINT i = 0; i < blen; ++i) {
>+      byteString.Append((char)buf[i]);
>+    }

Do we need this extra copy if we pass buf/blen to MultiByteToWideChar?

>+
>+    nsAutoTArray<PRUnichar,128> ustr;
>+    if (!ustr.SetLength(blen)) {
>+      return nsnull;
>+    }
>+    int ulen = ::MultiByteToWideChar(codepage, MB_PRECOMPOSED,
>+                                     byteString.BeginReading(), blen,
>+                                     ustr.Elements(), ustr.Length());

int MultiByteToWideChar(
  __in   UINT CodePage,
  __in   DWORD dwFlags,
  __in   LPCSTR lpMultiByteStr,
  __in   int cbMultiByte,
  __out  LPWSTR lpWideCharStr,
  __in   int cchWideChar
);

Did you want ustr.Elements() for param five? This doesn't look right.


>+  if (::GetFileVersionInfoW(lpFilepath, NULL, versionsize, verbuf)) {
>+    // TODO: get appropriately-localized info from plugin file
>+    UINT lang = 1033; // language = English
>+    UINT cp = 1252;   // codepage = Western

Please file a follow up on that todo, I wonder what we get when we run on non-english versions of windows. I'm guessing either english or blank depending on the plugin. That's probably something we want to investigate.

Comment 27

[Jonathan Kew [:jfkthame]]

(In reply to comment #26)

> >+    // copy the WCHAR cp1252 output from VerQueryValueW into a
> >+    // byte buffer so that we can use MultiByteToWideChar on it
> >+    nsCString byteString;
> >+    for (UINT i = 0; i < blen; ++i) {
> >+      byteString.Append((char)buf[i]);
> >+    }
> 
> Do we need this extra copy if we pass buf/blen to MultiByteToWideChar?

Yes; MultiByteToWideChar expects the cp1252 data as a byte string, but VerQueryValueW gives it to us as a string of WCHARs. So we have to "narrow" it. I know it sucks.

> >+    nsAutoTArray<PRUnichar,128> ustr;
> >+    if (!ustr.SetLength(blen)) {
> >+      return nsnull;
> >+    }
> >+    int ulen = ::MultiByteToWideChar(codepage, MB_PRECOMPOSED,
> >+                                     byteString.BeginReading(), blen,
> >+                                     ustr.Elements(), ustr.Length());
> 
> int MultiByteToWideChar(
>   __in   UINT CodePage,
>   __in   DWORD dwFlags,
>   __in   LPCSTR lpMultiByteStr,
>   __in   int cbMultiByte,
>   __out  LPWSTR lpWideCharStr,
>   __in   int cchWideChar
> );
> 
> Did you want ustr.Elements() for param five? This doesn't look right.

Yes, that's where MultiByteToWideChar is going to write its Unicode output.

> Please file a follow up on that todo, I wonder what we get when we run on
> non-english versions of windows. I'm guessing either english or blank depending
> on the plugin. That's probably something we want to investigate.

Will do. I don't think the version of Windows would matter; all that matters is what language(s) the plugin file provides. We always ask it for English, so that's what we'll get; if the plugin ONLY has non-English info, we'll get nothing.

Comment 28

[Jonathan Kew [:jfkthame]]

Attachment: patch, v4 - convert windows plugin info to unicode - updated with review comments

Comment 29

[Jonathan Kew [:jfkthame]]

Filed bug 620736 about getting localized info.

Comment 30

[Jonathan Kew [:jfkthame]]

Comment on attachment 499064 [details] [diff] [review]
patch, v4 - convert windows plugin info to unicode - updated with review comments

Requesting approval2.0; we unblocked on this during triage in December, but I still think we should take the patch. It should be safe, as it doesn't affect anything except plugin info strings on Windows, and showing a hexbox instead of the ™ symbol does look pretty bad.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We should have a test for this. It shouldn't be hard to write.

Comment 32

[Jonathan Kew [:jfkthame]]

Pushed the patch: http://hg.mozilla.org/mozilla-central/rev/3f8bee2e48a7

I have started on a test, but it's not yet working nicely across all platforms (there may be encoding issues in the test harnesses as well as the actual code?) - I'll leave this bug open for now until that is ready.

Comment 33

[Jonathan Kew [:jfkthame]]

Attachment: WIP - add a non-ASCII character to test plugin description string

To test our support for non-ASCII characters in plugin info, this adds a ™ (Trademark, U+2122) character to the end of the "Description" string of the Test Plug-in. (Note that this character is present in Windows codepage 1252, but with a _different_ codepoint from its Unicode value.)

Several testcases that check the contents of the Description string are updated accordingly.

Currently, together with the patch from bug 624789, this passes unit tests on tryserver on Linux and Mac OS X, but gives one failure in mochitest-plain-1 (in test_bug391728.html) and three failures in xpcshell (test_bug455213.js and test_plugins.js) on Windows only. Given that these tests pass on other platforms, and that the Windows code does in fact read the ™ character (0x99 in cp-1252) correctly from the plugin file (as shown by the Addons Manager window), I'm inclined to suspect an encoding-related problem in the test harnesses on Windows, but have not yet figured out exactly what's going wrong.

Comment 34

[JK]

(In reply to comment #32)
> Pushed the patch: http://hg.mozilla.org/mozilla-central/rev/3f8bee2e48a7
> 
> I have started on a test, but it's not yet working nicely across all platforms
> (there may be encoding issues in the test harnesses as well as the actual
> code?) - I'll leave this bug open for now until that is ready.

I'm still seeing the hexbox, unless I create a completely new profile.

I don't think that users can be expected to create new profiles, especially since as far as I know, profile creation is considered a developer/tester feature and the manager is going to be removed.

Comment 35

[Jonathan Kew [:jfkthame]]

(In reply to comment #34)

> I'm still seeing the hexbox, unless I create a completely new profile.

This is because we cache the plugin info in the profile, and don't re-read it unless the plugin file is updated; and the problem here happened during reading the info, not displaying it.

Actually, as I've been trying to test this, I'm coming to think that the real problem is in the Windows Live Photo Gallery plugin file, and we were *correct* to display the hexbox. (Note that if you use the Windows Explorer to examine "properties" of the NPWLPH.dll file, and look at the Details tab, it does *not* display the Trademark symbol in the "Product name"; the character immediately after "Live" in the string "Windows Live Photo Gallery" is simply missing.)

So I think this issue is far from closed; I'll be digging further, but expect that we'll end up reverting the "fix" here. It is really a hack for a single incorrectly-encoded dll, and would potentially break other cases where plugins have non-ASCII info.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You don't think we're supposed to interpret that registry value as CP1252? I thought that was pretty clear myself.

This problem is minor enough that I don't think we should worry about invalidating incorrect cached profile data.

Comment 37

[Jonathan Kew [:jfkthame]]

(In reply to comment #36)
> You don't think we're supposed to interpret that registry value as CP1252? I
> thought that was pretty clear myself.

(It's not a registry value, it's a string from the DLL's version resource.)

It seemed a bit strange to be getting CP1252 data in 16-bit code units (I'd have expected to get it in 8-bit code units), but the result of VerQueryValueW seemed to support that, and so I wrote the patch to interpret it as such.

However, the MS documentation explicitly states (in the context of the version resource, even) that "All strings in a version resource are in Unicode format" (see http://msdn.microsoft.com/en-us/library/ms648007(v=vs.85).aspx). A hex dump of the NPWLPG.dll file confirms that the data in it is *incorrect*: it has U+0099 (actually 0x99 0x00, because it's little-endian), where it should have 0x22 0x21 (U+2122, the ™ character). So that DLL is incorrect.

When I've been trying to create a test for this, I kept running into failures, and realized that it's because I was never getting a 0099 word into the resource; although I was providing cp1252 source, with the 0x99 byte in it, the Windows resource compiler (correctly) converts it to U+2122.

Such a resource, where the "040904E4" strings contain real Unicode, displays the string correctly in the Windows Explorer properties dialog, whereas the string from NPWLPG.dll does *not* display a ™ character.

> This problem is minor enough that I don't think we should worry about
> invalidating incorrect cached profile data.

Yes, I agree.

My concern now is that the patch to "fix" the string for the Photo Gallery plugin has in fact *broken* the display of any *correct* Unicode strings that contain non-ASCII characters (well, non-ISO-8859-1 characters, to be strictly accurate). That's why I've been unable to create a working test where the plugin description contains a (correctly-encoded) ™ character.

So I will be posting a further patch to basically revert the encoding-munging aspect of this, and a test to verify that we then handle non-ASCII strings successfully (they were broken on OS X, it turns out - just fixed in bug 624789). The display of the Photo Gallery plugin info will remain broken because it actually IS broken. I suppose we could filter out non-printing Unicode characters from the strings, if we want to prevent the hexbox showing up, but that's about all we can reasonably do for it.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, I'm convinced!

I don't think we need to do anything to skip the hexbox.

Comment 39

[Jonathan Kew [:jfkthame]]

Attachment: patch 2, undo incorrect encoding-munging, VerQueryValueW _does_ return Unicode

This basically undoes the encoding hackery from the previous patch, as that turned out to be incorrect: it worked for the Photo Gallery plugin, but only because that file actually has wrongly-encoded data in it, and it _breaks_ things for plugins that provide (correct) non-ASCII strings (see bug 626180).

Other code tidy-up/restructuring from the original patch is retained, as we'll want that when we address bug 620736.

Comment 40

[Jonathan Kew [:jfkthame]]

Attachment: add non-ASCII text to Test Plugin's "description" string

This adds some non-ASCII text to the Description string in the Test Plug-in, so that we can verify whether the encoding is handled properly. To test this, I've added the ™ character plus sample words in Hindi, Chinese and Arabic to the plug-in file, and modified the testcases that check the Description string accordingly.

Comment 41

[Jonathan Kew [:jfkthame]]

Comment on attachment 504247 [details] [diff] [review]
patch 2, undo incorrect encoding-munging, VerQueryValueW _does_ return Unicode

Requesting approval-2.0 in order to undo the badness from the previous patch here (and thereby fix bug 626180).

Comment 42

[Benjamin Smedberg]

Comment on attachment 504247 [details] [diff] [review]
patch 2, undo incorrect encoding-munging, VerQueryValueW _does_ return Unicode

You technically don't need approval for a backout (or a blocker, since this fixes a blocker bug). But here you go!

Comment 43

[Jonathan Kew [:jfkthame]]

Pushed patch (partial-revert) and test change:
http://hg.mozilla.org/mozilla-central/rev/d0323a335993
http://hg.mozilla.org/mozilla-central/rev/f48d19c20a7d

Closing this as WONTFIX, because the originally-reported problem will now NOT be fixed; it is correct behavior when presented with incorrectly-encoded plugin data. We can't "fix" it without breaking things for other correct plugins, which is clearly not something we want to do.