Closed
Bug 1044408
Opened 10 years ago
Closed 10 years ago
Enable crashing the fake GMP plugin from test code
Categories
(Core :: WebRTC: Audio/Video, defect, P1)
Core
WebRTC: Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: gfritzsche, Assigned: jesup)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file)
(deleted),
patch
|
ted
:
review+
cpearce
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Bug 1037125 added a fake GMP plugin and some automated tests.
To do proper automated test coverage of the full crash reporting code paths we should enable crashing the fake GMP plugin.
It should be easy to expose some Crash() function on the plugin, but i'm not sure off-hand where this could be exposed for test-code to trigger.
Updated•10 years ago
|
Blocks: WebRTC-OpenH264
Updated•10 years ago
|
Whiteboard: [openh264-uplift]
Comment 1•10 years ago
|
||
For the NPAPI test plugin we just added a scriptable method called Crash. Since GMP plugins don't have that same flexibility we'll have to figure something else out.
Updated•10 years ago
|
Priority: -- → P1
Target Milestone: --- → mozilla34
Reporter | ||
Comment 2•10 years ago
|
||
Side-note: This also at least complicates QA for crashreporting on OS X (e.g. bug 1043531).
Per bug 1012912 we can't trigger the crash reporter by just killing plugin-container.
Assignee | ||
Comment 3•10 years ago
|
||
Simple suggestion: provide a way to set a time-bomb in the GMP plugin to kill itself (environment var? Specific config value under SDP control?) which can be used when we need a plugin to crash.
ted: is there something we can control from tests that the (sandboxed) plugin will be able to see? Or do we need to use an SDP value? (which may not work as well for testing other types of GMP plugins like EME)
Flags: needinfo?(ted)
Flags: needinfo?(cpearce)
Comment 4•10 years ago
|
||
I don't really know what the plugin has access to, so I don't have a good sense of that. Could we make the plugin host code check a testing preference and ask the plugin to crash itself? That'd mean we'd have some test code that we'd have to ship in-product, but if it's behind a hidden pref and only the gmp-fake plugin honors it it doesn't seem so bad.
Flags: needinfo?(ted)
Assignee | ||
Comment 5•10 years ago
|
||
Ok, I can add an IPDL command telling it to kill itself (immediate, or after a delay?)
Comment 6•10 years ago
|
||
I assume the test code is in control of when the plugin gets instantiated, right? (By way of WebRTC or EME API.) Is that complicated by having multiple tests use the plugin? Will we reuse the same content process for each one? If we can guarantee that only the content process in use for the current test will be killed and it only gets instantiated on-demand for that test then killing it immediately should be fine.
Reporter | ||
Comment 7•10 years ago
|
||
An IPDL command would be fine.
From a mochitest-browser perspective both a triggering pref and a chrome-only scripting interface (gmpService.crash()) would work. I'd really like to avoid "set pref, then trigger plugin instantiation" scenarios though because of potential plugin-process re-use or that process still being around.
Reporter | ||
Comment 8•10 years ago
|
||
Note: the QA block for crashreporting on OS X should now be resolved by bug 1012912.
We still really want this for automated tests though.
Comment 9•10 years ago
|
||
If there's a chrome interface we can hang a crash method off of then that's definitely the simplest route.
Comment 10•10 years ago
|
||
Can we just communicate through the video channel? E.g., have a funny size incoming stream or draw from a video tag with in-band markings?
Comment 11•10 years ago
|
||
That's fine with me if that's workable. That would keep the testing code localized to the gmp-fake plugin, which would be nice.
Assignee | ||
Comment 12•10 years ago
|
||
content triggers are tricky and may bite you if you do things like feed screenshares through them - plus we don't have great "inject content" tools (at least not until we can source a MediaStream from a canvas). The best we'd have is screensize by changing size prefs and using fake:true - and that won't help for cpearce's uses probably.
I'd rather use either an ipdl command telling it to crash after <n> (where <n> can be 0), and control that via a pref, or (perhaps simpler) use SDP editing to set up a non-supported set of initial parameters (avoids special crash codes, but also likely wouldn't help cpearce).
Exposing this to scripting would be painful, unless we added it to something like PeerConnection - but again, this would be webrtc-specific. So we'd need to add it to a more global object (navigator?), and have that poke GMPService. Simplest for now is a hidden pref.
Assignee | ||
Comment 13•10 years ago
|
||
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8464314 [details] [diff] [review]
enable crashing all GMP plugins on a pref strobe
Ted - I'm using abort() here; perhaps we want a different crash? If so, easily substituted.
Crashes all loaded GMP plugins on strobing media.gmp.plugin.crash to true.
Attachment #8464314 -
Flags: review?(ted)
Attachment #8464314 -
Flags: review?(cpearce)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → rjesup
Comment 15•10 years ago
|
||
Comment on attachment 8464314 [details] [diff] [review]
enable crashing all GMP plugins on a pref strobe
Review of attachment 8464314 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/media/gmp/GMPChild.cpp
@@ +273,5 @@
>
> +bool
> +GMPChild::RecvCrashPluginNow()
> +{
> + abort();
You just want MOZ_CRASH here.
Attachment #8464314 -
Flags: review?(ted) → review+
Comment 16•10 years ago
|
||
I don't think it's a good idea to give users the ability to crash GMPs so easily.
Can't we just write another test GMP that crashes, and use that in the tests that you want to crash instead of the one that doesn't crash?
Flags: needinfo?(cpearce)
Comment 17•10 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #16)
> I don't think it's a good idea to give users the ability to crash GMPs so
> easily.
>
> Can't we just write another test GMP that crashes, and use that in the tests
> that you want to crash instead of the one that doesn't crash?
This would certainly be trivial to do.
Comment 18•10 years ago
|
||
Comment on attachment 8464314 [details] [diff] [review]
enable crashing all GMP plugins on a pref strobe
Review of attachment 8464314 [details] [diff] [review]:
-----------------------------------------------------------------
We'll need to fix this properly.
Attachment #8464314 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 19•10 years ago
|
||
status-firefox33:
--- → affected
status-firefox34:
--- → affected
Comment 20•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 21•10 years ago
|
||
Comment 22•10 years ago
|
||
QA Whiteboard: [qa-]
Reporter | ||
Comment 23•10 years ago
|
||
Comment on attachment 8464314 [details] [diff] [review]
enable crashing all GMP plugins on a pref strobe
Approval Request Comment
[Feature/regressing bug #]: GMP / OpenH264 integration.
[User impact if declined]: No good way for crash-testing for QA and manual dev tests.
[Describe test coverage new/current, TBPL]: Fine on m-c, has QA and dev check coverage.
[Risks and why]: Low-risk, this just triggers a crash in the GMP child on a hidden pref.
[String/UUID change made/needed]: none.
Attachment #8464314 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Updated•10 years ago
|
Attachment #8464314 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Reporter | ||
Comment 24•10 years ago
|
||
Reporter | ||
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Whiteboard: [openh264-uplift]
You need to log in
before you can comment on or make changes to this bug.
Description
•