1184333 - [EME] GMP loading fails if Chinese characters in GMP dir path

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

If there is Chinese characters in the path to the GMP dir (for example in either the user's Windows username or in the Firefox profile name), GMP loading fails, and the GMP crashes.

In Firefox 39 Release we fallback to Silverlight on Netflix. In 40, we get the crash and the Netfix page seems to hang forever. This seems to be because the GMPTrialVideoCreator isn't getting the crash report, and navigator.requestMediaKeySystemAccess()'s promise is never being resolved.

STR:
1. Start Firefox and create a new profile with Chinese characters in it (e.g. 你好).
2. Open http://drmtest2.adobe.com/HTML5AAXSPlayer/2/mse-access/
3. Click "Play" button.

Expected results: Alice in wonderland trailer video plays.

Observed results: Adobe CDM crashes, a crash report notification box drops down from browser chrome.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

The problem here is that we get the path to the GMP DLL from the command line passed to plugin-container.exe as a "system" wide string. We then convert it to UTF8 in GMPProcessChild::Init(), and pass that to GMPChild. It gets converted into a "native" 8-bit string in GMPChild::GetLibPath(). Presumably this mangles it. Then once we've passed that native 8-bit string to GMPLoader, we use that in PR_LoadLibraryWithFlags(), which fails.

Other callers of PR_LoadLibraryWithFlags() set PRLibSpec::type to PR_LibSpec_PathnameU on Windows, and use a wide string, so we can take the result of the MultiByteToWideChar call inside GMPLoaderImpl::Load() and use that as the path. We need to change the MultiByteToWideChar call from CP_ACP to CP_UTF8 to get that to work.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 1: Handle CDM crashing during trial create

If the GMP child process crashes or aborts during trial create (for example if it can't find the DLL for example), if the timing's right, we can end up failing to get a GMPVideoEncodedFrame from the GMPVideoHost inside TestGMPVideoDecoder::CreateFrame().

We need to null check the frame we get back from TestGMPVideoDecoder::CreateFrame(), to ensure Gecko doesn't crash when this happens.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2: Ensure we use UTF8/16 GMP paths

* The code in GMPChild::PreLoadLibraries() to read the GMPs' .info from the profile dir file is working on paths with Chinese characters, so I copied that pattern over to the function to read the plugin and sandbox vouchers.
* Be careful to always pass around UTF8 paths.
* Use the wide string path used to init the sandbox in GMPLoaderImpl::Load() as the lib to load.

I'm not sure how to write an automated test for this; I haven't managed to get MOZ_GMP_PATH to work with non-ASCII paths, and our test harnesses rely on that.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

I just got a crash in 39 when loading Netflix with Chinese characters in the profile name.
https://crash-stats.mozilla.com/report/index/2052c8bd-3fe1-414e-a475-2548a2150716

It's the same GMPChild::ProcessingError() signature as in bug 1164245.

Comment 5

[Bob Owen (:bobowen)]

Comment on attachment 8634540 [details] [diff] [review]
Patch 2: Ensure we use UTF8/16 GMP paths

Review of attachment 8634540 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know the string handling or GMP code too well, but I think I followed all this correctly and it looks OK to me.

(In reply to Chris Pearce (:cpearce) from comment #3)

> I'm not sure how to write an automated test for this; I haven't managed to
> get MOZ_GMP_PATH to work with non-ASCII paths, and our test harnesses rely
> on that.

I think you'd need to use _wputenv and _wgetenv, but there may be other complications.

::: dom/media/gmp/GMPChild.cpp
@@ +275,5 @@
>    SendPCrashReporterConstructor(CrashReporter::CurrentThreadId());
>  #endif
>  
>    mPluginPath = aPluginPath;
> +  mSandboxVoucherPath = aVoucherPath;

Is this really a voucher for just the sandbox, I thought it was for the whole plugin-container?
But I guess you're just distinguishing this from the plugin's own voucher.

@@ +634,5 @@
>    nsCOMPtr<nsIFile> voucherFile;
>    GetPluginVoucherFile(aPluginPath, voucherFile);
>  
> +  if (!FileExists(voucherFile)) {
> +    return true;

I'm guessing this needs to be true because not all GMPs have vouchers.

If the voucher is missing for EME, do we get a different error later on?

@@ +683,5 @@
> +  NS_ConvertUTF8toUTF16 utf16Path(utf8Path);
> +  stream.open(static_cast<const wchar_t*>(utf16Path.get()), std::ios::binary);
> +  #else
> +  stream.open(mSandboxVoucherPath.c_str(), std::ios::binary);
> +  #endif

If I understand correctly, this is the voucher that lives in the same directory as plugin-containter.exe.
So, I guess the path for this is less likely to cause problems, but it could if people have done a custom install.

In fact don't we get this originally as a wstring on Windows.
Although I suppose that would mean holding and passing the command line arg as a different type on Windows.

::: dom/media/gmp/GMPLoader.cpp
@@ +200,5 @@
>    // If the GMP DLL is a side-by-side assembly with static imports then the DLL
>    // loader will attempt to create an activation context which will fail because
>    // of the sandbox. If we create an activation context before we start the
>    // sandbox then this one will get picked up by the DLL loader.
> +  int pathLen = MultiByteToWideChar(CP_UTF8, 0, aUTF8LibPath, -1, nullptr, 0);

I wonder if this is what has been causing the problem with this on some configuration and Win10, that meant we couldn't use the USER_LOCKDOWN token anymore.

@@ +205,5 @@
>    if (pathLen == 0) {
>      return false;
>    }
>  
> +  nsAutoArrayPtr<wchar_t> widePath(new wchar_t[pathLen]);

I wish I'd know about this class when I wrote this. :)

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Bob Owen (:bobowen) from comment #5)
> Comment on attachment 8634540 [details] [diff] [review]
> Patch 2: Ensure we use UTF8/16 GMP paths
> 
> Review of attachment 8634540 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't know the string handling or GMP code too well, but I think I
> followed all this correctly and it looks OK to me.
> 
> (In reply to Chris Pearce (:cpearce) from comment #3)
> 
> > I'm not sure how to write an automated test for this; I haven't managed to
> > get MOZ_GMP_PATH to work with non-ASCII paths, and our test harnesses rely
> > on that.
> 
> I think you'd need to use _wputenv and _wgetenv, but there may be other
> complications.

Thanks! Will check those out.

> ::: dom/media/gmp/GMPChild.cpp
> @@ +275,5 @@
> >    SendPCrashReporterConstructor(CrashReporter::CurrentThreadId());
> >  #endif
> >  
> >    mPluginPath = aPluginPath;
> > +  mSandboxVoucherPath = aVoucherPath;
> 
> Is this really a voucher for just the sandbox, I thought it was for the
> whole plugin-container?
> But I guess you're just distinguishing this from the plugin's own voucher.

You thought correctly, this is the voucher for plugin-container.exe, not gmp-eme-adobe.dll. I kept getting the two vouchers confused, so I renamed this one to make it clearer.


> 
> @@ +634,5 @@
> >    nsCOMPtr<nsIFile> voucherFile;
> >    GetPluginVoucherFile(aPluginPath, voucherFile);
> >  
> > +  if (!FileExists(voucherFile)) {
> > +    return true;
> 
> I'm guessing this needs to be true because not all GMPs have vouchers.

Yup. People also kept complaining about the warning emitted when we didn't find a voucher for plugins that didn't need a voucher. Now that won't happen.

> If the voucher is missing for EME, do we get a different error later on?

We should actually fail earlier on, I have code that explicitly checks that the voucher is there before we start the GMP child process for Adobe EME (and Adobe EME was failing with an error saying the plugin voucher was missing, which is why I went looking and found this bug).


> 
> @@ +683,5 @@
> > +  NS_ConvertUTF8toUTF16 utf16Path(utf8Path);
> > +  stream.open(static_cast<const wchar_t*>(utf16Path.get()), std::ios::binary);
> > +  #else
> > +  stream.open(mSandboxVoucherPath.c_str(), std::ios::binary);
> > +  #endif
> 
> If I understand correctly, this is the voucher that lives in the same
> directory as plugin-containter.exe.
> So, I guess the path for this is less likely to cause problems, but it could
> if people have done a custom install.

Yeah, this is insurance against custom installs.


> In fact don't we get this originally as a wstring on Windows.
> Although I suppose that would mean holding and passing the command line arg
> as a different type on Windows.

Yup, the command line arg is a wstring. I think ideally we should change all of this code to use nsStrings and XPCOM file I/O, as that should be less likely to do spurious conversions, and be consistent with the rest of our codebase... I feel like Gecko solved these problem 10 years ago, and we tripped up on it because we tried to use non-Gecko based strings and I/O. However I didn't want to make such a big patch and rewrite all this code, as that's a riskier uplift.

Comment 7

[Bob Owen (:bobowen)]

(In reply to Chris Pearce (:cpearce) from comment #6)
 
> Yup, the command line arg is a wstring. I think ideally we should change all
> of this code to use nsStrings and XPCOM file I/O, as that should be less
> likely to do spurious conversions, and be consistent with the rest of our
> codebase... I feel like Gecko solved these problem 10 years ago, and we
> tripped up on it because we tried to use non-Gecko based strings and I/O.
> However I didn't want to make such a big patch and rewrite all this code, as
> that's a riskier uplift.

Makes sense, given the uplift.
Although, I think we might hit issues trying to use Gecko based strings for some of this as it's in plugin-container not xul.
I tried when I first wrote the widePath part and had to resort to MultiByteToWideChar, but that could just have been my lack of knowledge.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a167684aba0c

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Bob Owen (:bobowen) from comment #7)
> Although, I think we might hit issues trying to use Gecko based strings for
> some of this as it's in plugin-container not xul.

I had the same issue with anything in plugin-container; the GMPLoader code for example. It's not just you! ;)

I wasn't clear, I meant GMPChild and friends, which live inside xul.dll.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/409277668aa8
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0a0b67691d5

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/409277668aa8
https://hg.mozilla.org/mozilla-central/rev/c0a0b67691d5

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

[Tracking Requested - why for this release]:

We'll need this fixed for EME; anyone with non-ASCII characters in their Windows username or Firefox profile names will not be able to use EME, or OpenH264 for that matter.


Beta Try Push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fb701aad443

Aurora Try Push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=75988801249c

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8634482 [details] [diff] [review]
Patch 1: Handle CDM crashing during trial create

Approval Request Comment
[Feature/regressing bug #]: EME

[User impact if declined]: Firefox may crash if the user has either a Windows username with contains non-ASCII characters in it when starting Netflix, or if the path to their Firefox profile directory otherwise contains non-ASCII characters.

[Describe test coverage new/current, TreeHerder]: We don't test non ASCII usernames in our test harnesses AFAICT. We have lots of mochitests covering EME, so I'm confident this doesn't break our existing EME code.

[Risks and why]: Low-to-medium. This changes our GMP loading code, so mistakes here could totally break EME and OpenH264. 

We should get QA to verify this on Netflix and in the OpenH264 test page once it lands (I've already verified this patch fixes the bug in Nightly).

[String/UUID change made/needed]: None.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8634540 [details] [diff] [review]
Patch 2: Ensure we use UTF8/16 GMP paths

Approval Request Comment
[Feature/regressing bug #]: EME

[User impact if declined]: Firefox may fail to start Netflix if the user has either a Windows username with contains non-ASCII characters in it when starting Netflix, or if the path to their Firefox profile directory otherwise contains non-ASCII characters.

[Describe test coverage new/current, TreeHerder]: We don't test non ASCII usernames in our test harnesses AFAICT. We have lots of mochitests covering EME, so I'm confident this doesn't break our existing EME code.

[Risks and why]: Low-to-medium. This changes our GMP loading code, so mistakes here could totally break EME and OpenH264. 

We should get QA to verify this on Netflix and in the OpenH264 test page once it lands (I've already verified this patch fixes the bug in Nightly).

[String/UUID change made/needed]: None.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Must remember to uplift.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8634482 [details] [diff] [review]
Patch 1: Handle CDM crashing during trial create

Let's land this patch in Aurora based on the risk assessment provided by dev.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8634540 [details] [diff] [review]
Patch 2: Ensure we use UTF8/16 GMP paths

Let's land this patch in Aurora.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Requesting QE team to add this to their EME-related test plan and verify this fix works as expected.

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/832a06a3935f
https://hg.mozilla.org/releases/mozilla-aurora/rev/dbd1d3a58d3f

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

Given that this is medium risk and may potentially break OpenH264, I'd like to see the fix verified before uplifting to Beta.

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

I have tested the fix in Nightly and verified that OpenH264 still works and that Adobe EME still works and that it works with Chinese characters in the pathname to the plugin.

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8634482 [details] [diff] [review]
Patch 1: Handle CDM crashing during trial create

Chris has tested this fix on Nightly. Let's get this into beta6. Beta+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c8499fa138a9
https://hg.mozilla.org/releases/mozilla-beta/rev/088c2c8e4bec

Comment 24

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Setting the verified flag for Firefox 42 based on comment 21. This will also be verified in Firefox 40 Beta.

Comment 25

[Ada [:adalucinet]]

Reproduced on 40.0a2 from 2015-06-29 under Windows 7 64-bit with STR from comment 0: bp-6285f05c-f981-4b76-a0b1-c9fab2150722 
Verified fixed on 40.0b6 (Build ID: 20150720220238) and latest 41.0a2 (from 2015-07-21) under Windows 7 64-bit and Windows 10 32-bit: Openh264 is properly offered via http://mozilla.github.io/webrtc-landing/pc_test_h264.html and Adobe EME via http://drmtest2.adobe.com/HTML5AAXSPlayer/2/mse-access/ and Netflix.