803086 - Crash in mozilla::MediaPipelineTransmit::ProcessVideoChunk

Status: VERIFIED FIXED

(Core :: WebRTC, defect)

Description

[Mark Banner (:standard8)]

This bug was filed from the Socorro interface and is 
report bp-2b8c9fea-f335-40d6-a553-837d42121018 .
============================================================= 

Turning on Video streaming crashes in today's (18th Oct) nightly build.

STR:

1) Visit http://people.mozilla.com/~anarayanan/webrtc/pc_test.html with the relevant prefs set to true.
2) Click Start
3) Share the Camera

=> Nightly Crashes.

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dac5700acf8b&tochange=cb573b9307e5

Top of stack:

0 	XUL 	mozilla::MediaPipelineTransmit::ProcessVideoChunk 	ImageContainer.h:72
1 	XUL 	mozilla::MediaPipelineTransmit::PipelineListener::NotifyQueuedTrackChanges 	MediaPipeline.cpp:538
2 	XUL 	mozilla::MediaStreamGraphImpl::ExtractPendingInput 	MediaStreamGraph.cpp:612
3 	XUL 	mozilla::MediaStreamGraphImpl::RunThread 	MediaStreamGraph.cpp:1392

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Even though the testcase is public we should mark a security issue as such.

Comment 2

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

Known upstream issue with SRTP, already reported by Christoph.  There's a patch for upstream; I should apply the patch here; it didn't get included when we landed SRTP in m-c.

The read-past-end of this doesn't generate any actual leak of the data and was already noted in the source; so I don't feel this is a security bug.

Comment 3

[Mark Banner (:standard8)]

This still happens in today's nightly (2012-10-19):

https://crash-stats.mozilla.com/report/index/bp-034ec2e6-8cbb-4e38-a0fe-6d6a12121019

Built with: http://hg.mozilla.org/mozilla-central/rev/0ff60bfb3442 which includes the changeset landed on bug 790083.

Comment 5

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

Comment on attachment 672781 [details]
callstack

This callstack is another bug.  The original report is still extant.

See also https://crash-stats.mozilla.com/report/index/bp-5e4ab5ed-7a7f-4586-ac2b-4c65e2121019

Comment 6

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

Attachment: Protect against NULL image chunks in NotifyQueuedTrackChanges()

This is really wallpaper over the bug until we figure out what and why it's happening, or if NULL images are actually allowed here

Comment 7

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

Attachment: Protect against NULL image chunks in NotifyQueuedTrackChanges()

Comment 8

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

Comment on attachment 673243 [details] [diff] [review]
Protect against NULL image chunks in NotifyQueuedTrackChanges()

When I modified getUserMedia to fix video insertion, I took roc's advice to send NULL images until the first one is available (the alternative was to insert a black frame, because otherwise MediaStreams will block until video is up, and audio will queue in the meantime.)  However, apparently there was always an unstated requirement that NotifyQueuedTrackChanges() can give you null images.  This should be documented.

Not easily testable without modifying NotifyPull() for video to force NULL frames.

Only an issue since I landed the patch for bug 801843 which added NULL frames.

We'll want to uplift this with bug 801843

Comment 9

[Eric Rescorla (:ekr)]

Comment on attachment 673243 [details] [diff] [review]
Protect against NULL image chunks in NotifyQueuedTrackChanges()

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

lgtm

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/c20ed5e12b8d

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c20ed5e12b8d

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Randell Jesup [:jesup] from comment #6)
> This is really wallpaper over the bug until we figure out what and why it's
> happening, or if NULL images are actually allowed here

Randell, shouldn't we keep this bug open then?

Comment 13

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

That patch was obsoleted.  Fixed is correct.

Comment 14

[Jason Smith [:jsmith]]

Verified on 10/23 build by trying out the test case described in comment 0 a few times. Couldn't reproduce the crash.