Closed
Bug 1072024
Opened 10 years ago
Closed 10 years ago
Test for cross-origin separation of GMPs
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: eflores, Assigned: eflores)
References
Details
Attachments
(4 files, 2 obsolete files)
1.08 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
1.38 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
8.75 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
6.14 KB,
patch
|
eflores
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Carry over r+ from bug 1039886
Attachment #8494136 -
Flags: review+
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8494138 -
Flags: review?(gps)
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
Wasted enough time here. Hack around ancient build infra for the meantime.
Attachment #8495608 -
Flags: review?(cpearce)
Updated•10 years ago
|
Attachment #8495608 -
Flags: review?(cpearce) → review+
Comment 5•10 years ago
|
||
Jed: FYI, we're also having to hack around the ancient test machines not supporting sandboxing (see attachment 8495608 [details] [diff] [review]).
Comment 6•10 years ago
|
||
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.
Assignee | ||
Comment 7•10 years ago
|
||
yuck.
Attachment #8495608 -
Attachment is obsolete: true
Attachment #8495644 -
Flags: review?(ted)
Assignee | ||
Updated•10 years ago
|
Attachment #8494138 -
Attachment is obsolete: false
Attachment #8494138 -
Flags: review?(gps)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8496657 -
Flags: review?(cpearce)
Assignee | ||
Comment 10•10 years ago
|
||
Plugged a couple of memory leaks in the gtest. Carry r+
Attachment #8496659 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8494136 -
Attachment is obsolete: true
Comment 11•10 years ago
|
||
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+
Assignee | ||
Comment 12•10 years ago
|
||
green: https://tbpl.mozilla.org/?tree=Try&rev=379447798500
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
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 15•10 years ago
|
||
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+
Comment 16•10 years ago
|
||
(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 ```
Assignee | ||
Comment 17•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/282fb5ca6310 https://hg.mozilla.org/integration/mozilla-inbound/rev/bdba7bcc32dc https://hg.mozilla.org/integration/mozilla-inbound/rev/7600cd87ae20 https://hg.mozilla.org/integration/mozilla-inbound/rev/85634997827e
Assignee | ||
Comment 18•10 years ago
|
||
+bustage fix https://hg.mozilla.org/integration/mozilla-inbound/rev/50d11b7914e9
Comment 19•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/282fb5ca6310 https://hg.mozilla.org/mozilla-central/rev/bdba7bcc32dc https://hg.mozilla.org/mozilla-central/rev/7600cd87ae20 https://hg.mozilla.org/mozilla-central/rev/85634997827e https://hg.mozilla.org/mozilla-central/rev/50d11b7914e9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 20•9 years ago
|
||
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)
Assignee | ||
Comment 21•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Attachment #8496657 -
Flags: feedback?(edwin)
You need to log in
before you can comment on or make changes to this bug.
Description
•