Closed Bug 1158627 Opened 5 years ago Closed 5 years ago

Crash in VCMJitterBuffer::GetFrame on Windows

Categories

(Core :: WebRTC: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox38 --- fixed
firefox38.0.5 --- fixed
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: ehugg, Assigned: ehugg)

References

Details

Attachments

(1 file, 1 obsolete file)

Found by the Spark team when screensharing from Windows on FF38:

https://crash-stats.mozilla.com/report/index/53123e67-d7ec-4a64-ad32-8b6c22150425
https://crash-stats.mozilla.com/report/index/5f9390fe-0ae5-4f14-9c0f-68ac32150425
https://crash-stats.mozilla.com/report/index/4c86d802-ffe6-4df4-8ee2-5862a2150425

Looking at the code I noticed that here:
https://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_coding/main/source/jitter_buffer.cc#663
assert(*frame);

used to look more like:
if (!*frame) 
  return kGeneralError;

before the webrtc40 update, so I think this crash is new with that update.
Assignee: nobody → ethanhugg
Status: NEW → ASSIGNED
Duplicate of this bug: 1143561
I am unable to replicate this with my Win8 machine, but here is a try run on top of FF38 of this band-aid patch that should keep it from crashing.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58237c06dc16
Comment on attachment 8597741 [details] [diff] [review]
WebRTC return error if GetEmptyFrame returns null


I just got confirmation from Dave Brown on the Spark team that this patch keeps his FF38 from crashing on Windows, so I'm putting it up for review.

This is similar to what was in v3.43 of webrtc which was pushed in bug 932112

I see no reason to keep in this possible null deref so I'd like to request uplift to 38 after this lands.
Attachment #8597741 - Flags: review?(rjesup)
Comment on attachment 8597741 [details] [diff] [review]
WebRTC return error if GetEmptyFrame returns null

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

And I think this is a safe fix to push to Beta/38 - restoring a simple null-check that was in 37.  If preferred, we can leave off the debug I asked for in the uplift.

::: media/webrtc/trunk/webrtc/modules/video_coding/main/source/jitter_buffer.cc
@@ +660,5 @@
>      LOG(LS_WARNING) << "Unable to get empty frame; Recycling.";
>      bool found_key_frame = RecycleFramesUntilKeyFrame();
>      *frame = GetEmptyFrame();
> +    if (!*frame) {
> +      return kGeneralError;

Please log strongly - this really shouldn't be happening; this implies something wrong in the depacketization and/or packetization and/or error recovery.
Attachment #8597741 - Flags: review?(rjesup) → review+
Yes, it shouldn't be happening.  I think there's a larger problem here with 38/Win.  We're seeing the video freeze when the stats say packets are still be received.  It's something that doesn't happen on 37/Win.  We are still investigating. 

Also interesting is that I am unable to work with win debug builds - bug 1158913.
Attachment #8597741 - Attachment is obsolete: true
Comment on attachment 8598215 [details] [diff] [review]
WebRTC return error if GetEmptyFrame returns null


Added log message.  Bringing forward r+ from Jesup.

New try run with unittests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed7d8ee5f910
Attachment #8598215 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/21566d28e474
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment on attachment 8598215 [details] [diff] [review]
WebRTC return error if GetEmptyFrame returns null

Approval Request Comment
[Feature/regressing bug #]: 
I believe this appears with the webrtc40 update - bug 1109248.  The code I added back I found in the earlier webrtc3.43 update - bug 932112.

[User impact if declined]:  
Under certain conditions the jitter buffer will back up and GetEmptyFrame will return NULL.  Without this patch we will crash a few lines after the end of this patch on the line (*frame)->Reset();

[Describe test coverage new/current, TreeHerder]:
On M-C and tested in a scenario that was crashing regularly (Win7 using Spark).  Not really covered by our normal unit tests.

[Risks and why]: 
Low risk, instead of crashing we end up dropping some frames when we return the kGeneralError.

[String/UUID change made/needed]:
none.
Attachment #8598215 - Flags: approval-mozilla-beta?
Attachment #8598215 - Flags: approval-mozilla-aurora?
Randell, I may need your input to get this uplifted in time.
Flags: needinfo?(rjesup)
Comment on attachment 8598215 [details] [diff] [review]
WebRTC return error if GetEmptyFrame returns null

[Triage Comment]
Should be in 38 beta 9.
Attachment #8598215 - Flags: approval-mozilla-release+
Attachment #8598215 - Flags: approval-mozilla-beta?
Attachment #8598215 - Flags: approval-mozilla-aurora?
Attachment #8598215 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.