1040905 - Fix GetNativePath usage in GMP - Profiles with unicode characters fail on Windows

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Benjamin Smedberg]

I am about 99% certain that the GMP code will fail to load OpenH264 on Windows if the username or profile directory has unicode characters. This is because it communicates the GMP plugin path using the native charset.

STR for testing:
* In an English or other-locale Windows, create a directory with a Thai or Hebrew name.
* Launch Firefox -profile <thaidirname>
* Trigger OpenH264 download and install
* Try to load an OpenH264 test page

This is because of the GetNativePath call at http://hg.mozilla.org/mozilla-central/annotate/330ba968ed61/content/media/gmp/GMPParent.cpp#l62 and the subsequent native path usage at http://hg.mozilla.org/mozilla-central/annotate/330ba968ed61/content/media/gmp/GMPChild.cpp#l60 and #l83

In other code the way we deal with this is that for Windows, we convert the unicode path to UTF8 in the parent and convert it back in the child. This represents the full unicode range.

PR_LoadLibrary should be replaced with nsIFile->Load which does the correct unicode magic.

Jesup, would you be willing to mentor a volunteer for this? Qeole has worked on plugin stuff in the past and might be willing to take this, although he'll need some help with testing.

Comment 1

[Qeole]

I'd be glad to help, but I'll have to find/set a machine running Windows, and I'm not used to working/developping on this OS (or for MacOS/X, by the way).

I'll have a look at what I can do (with a virtual machine, or by finding space on my old dual boot install maybe). I tell you in a few days if I feel like I'm able to work on this − if it's OK for you.

Comment 2

[Qeole]

I managed to compile and run Firefox under Windows 7 (with French locale) after all, so it should be OK. I'll have a look at OpenH264 and try to reproduce the bug during coming week.

Comment 3

[Maire Reavy [:mreavy]]

Qeole -- Thank you for your help with this!  Is it ok if I assign this bug to you?  I talked with Randell (Jesup), and he'd be happy to mentor you on this bug.

Comment 4

[Qeole]

Yes it's ok! Please assign it.

As Benjamin had supposed, I'd like some help for testing and understanding GMP/OpenH264 (In particular: to activate it, I just have to activate the OpenH264 addon which was automatically downloaded, right? On what webpage could I test it? I tried [1], but video works whether or not addon is activated). Needinfo-ing Jesup again about this.

[1] http://www.html5videoplayer.net/html5video/mp4-h-264-video-test/

Comment 5

[Maire Reavy [:mreavy]]

Qeole - Thanks for taking this bug.  This is for webrtc (interactive) video, not media playback.  So you want to go to mozilla.github.com/webrtc-landing/pc_test_h264.html and press start.  If you see video in the 2 large windows (one will be from your camera, the other will be a rectangle of solid color that changes color), then H.264 video is working.  To test downloading, you probably want to start your browser with a fresh profile (start the browser with -profilemanager).  Look for OpenH264 in Tools->Add-ons.  NOTE: It takes a few minutes to download and install the plugin depending on your connection speed.

Comment 6

[Qeole]

Thanks Maire.
Actually I had also tried that, but I did not know how to interpret what I had seen.

So I tried again: with a normal profile directory (no Thai/Hebrew characters), and once OpenH264 plugin is downloaded and activated (by activated I mean I switch drop-down menu to “always activate”).
Everything is expected to work, but I get:
 * video in the 2 SMALL windows (the same as described in comment #5 for large windows: one from camera, the other with solid blue/green color);
 * a plain white window for the large window on the left;
 * large window on the right does not activate automatically (remains gray with a “play” triangular button). On activation click, it becomes white (and then looks the same as the left large window).
(I can include a screenshot if it's unclear.)

As I see nothing in the two large windows, I suppose it's not working. But then I do not know why. I've got some output below the windows; I don't know if it helps, but I pasted it there: http://pastebin.com/6bK6D4Dr (I only masked IP addresses).

(For info: as for profiles, mach run / mach debug know run with a new scratch profile by default; see bug 1012924 for details).

Any idea why it's not working, or what I could do to find the cause of this?

Comment 7

[Maire Reavy [:mreavy]]

Based on what you're describing, it sounds like the plugin failed to load (exactly as bsmedberg indicated).  It believed it had the plugin (which is why it offered H.264), but it couldn't load it and execute (that's why you didn't see video).

The code to find the plugin is here: GMPService.cpp in GeckoMediaPluginService::AddOnGMPThread()
The code to load the plugin begins here: GMPParent.cpp in GMPParent::LoadProcess()

If you have more questions, please keep asking in the bug -- or come find me (mreavy) or jesup in #media on irc.

Thanks for your help with this!

Comment 8

[Qeole]

Thanks Maire. Finally I made it work with Latin characters in profile directory name. For information, OpenH264 plugin is now enabled by default.

I confirm that it doesn't work with Hebrew or Thai characters in profile directory name (tried with "שלום_עולם" and "สวัสดีชาวโลก", both failed): plugin doesn't load, as Benjamin expected.

I've tried to fix it, but I've been unsuccessful so far. I tried to replace
  mLib = PR_LoadLibrary(nativePath.get());
with
  libFile->Load(&mLib);
but that's not enough (compiles but does not fix the issue), and I don't understand everything happening whith the strings here.

(In reply to Benjamin Smedberg  [:bsmedberg] Away 19-July through 3-Aug from comment #0)> 
> In other code the way we deal with this is that for Windows, we convert the
> unicode path to UTF8 in the parent and convert it back in the child. This
> represents the full unicode range.

Could someone indicate me where I could find such an example? Is it what I should do here?

> 
> PR_LoadLibrary should be replaced with nsIFile->Load which does the correct
> unicode magic.

I couldn't understand how exactly |nsIFile->Load| does this, or even what magic exactly it should performs. nsIFile seems to be part of the XPIDL code and is not completely implemented in C++ (or I couldn't find it), so where should I search for its code?

As long as I understand, we have GMPParent taking the directory name as a narrow (8-bit) string at [1] (is it the convertion to UTF-8 mentioned by Benjamin?), then the GMPChild gets this narrow string in [2] and use it for a nsIFile object, from which it is exctracted as a narrow string again in [3] to call the plugin; but on Windows, we would like to use a wide (16-bit) string to call the plugin library, am I right?
I don't know how could |libFile->Load(&mLib);| take part in that, any guidance would be appreciated :/

(Also I'm having an issue with Windows: when I edit source code, |mach build| does not take modifications into account (compiles even with garbage in files, and run previous code). I found nothing better than clobbering each time, and as you may guess it's pretty annoying. Anyone having a hint about how I could fix that?)

[1] https://hg.mozilla.org/mozilla-central/annotate/330ba968ed61/content/media/gmp/GMPParent.cpp#l62
[2] https://hg.mozilla.org/mozilla-central/annotate/330ba968ed61/content/media/gmp/GMPChild.cpp#l60
[3] https://hg.mozilla.org/mozilla-central/annotate/330ba968ed61/content/media/gmp/GMPChild.cpp#l83

Comment 9

[Maire Reavy [:mreavy]]

Sorry that I didn't respond sooner.  (It has been a super-busy few days!)  The questions you're asking are outside of my area.  bsmedberg is really the expert on this, but Georg (gfritzsche) may be able to help you;  so I'm needinfo'ing Georg.

Comment 10

[Georg Fritzsche [:gfritzsche]]

I'm short on time and certainly can't answer without some digging, so moving on to bsmedberg who is back on monday.

Comment 11

[Benjamin Smedberg]

> I've tried to fix it, but I've been unsuccessful so far. I tried to replace
>   mLib = PR_LoadLibrary(nativePath.get());
> with
>   libFile->Load(&mLib);
> but that's not enough (compiles but does not fix the issue), and I don't
> understand everything happening whith the strings here.

Yeah, it's not enough but it is necessary.

> Could someone indicate me where I could find such an example? Is it what I
> should do here?

The NPAPI plugins example is really hard to follow. Here's how we deal with main() on Windows: unicode->utf8: http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsWindowsWMain.cpp#77
utf8->nsIFile: http://mxr.mozilla.org/mozilla-central/source/toolkit/profile/nsToolkitProfileService.cpp#1054

> I couldn't understand how exactly |nsIFile->Load| does this, or even what
> magic exactly it should performs. nsIFile seems to be part of the XPIDL code
> and is not completely implemented in C++ (or I couldn't find it), so where
> should I search for its code?

You probably don't need to know, since you can just use nsIFile->Load, but here's the code: http://hg.mozilla.org/mozilla-central/annotate/71497ed2e0db/xpcom/io/nsLocalFileWin.cpp#l2279

> As long as I understand, we have GMPParent taking the directory name as a
> narrow (8-bit) string at [1] (is it the convertion to UTF-8 mentioned by
> Benjamin?)

This is where we get the narrow string, but on Windows it's not UTF8, it's the "native charset", which for English Windows is ANSI-1252. That's why this breaks. What we need to do here is use GetPath+UTF16ToUTF8 instead of GetNativePath.


, then the GMPChild gets this narrow string in [2] and use it for
> a nsIFile object, from which it is exctracted as a narrow string again in
> [3] to call the plugin; but on Windows, we would like to use a wide (16-bit)
> string to call the plugin library, am I right?

Correct. So at [2] we need to use UTF8ToUTF16 + InitWithPath instead of InitWithNativePath.

> (Also I'm having an issue with Windows: when I edit source code, |mach
> build| does not take modifications into account (compiles even with garbage
> in files, and run previous code). I found nothing better than clobbering
> each time, and as you may guess it's pretty annoying. Anyone having a hint
> about how I could fix that?)

That's really surprising. What editor are you using? The only thing I can think of is that version control and editor aren't using matching timestamps.

Comment 12

[Qeole]

Attachment: Converting GMP plugin path to UTF-8.patch

Thank you Benjamin for all those explanations. I got it, and managed to fix the bug (I enclose a patch).

I proceeded to following test: I built with the patch under Windows with French locale, then:
 * I launched Firefox with either שלום_עולם or สวัสดีชาวโลก as profile directory name 
 * I opened plugin-manager tab and waited for OpenH264 to automatically install
 * I went to test URL [1]
 * I started a call, and got expected result: video in both big windows, and “HIP HIP HIP HOORAY” at the bottom of page. No crash on exit. Still compiles and runs on my Linux.

Remark: Since bug was opened, GMPChild.cpp was modified and I can't remove declaration of |nativePath| on line 102 [2], which was only in use at line 111, but is now used on line 108 as well.
I don't know if this should be somehow written in a different way or if it's fine.

As for my compiling issue,
> That's really surprising. What editor are you using? The only thing I can think of is that version
> control and editor aren't using matching timestamps.
I'm using Notepad++, but it does not seem to be related to it. In the console I use to compile, timestamps (from ls -l) look consistent. Plus compiling does not take modification to source code induced by |hg qpush some_patch| either. Files *are* edited though: I double-checked it (and otherwise, even clobbering would turn useless).

[1] http://mozilla.github.io/webrtc-landing/pc_test_h264.html
[2] https://hg.mozilla.org/mozilla-central/file/bdf301b20cab/content/media/gmp/GMPChild.cpp#l102

Comment 13

[Benjamin Smedberg]

Comment on attachment 8468478 [details] [diff] [review]
Converting GMP plugin path to UTF-8.patch

The other use of nativePath in GMPChild::LoadPluginLibrary is #if defined(XP_LINUX) && defined(MOZ_GMP_SANDBOX)

So you could just move the nativePath getter within that block.

I think this is correct and does exactly what we need.

Comment 14

[Qeole]

I'll move these lines into said block immediately.
Should I push to Try servers after that?

Comment 15

[Benjamin Smedberg]

If you haven't already, then yes.

Comment 16

[Qeole]

Attachment: Converting GMP plugin path to UTF-8.patch

Here is updated patch.

When trying to push to try, I'm currently getting an error stating that tree try is closed.
I'll watch for re-opening and try again later.

Comment 17

[Qeole]

Here's the try link:
https://tbpl.mozilla.org/?tree=Try&rev=71e5f054e3d2

Oranges appearing on Treeherder (https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=71e5f054e3d2) for JetPack SDK seem to be related to bug 1049215.

Can I ask for checkin?

Comment 18

[Georg Fritzsche [:gfritzsche]]

This looks fine test-wise, go ahead with checkin.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e79ca6ca1173

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7d99b4caf042 because it was conflicting with the merge of mozilla-central back over to mozilla-inbound.

Jesup has volunteered to help review and mentor you in getting this rebased onto the current mozilla-inbound code.

Comment 21

[Qeole]

Attachment: Converting GMP plugin path to UTF-8 _ rebased.patch

I've made a first attempt at rebasing.

So my patch concerns GMPParent.cpp and GMPChild.cpp. I did not have to modify anything for GMPParent.cpp since my former patch, conflict was on GMPChild.cpp only.

In my former patch I would call |libFile->Load()| method to load the plugin. Since the code was modified, |libFile| instance declaration and initialization  was removed from |LoadPluginLibrary()| and moved to a new |GetPluginBinaryPath()| static method.
In this new patch I propose to split |GetPluginBinaryPath()| into two methods. The first one (I called it |GetPluginBinaryFile()|) handles nsIFile instance declaration and initialization. Second part remains |GetPluginBinaryPath()|, and it just calls the first part and extracts path name from nsIFile object.
In this way I can call |GetPluginBinaryFile()| inside |LoadPluginLibrary()| to get |libFile| back; while |GetPluginBinaryPath()| can still be called from other functions (in particular |OnChannelConnected()|.

Remark: with my patch |GetPluginBinaryPath()| becomes an unused function for builds other than for MacOSX.

I also had to move some preprocessing directives in |LoadPluginLibrary()|.

This new patch is also working (tested under the same conditions), but I haven't pushed to try yet.

Randell, Benjamin, could one of you provide feedback on selected approach, please?

Comment 22

[Benjamin Smedberg]

Comment on attachment 8471075 [details] [diff] [review]
Converting GMP plugin path to UTF-8 _ rebased.patch

Since smichaud wrote the conflicting patch in bug 1012949, he should probably provide the feedback on this. The approach looks fine in general.

It's a bit weird to have a nsCOMPtr<nsIFile>& for an outparam: typically we'd use nsIFile** as an outparam. But since this is all in one file, it's probably fine this way.

Comment 23

[Qeole]

All right. Smichaud said (in bug 1012949, comment #107) that he's gone this week, so it will wait until he's back.

Comment 24

[Georg Fritzsche [:gfritzsche]]

[Tracking Requested - why for this release]: This breaks OpenH264 usage on Windows for profiles with Unicode characters, but we want this in Firefox 33.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Tracking because we will have users using both OpenH264, Windows and unicode in ff33.

Comment 26

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8471075 [details] [diff] [review]
Converting GMP plugin path to UTF-8 _ rebased.patch

This looks fine to me.  (Though note that I haven't tested it.)

Comment 27

[Qeole]

Attachment: Converting GMP plugin path to UTF-8 _rebased-v2.patch

Thank you Steven.

Since a week has passed I had to make another (but minor) rebasing for GMPChild.cpp. Updated version of patch enclosed.

Patch works for me, and shows nothing wrong when pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=1bfc93a9f94d

Benjamin, could you please provide review again?

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/358058008e40

we'll want to uplift this soon

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/358058008e40

Comment 30

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8475826 [details] [diff] [review]
Converting GMP plugin path to UTF-8 _rebased-v2.patch

Approval Request Comment
[Feature/regressing bug #]: Needed for uplift of bug 1012949 (Mac GMP Sandboxing)
[User impact if declined]: FF 33 won't have Mac GMP sandboxing
[Describe test coverage new/current, TBPL]: Baked on m-c for about a week with no problems
[Risks and why]: Low risk (moderate for all bug 1012949 uplifts together)
[String/UUID change made/needed]: none

This patch should be uplifted together with the other patches listed in bug 1012949 comment #110.

Comment 31

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #30)

I should have added that this is also needed for Windows GMP sandboxing.

Comment 32

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8475826 [details] [diff] [review]
Converting GMP plugin path to UTF-8 _rebased-v2.patch

Landed on mozilla-aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/c85d1d97e2b4