Closed Bug 1072024 Opened 5 years ago Closed 5 years ago

Test for cross-origin separation of GMPs

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: eflores, Assigned: eflores)

References

Details

Attachments

(4 files, 2 obsolete files)

Follow-up from bug 1039886. Mostly done, but doesn't work on linux debug build slaves due to the ancient OS we run on them.
Attached patch 1039886-tests.patch (obsolete) — Splinter Review
Carry over r+ from bug 1039886
Attachment #8494136 - Flags: review+
Comment on attachment 8494138 [details] [diff] [review]
Fix TestGMPCrossOrigin when run with |mach gtest|

Looks like this needs to be a cpptest as the build machines don't support sandboxing.
Attachment #8494138 - Attachment is obsolete: true
Attachment #8494138 - Flags: review?(gps)
Wasted enough time here. Hack around ancient build infra for the meantime.
Attachment #8495608 - Flags: review?(cpearce)
Attachment #8495608 - Flags: review?(cpearce) → review+
Jed: FYI, we're also having to hack around the ancient test machines not supporting sandboxing (see attachment 8495608 [details] [diff] [review]).
Summarizing from IRC: the actual test machines are Ubuntu 12.04, which does support sandboxing, but these tests run during the build job, which runs on CentOS 6, which doesn't.  But, as a less bad workaround, if the environment variable MOZ_DISABLE_GMP_SANDBOX could be set for these tests (checked at parent process startup, so it would have to be set in the testing infrastructure), then that would re-enable GMP loading.
yuck.
Attachment #8495608 - Attachment is obsolete: true
Attachment #8495644 - Flags: review?(ted)
Attachment #8494138 - Attachment is obsolete: false
Attachment #8494138 - Flags: review?(gps)
Duplicate of this bug: 1044667
Plugged a couple of memory leaks in the gtest. Carry r+
Attachment #8496659 - Flags: review+
Attachment #8494136 - Attachment is obsolete: true
Comment on attachment 8496657 [details] [diff] [review]
1072024-plug-leak.patch

Review of attachment 8496657 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/gmp/GMPAudioDecoderParent.cpp
@@ +164,5 @@
>  
> +  if (mShuttingDown) {
> +    return NS_OK;
> +  }
> +

I think you should set mShuttingDown=true before calling Terminated(), just in case Terminated() calls us back. Ditto elsewhere.
Attachment #8496657 - Flags: review?(cpearce) → review+
Comment on attachment 8494138 [details] [diff] [review]
Fix TestGMPCrossOrigin when run with |mach gtest|

Review of attachment 8494138 [details] [diff] [review]:
-----------------------------------------------------------------

Is dist/bin the XRE directory on OS X? I think Ted should get both of these reviews.
Attachment #8494138 - Flags: review?(gps) → review?(ted)
Comment on attachment 8495644 [details] [diff] [review]
Disable sandbox on linux gtests

Review of attachment 8495644 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/gtest/rungtests.py
@@ +64,5 @@
>          env["MOZ_RUN_GTEST"] = "1"
>          # Normally we run with GTest default output, override this to use the TBPL test format.
>          env["MOZ_TBPL_PARSER"] = "1"
> +
> +        if sys.platform.startswith("linux"):

Use mozinfo.isLinux here. Is there any way to check whether the current machine is new enough to run sandboxing? Is it just a kernel version check? I'd prefer that instead of hardcoding Linux here.

@@ +67,5 @@
> +
> +        if sys.platform.startswith("linux"):
> +          # XXX This is horrible. Our linux build boxes run CentOS 6, which is
> +          # too old to support sandboxing. Disable sandbox for gtests until
> +          # they can be upgraded.

File a bug on reverting this and put the bug number here instead of just XXX. Presumably the real solution will be "run gtests from the test package" (bug 992983).
Attachment #8495644 - Flags: review?(ted) → review+
Comment on attachment 8494138 [details] [diff] [review]
Fix TestGMPCrossOrigin when run with |mach gtest|

Review of attachment 8494138 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/mach_commands.py
@@ +659,5 @@
>          # https://code.google.com/p/googletest/wiki/AdvancedGuide#Running_Test_Programs:_Advanced_Options
>          gtest_env = {b'GTEST_FILTER': gtest_filter}
>  
> +        xre_path = os.path.join(self.topobjdir, "dist", "bin")
> +        gtest_env["MOZ_XRE_DIR"] = xre_path

n.b.: this might not work properly on Mac given the bundle layout changes that landed recently. You should check on that before landing.
Attachment #8494138 - Flags: review?(ted) → review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #14)
> Use mozinfo.isLinux here. Is there any way to check whether the current
> machine is new enough to run sandboxing? Is it just a kernel version check?
> I'd prefer that instead of hardcoding Linux here.

The approximation I've been using is ≥3.2 if Ubuntu, otherwise ≥3.5.  That won't hold up on non-x86 or with custom kernels that disable it (or Android), but it should be good enough for the build hosts.

Or we can test it directly, the same way the C++ code does:

```
import ctypes, errno

PR_SET_SECCOMP = 22
SECCOMP_MODE_FILTER = 2

def has_seccomp_bpf():
  ctypes.CDLL("libc.so.6", use_errno=True).prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, 0)
  return ctypes.get_errno() == errno.EFAULT
```
Comment on attachment 8496657 [details] [diff] [review]
1072024-plug-leak.patch

Review of attachment 8496657 [details] [diff] [review]:
-----------------------------------------------------------------

edwin: gerald is adding back a guard "if" statement here that you removed in Bug 1154513 to fix a leak. I think it makes sense to add it back because the guard in mIShuttingDown in GMP*::Shutdown() guards against Shutdown() being called multiple times, whereas the guard around mIsOpen in mIShuttingDown in GMP*::Shutdown() guards against Shutdown() being called after GMP*::ActorDestroy(). What do you think edwin? Does adding the mIsOpen guard back sounds reasonable?
Attachment #8496657 - Flags: feedback?(edwin)
(In reply to Chris Pearce (:cpearce) from comment #20)
> edwin: gerald is adding back a guard "if" statement here that you removed in
> Bug 1154513 to fix a leak. I think it makes sense to add it back because the
> guard in mIShuttingDown in GMP*::Shutdown() guards against Shutdown() being
> called multiple times, whereas the guard around mIsOpen in mIShuttingDown in
> GMP*::Shutdown() guards against Shutdown() being called after
> GMP*::ActorDestroy(). What do you think edwin? Does adding the mIsOpen guard
> back sounds reasonable?

Sounds okay. Can't remember anymore why I removed it.
You need to log in before you can comment on or make changes to this bug.