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)

defect

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox33 --- fixed
firefox34 --- fixed

People

(Reporter: gfritzsche, Assigned: jesup)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

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.
Whiteboard: [openh264-uplift]
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.
Priority: -- → P1
Target Milestone: --- → mozilla34
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.
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)
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)
Ok, I can add an IPDL command telling it to kill itself (immediate, or after a delay?)
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.
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.
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.
If there's a chrome interface we can hang a crash method off of then that's definitely the simplest route.
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?
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.
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.
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: nobody → rjesup
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+
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)
(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 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+
Blocks: 1046052
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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?
Attachment #8464314 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [openh264-uplift]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: