Closed Bug 1039886 Opened 8 years ago Closed 7 years ago

GMPs are no longer segregated by origin but should be

Categories

(Core :: Audio/Video, defect)

29 Branch
x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: cpearce, Assigned: eflores)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 5 obsolete files)

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.
Which code? It looks like at least most of the code from bug 1024300 is still present.
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.
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)
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+
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-
Attachment #8461257 - Flags: review?(cpearce) → review+
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-
Changed the test to depend on the existing gmp-fake instead of adding a new test gmp.
Attachment #8488453 - Flags: review?(cpearce)
Attachment #8461254 - Attachment is obsolete: true
Attachment #8488453 - Attachment description: 1039886-tests.patch → Test for GMP cross-origin separation
Attachment #8461255 - Attachment is obsolete: true
Attachment #8488455 - Flags: review?(rjesup) → review+
Attachment #8488454 - Flags: review?(cpearce) → review+
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+
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)
Attachment #8493530 - Attachment is obsolete: true
Attachment #8493530 - Flags: review?(gps)
Attachment #8488453 - Attachment is obsolete: true
Mass update firefox-status to track EME uplift.
You need to log in before you can comment on or make changes to this bug.