1039886 - GMPs are no longer segregated by origin but should be

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

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

[Benjamin Smedberg]

Which code? It looks like at least most of the code from bug 1024300 is still present.

Comment 2

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

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.

Comment 3

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Test for GMP cross-origin separation

Comment 4

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Cross-origin separation of GMPs

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Add some warnings to GMP loading code

Comment 6

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Fix ProcessKill in chromium IPC code

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.

Comment 7

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

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...

Comment 8

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

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.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 10

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

green on try: https://tbpl.mozilla.org/?tree=Try&rev=656257b8ca92

Comment 11

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Test for GMP cross-origin separation

Changed the test to depend on the existing gmp-fake instead of adding a new test gmp.

Comment 12

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Cross-origin separation of GMPs

Comment 13

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Update ProcessKill logic to match upstream

Comment 14

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

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.

Comment 15

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Fix TestGMPCrossOrigin when run with |mach gtest|

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.

Comment 16

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5509ae0e4b80
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed858dc6599c
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbcf41cc455c

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5509ae0e4b80
https://hg.mozilla.org/mozilla-central/rev/ed858dc6599c
https://hg.mozilla.org/mozilla-central/rev/cbcf41cc455c

Comment 18

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

Mass update firefox-status to track EME uplift.