1158627 - Crash in VCMJitterBuffer::GetFrame on Windows

Status: RESOLVED FIXED

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

Description

[Ethan Hugg [:ehugg]]

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.

Comment 1

[Ethan Hugg [:ehugg]]

Attachment: WebRTC return error if GetEmptyFrame returns null

Comment 3

[Ethan Hugg [:ehugg]]

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 4

[Ethan Hugg [:ehugg]]

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.

Comment 5

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

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.

Comment 6

[Ethan Hugg [:ehugg]]

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.

Comment 7

[Ethan Hugg [:ehugg]]

Attachment: WebRTC return error if GetEmptyFrame returns null

Comment 8

[Ethan Hugg [:ehugg]]

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

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/21566d28e474

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/21566d28e474

Comment 11

[Ethan Hugg [:ehugg]]

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.

Comment 12

[Ethan Hugg [:ehugg]]

Randell, I may need your input to get this uplifted in time.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8598215 [details] [diff] [review]
WebRTC return error if GetEmptyFrame returns null

[Triage Comment]
Should be in 38 beta 9.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/517b0df66a86

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/f1cd36f7e0e1

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f1cd36f7e0e1