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

RESOLVED FIXED in Firefox 33

Status

()

Core
WebRTC: Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Benjamin Smedberg, Assigned: qeole, Mentored)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla34
All
Windows 7
Points:
2
Dependency tree / graph

Firefox Tracking Flags

(firefox33+ fixed, firefox34+ fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
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.
Flags: needinfo?(rjesup)
Flags: needinfo?(qeole)
(Assignee)

Comment 1

3 years ago
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.
Flags: needinfo?(qeole)
(Assignee)

Comment 2

3 years ago
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.
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.
Flags: needinfo?(rjesup) → needinfo?(qeole)
(Assignee)

Comment 4

3 years ago
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/
Flags: needinfo?(qeole) → needinfo?(rjesup)

Updated

3 years ago
Assignee: nobody → qeole
Status: NEW → ASSIGNED
Flags: needinfo?(rjesup)
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.
Whiteboard: [openh264-uplift]
(Assignee)

Comment 6

3 years ago
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?
Flags: needinfo?(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!
Flags: needinfo?(mreavy)
QA Contact: drno
(Assignee)

Comment 8

3 years ago
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
Flags: needinfo?(rjesup)
Flags: needinfo?(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.
Flags: needinfo?(mreavy) → needinfo?(georg.fritzsche)
I'm short on time and certainly can't answer without some digging, so moving on to bsmedberg who is back on monday.
Flags: needinfo?(georg.fritzsche) → needinfo?(benjamin)
(Reporter)

Comment 11

3 years ago
> 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.
Mentor: benjamin@smedbergs.us
Flags: needinfo?(benjamin)
(Assignee)

Comment 12

3 years ago
Created attachment 8468478 [details] [diff] [review]
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
Attachment #8468478 - Flags: feedback?(benjamin)
Flags: needinfo?(rjesup)
(Reporter)

Comment 13

3 years ago
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.
Attachment #8468478 - Flags: feedback?(benjamin) → review+
(Assignee)

Comment 14

3 years ago
I'll move these lines into said block immediately.
Should I push to Try servers after that?
Flags: needinfo?(benjamin)
(Reporter)

Comment 15

3 years ago
If you haven't already, then yes.
Flags: needinfo?(benjamin)
(Assignee)

Comment 16

3 years ago
Created attachment 8469501 [details] [diff] [review]
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.
Attachment #8468478 - Attachment is obsolete: true
(Assignee)

Comment 17

3 years ago
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?
Flags: needinfo?(benjamin)
This looks fine test-wise, go ahead with checkin.
Flags: needinfo?(benjamin)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/e79ca6ca1173
Keywords: checkin-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.
Flags: needinfo?(rjesup)
Flags: needinfo?(qeole)
(Assignee)

Comment 21

3 years ago
Created attachment 8471075 [details] [diff] [review]
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?
Attachment #8469501 - Attachment is obsolete: true
Attachment #8471075 - Flags: feedback?(rjesup)
Attachment #8471075 - Flags: feedback?(benjamin)
Flags: needinfo?(qeole)
(Reporter)

Comment 22

3 years ago
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.
Attachment #8471075 - Flags: feedback?(benjamin) → feedback?(smichaud)
(Assignee)

Comment 23

3 years ago
All right. Smichaud said (in bug 1012949, comment #107) that he's gone this week, so it will wait until he's back.
[Tracking Requested - why for this release]: This breaks OpenH264 usage on Windows for profiles with Unicode characters, but we want this in Firefox 33.
status-firefox33: --- → affected
status-firefox34: --- → affected
tracking-firefox33: --- → ?
Tracking because we will have users using both OpenH264, Windows and unicode in ff33.
tracking-firefox33: ? → +
tracking-firefox34: --- → +
Blocks: 1053727

Updated

3 years ago
Attachment #8471075 - Flags: feedback?(rjesup) → feedback+
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.)
Attachment #8471075 - Flags: feedback?(smichaud) → feedback+
(Assignee)

Comment 27

3 years ago
Created attachment 8475826 [details] [diff] [review]
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?
Attachment #8471075 - Attachment is obsolete: true
Attachment #8475826 - Flags: review?(benjamin)
(Reporter)

Updated

3 years ago
Attachment #8475826 - Flags: review?(benjamin) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Flags: needinfo?(rjesup)
https://hg.mozilla.org/integration/mozilla-inbound/rev/358058008e40

we'll want to uplift this soon
Target Milestone: --- → mozilla34
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/358058008e40
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox34: affected → fixed
Resolution: --- → FIXED
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.
Attachment #8475826 - Flags: approval-mozilla-aurora?
(Following up comment #30)

I should have added that this is also needed for Windows GMP sandboxing.
Attachment #8475826 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
status-firefox33: affected → fixed

Updated

3 years ago
Whiteboard: [openh264-uplift]
You need to log in before you can comment on or make changes to this bug.