Closed Bug 1184333 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla42
Tracking Status
firefox39 --- wontfix
firefox40 + verified
firefox41 + verified
firefox42 --- verified

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
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.
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.
Attachment #8634482 - Flags: review?(edwin)
* 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.
Attachment #8634540 - Flags: review?(bobowen.code)
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 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. :)
Attachment #8634540 - Flags: review?(bobowen.code) → review+
(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.
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/409277668aa8
https://hg.mozilla.org/mozilla-central/rev/c0a0b67691d5
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
[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 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.
Attachment #8634482 - Flags: approval-mozilla-beta?
Attachment #8634482 - Flags: approval-mozilla-aurora?
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.
Attachment #8634540 - Flags: approval-mozilla-beta?
Attachment #8634540 - Flags: approval-mozilla-aurora?
Must remember to uplift.
Flags: needinfo?(cpearce)
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.
Attachment #8634482 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8634540 [details] [diff] [review]
Patch 2: Ensure we use UTF8/16 GMP paths

Let's land this patch in Aurora.
Attachment #8634540 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Requesting QE team to add this to their EME-related test plan and verify this fix works as expected.
Flags: qe-verify+
Given that this is medium risk and may potentially break OpenH264, I'd like to see the fix verified before uplifting to Beta.
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.
Flags: needinfo?(cpearce)
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+
Attachment #8634482 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8634540 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Setting the verified flag for Firefox 42 based on comment 21. This will also be verified in Firefox 40 Beta.
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Blocks: 1187031
See Also: → 1416646
You need to log in before you can comment on or make changes to this bug.