Closed
Bug 1039886
Opened 11 years ago
Closed 11 years ago
GMPs are no longer segregated by origin but should be
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: cpearce, Assigned: eflores)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 5 obsolete files)
3.06 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
4.08 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
1.34 KB,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
I noticed that the code to segregate GMPs by origin that I landed in bug 1024300 has been removed. We need to fix this, and add a gtest with a test plugin to ensure that it stays fixed.
Comment 1•11 years ago
|
||
Which code? It looks like at least most of the code from bug 1024300 is still present.
Reporter | ||
Comment 2•11 years ago
|
||
The call to RefreshPluginList() in GeckoMediaPluginService::SelectPluginForAPI() was removed. This ensured that if a plugin instance was already assigned to work for an origin, a new instance would be created that could be assigned to a new origin. Explanation in bug 1024300 comment 10.
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #8461254 -
Flags: review?(cpearce)
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #8461255 -
Flags: review?(cpearce)
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #8461257 -
Flags: review?(cpearce)
Assignee | ||
Comment 6•11 years ago
|
||
Ran into this bug when we wait for GMP process shutdown.
Upstream has taken out the kill(SIGKILL) call completely, but I think it's better to simply guard it like this.
Attachment #8461261 -
Flags: review?(benjamin)
Reporter | ||
Comment 7•11 years ago
|
||
Comment on attachment 8461254 [details] [diff] [review]
Test for GMP cross-origin separation
Review of attachment 8461254 [details] [diff] [review]:
-----------------------------------------------------------------
Testerific!
I just landed a bunch of changes to the gmp-apis on inbound which will trivially bitrot your test plugin...
Attachment #8461254 -
Flags: review?(cpearce) → review+
Reporter | ||
Comment 8•11 years ago
|
||
Comment on attachment 8461255 [details] [diff] [review]
Cross-origin separation of GMPs
Review of attachment 8461255 [details] [diff] [review]:
-----------------------------------------------------------------
Almost...
::: content/media/gmp/GMPService.cpp
@@ +486,5 @@
> + mozilla::SyncRunnable::DispatchToThread(mainThread, task);
> + }
> +
> + nsRefPtr<GMPParent> gmp = task->GetParent();
> + nsresult rv = gmp->CloneFrom(aOriginal);
GMPParent::CloneFrom() asserts that you're on the GMP thread, but JS/C++ can call into this on the main/non-GMP thread by calling hasPluginForAPI() (which calls SelectPluginForAPI(), which calls this), and I don't think that you should be initializing a GMP on a thread other than the GMP thread; there's assertions to prevent that, and it does I/O.
I think you need to add a flag to SelectPluginForAPI() to denote whether it's OK to clone, and pass false in the hasPluginForAPI() call site, and true elsewhere. Then assert here that you're running on the GMP thread.
Attachment #8461255 -
Flags: review?(cpearce) → review-
Reporter | ||
Updated•11 years ago
|
Attachment #8461257 -
Flags: review?(cpearce) → review+
Comment 9•11 years ago
|
||
Comment on attachment 8461261 [details] [diff] [review]
Fix ProcessKill in chromium IPC code
Review of attachment 8461261 [details] [diff] [review]:
-----------------------------------------------------------------
r-, r+ if you add the boolean like they have. (Wouldn't hurt to check the routine for other substantive changes while you're at it. Our ipc/chromium code is ancient...)
::: ipc/chromium/src/base/process_util_posix.cc
@@ +79,5 @@
> sleep(1);
> }
>
> + if (!tries) {
> + result = kill(process_id, SIGKILL) == 0;
If tries is 1, and while(tries-- > 0) runs, tries will be 0. It may then succeed and break, but we'll assume it failed. See latest upstream code:
https://code.google.com/p/chrome-browser/source/browse/trunk/src/base/process_util_posix.cc#134
Attachment #8461261 -
Flags: review?(benjamin) → review-
Assignee | ||
Comment 10•11 years ago
|
||
green on try: https://tbpl.mozilla.org/?tree=Try&rev=656257b8ca92
Assignee | ||
Comment 11•11 years ago
|
||
Changed the test to depend on the existing gmp-fake instead of adding a new test gmp.
Attachment #8488453 -
Flags: review?(cpearce)
Assignee | ||
Updated•11 years ago
|
Attachment #8461254 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8488453 -
Attachment description: 1039886-tests.patch → Test for GMP cross-origin separation
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #8488454 -
Flags: review?(cpearce)
Assignee | ||
Updated•11 years ago
|
Attachment #8461255 -
Attachment is obsolete: true
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #8461261 -
Attachment is obsolete: true
Attachment #8488455 -
Flags: review?(rjesup)
Updated•11 years ago
|
Attachment #8488455 -
Flags: review?(rjesup) → review+
Reporter | ||
Updated•11 years ago
|
Attachment #8488454 -
Flags: review?(cpearce) → review+
Reporter | ||
Comment 14•11 years ago
|
||
Comment on attachment 8488453 [details] [diff] [review]
Test for GMP cross-origin separation
Review of attachment 8488453 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/media/gtest/TestGMPCrossOrigin.cpp
@@ +49,5 @@
> +
> + EXPECT_TRUE(host);
> + EXPECT_TRUE(decoder);
> + EXPECT_TRUE(decoder2);
> + EXPECT_TRUE(encoder);
You should call Close() on the proxies when you're done with them, unless your deliberately trying to leak them.
@@ +81,5 @@
> +
> + mService->GetGMPVideoEncoder(&tags, NS_LITERAL_STRING("origin1"), &host, &encoder2);
> + EXPECT_TRUE(!!encoder1 && !!encoder2 &&
> + encoder1->ParentID() == encoder2->ParentID());
> +}
Also test that two decoders with an empty origin string are the same.
And remember to Close() the decoders here again too.
Attachment #8488453 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 15•11 years ago
|
||
This patch makes |mach gtest| set the path to the GMP and to the plugin-container binary in the environment so we can test GMP plugins.
Attachment #8493530 -
Flags: review?(gps)
Assignee | ||
Comment 16•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8493530 -
Attachment is obsolete: true
Attachment #8493530 -
Flags: review?(gps)
Assignee | ||
Updated•11 years ago
|
Attachment #8488453 -
Attachment is obsolete: true
Comment 17•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5509ae0e4b80
https://hg.mozilla.org/mozilla-central/rev/ed858dc6599c
https://hg.mozilla.org/mozilla-central/rev/cbcf41cc455c
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Reporter | ||
Comment 18•10 years ago
|
||
Mass update firefox-status to track EME uplift.
You need to log in
before you can comment on or make changes to this bug.
Description
•