Closed
Bug 1190252
Opened 9 years ago
Closed 9 years ago
[EME] Loading GMPs can fail due to use of std:string and std:ifstream in GMP child process
Categories
(Core :: Audio/Video: MediaStreamGraph, defect, P1)
Core
Audio/Video: MediaStreamGraph
Tracking
()
RESOLVED
FIXED
mozilla42
People
(Reporter: cpearce, Assigned: cpearce)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
10.79 KB,
patch
|
eflores
:
review+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
10.92 KB,
patch
|
eflores
:
review+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
7.33 KB,
patch
|
mozbugz
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
We should not mix std strings and I/O where possible into Gecko code. We've had Adobe's GMP fail to load when the in the path to the GMP dir has non-ASCII characters in it due to the characters being mangled. We're still seeing loading Adobe's GMP failing, so I'm suspecting it's because we're using std::ifstream in GMPChild::PreLoadLibraries(), PreLoadPluginVoucher() and PreLoadSandboxVoucher(). I assume this problem was fixed years ago in NSPR file handling, so removing std string and ifstream from GMPChild will eliminate this as a possibility.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8642248 -
Flags: review?(edwin)
Assignee | ||
Comment 2•9 years ago
|
||
Remove std::ifstream usage. We can't use nsILineInputStream etc for I/O as that requires XPCOM to be initialized. So I had to roll my own split functions.
Attachment #8642249 -
Flags: review?(edwin)
Assignee | ||
Comment 3•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a64a4ee3efee
Attachment #8642249 -
Flags: review?(edwin) → review+
Attachment #8642248 -
Flags: review?(edwin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba77cb6046b7 https://hg.mozilla.org/integration/mozilla-inbound/rev/880fc4c49f7e
Assignee | ||
Updated•9 years ago
|
Summary: [EME] GMPChild should not use std:string and std:ifstream → [EME] Loading GMPs can fail due to use of std:string and std:ifstream in GMP child process
Backed out for Windows gtest failures in https://hg.mozilla.org/integration/mozilla-inbound/rev/fa8755511ac2 https://treeherder.mozilla.org/logviewer.html#?job_id=12440970&repo=mozilla-inbound
Assignee | ||
Comment 7•9 years ago
|
||
Comment on attachment 8642249 [details] [diff] [review] Patch 2: Use nsIFile and NSPR I/O instead of std::io Review of attachment 8642249 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/gmp/GMPChild.cpp @@ +334,5 @@ > +{ > + nsTArray<uint8_t> buf; > + bool rv = ReadIntoArray(aFile, buf, aMaxLength); > + if (rv) { > + aOutDst = nsDependentCString((const char*)buf.Elements(), buf.Length()); nsDependentCString is throwing because I'm passing a non-null terminated buffer here. So insert: buf.AppendElement(0); // Append null terminator, required by nsC*String. @@ +400,4 @@ > } > + // Line starts with "libraries:". > + nsTArray<nsCString> libs = SplitAt(Tokenizer::Token::Char(','), > + Substring(line, offset)); That Substring needs to be: Substring(line, offset + strlen("libraries:")) So that the first whiteListedLib below is "dxva2.dll" and not "libraries: dxva2.dll" when parsing clearkey.info.
Assignee | ||
Comment 8•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=49a3f4dc9f00
https://hg.mozilla.org/integration/mozilla-inbound/rev/064871541f3d https://hg.mozilla.org/integration/mozilla-inbound/rev/ad6eae12a195
Comment 10•9 years ago
|
||
Chris - just so I understand a bit better -- is it a problem if the GMP plugin itself is using std:string? We should not be using std:ifstream AFAIK, but std:string is being used.
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Joe Steele from comment #10) > Chris - just so I understand a bit better -- is it a problem if the GMP > plugin itself is using std:string? We should not be using std:ifstream > AFAIK, but std:string is being used. I don't believe your GMP using std::string would be a problem. I believe the problems we have are caused by non-ASCII characters being in the paths to files (your DLL, voucher, and the plugin-container voucher) and the use of std::string somehow munging the characters, and/or std::ifstream internally fails to load paths with those characters in them. Since your GMP doesn't do any of that, and indeed probably won't ever be exposed to any input which could non 8-bit bytes/characters, I think you don't need to worry about it.
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/064871541f3d https://hg.mozilla.org/mozilla-central/rev/ad6eae12a195
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Assignee | ||
Comment 13•9 years ago
|
||
41 (Aurora ) Try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=f457452dbf36
Assignee | ||
Comment 14•9 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #13) > 41 (Aurora ) Try push > https://treeherder.mozilla.org/#/jobs?repo=try&revision=f457452dbf36 This push failed to build because mozilla/Tokenizer.h is not available on Aurora yet...
Assignee | ||
Comment 15•9 years ago
|
||
Switch from the only-in-Nightly mozilla/Tokenizer to NS_strtok. Note NS_strtok doesn't rely on static storage, so it's thread safe. Also included a gtest for my line-break code. Edwin's probably still sitting on a plane, so r?gerald.
Attachment #8645513 -
Flags: review?(gsquelart)
Attachment #8645513 -
Flags: review?(gsquelart) → review+
Assignee | ||
Comment 18•9 years ago
|
||
Backed out because clang MacOSX doesn't like my std::vector<std::string> initializer list, so I've removed them: https://treeherder.mozilla.org/#/jobs?repo=try&revision=28fa781f757c
Assignee | ||
Comment 19•9 years ago
|
||
With unified build failures fixed... https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cba0e0ae82e
Assignee | ||
Comment 22•9 years ago
|
||
Just to clarify the situation here, there was two landings; the main fix, and a follow up to make it build in 41. The follow up to make it build in 41 landed after the uplift. These changes are in 42: https://hg.mozilla.org/integration/mozilla-inbound/rev/064871541f3d https://hg.mozilla.org/integration/mozilla-inbound/rev/ad6eae12a195 This change is in 43: https://hg.mozilla.org/mozilla-central/rev/b38760515fda Therefore... I need to uplift to 42: b38760515fda I need to uplift to 41: 064871541f3d, ad6eae12a195, and b38760515fda
status-firefox40:
--- → wontfix
status-firefox41:
--- → affected
Assignee | ||
Comment 23•9 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #22) > I need to uplift to 42: b38760515fda Auror 42 Try push of b38760515fda: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa6c51bf0941 (Green) > I need to uplift to 41: 064871541f3d, ad6eae12a195, and b38760515fda Beta 41 Try push of 064871541f3d, ad6eae12a195, and b38760515fda https://treeherder.mozilla.org/#/jobs?repo=try&revision=318b2c34a8f3 (Only jobs that are not run on our actual beta branches are failing here, i.e. we run boxes on Try that we don't run on Beta, and those extra boxes are failing).
Assignee | ||
Comment 24•9 years ago
|
||
Comment on attachment 8642248 [details] [diff] [review] Patch 1: Remove std::string from GMPChild and friends Approval Request Comment [Feature/regressing bug #]: EME [User impact if declined]: Users who have installed Firefox into a directory with non-ASCII characters in the path may fail to start up playback of EME protected videos. We're seeing some EME video playback failures due to failing to load some files out of the Firefox install directory, and I think this patch is likely to fix them. [Describe test coverage new/current, TreeHerder]: We have lots of tests covering the area of code touched (GMP loading). [Risks and why]: Low. This patch just changes how we store some file paths in memory, so use APIs that we've used for years. [String/UUID change made/needed]: None.
Attachment #8642248 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 25•9 years ago
|
||
Comment on attachment 8642249 [details] [diff] [review] Patch 2: Use nsIFile and NSPR I/O instead of std::io Approval Request Comment [Feature/regressing bug #]: EME [User impact if declined]: Users who have installed Firefox into a directory with non-ASCII characters in the path may fail to start up playback of EME protected videos. We're seeing some EME video playback failures due to failing to load some files out of the Firefox install directory, and I think this patch is likely to fix them. [Describe test coverage new/current, TreeHerder]: We have lots of tests covering the area of code touched (GMP loading). [Risks and why]: Low. This patch just changes how we store some read files out of the Firefox install dir, to use APIs that we've used for years. See comment 23 for Green try pushes. [String/UUID change made/needed]: None.
Attachment #8642249 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 26•9 years ago
|
||
Comment on attachment 8645513 [details] [diff] [review] Patch 3: Switch from mozilla/Tokenizer to NS_strtok Approval Request Comment [Feature/regressing bug #]: EME [User impact if declined]: Users who have installed Firefox into a directory with non-ASCII characters in the path may fail to start up playback of EME protected videos. We're seeing some EME video playback failures due to failing to load some files out of the Firefox install directory, and I think this patch is likely to fix them. [Describe test coverage new/current, TreeHerder]: We have lots of tests covering the area of code touched (GMP loading). [Risks and why]: Low. This patch just changes how we store some read the EME plugin metadata file to use APIs that are not Gecko42-and-later only; i.e. this patch makes the earlier patches build on Firefox41. See comment 23 for Green try pushes. [String/UUID change made/needed]: None. (Note this patch is needed on 42 and 41, whereas the other two uplift requests only need to be uplifted to 41 since they're already in 42)
Attachment #8645513 -
Flags: approval-mozilla-beta?
Attachment #8645513 -
Flags: approval-mozilla-aurora?
Comment on attachment 8645513 [details] [diff] [review] Patch 3: Switch from mozilla/Tokenizer to NS_strtok This patch is pretty big, let's uplift to Aurora first and then wait a day or two before we uplift to Beta. Fix has baked in m-c for a week, seems safe to land on Aurora.
Attachment #8645513 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8642248 [details] [diff] [review] Patch 1: Remove std::string from GMPChild and friends Approved for uplift to Beta. We want this for EME/Netflix testing in FF41 Beta2. Try pushes look clean.
Attachment #8642248 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8642249 [details] [diff] [review] Patch 2: Use nsIFile and NSPR I/O instead of std::io Approved.
Attachment #8642249 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8645513 [details] [diff] [review] Patch 3: Switch from mozilla/Tokenizer to NS_strtok Approved for Beta as this code has been in Aurora for ~2 days. We want this fix in 41.0 Beta1 for EME testing by Netflix.
Attachment #8645513 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 32•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/0b269e40d0c3 https://hg.mozilla.org/releases/mozilla-beta/rev/7128ab30f910 https://hg.mozilla.org/releases/mozilla-beta/rev/4f6dcfdc2801
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(cpearce)
You need to log in
before you can comment on or make changes to this bug.
Description
•