Bug 209827 – CRASH in MediaPlayerPrivateMediaSourceAVFObjC::addAudioRenderer(), uncaught ObjC exception

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

CRASH in MediaPlayerPrivateMediaSourceAVFObjC::addAudioRenderer(), uncaught ObjC exception

[Jer Noble]

<rdar://problem/61113080>

[Radar WebKit Bug Importer]

<rdar://problem/61120016>

[Jer Noble]

Created attachment 395081 [details]
Patch

[Darin Adler]

Comment on attachment 395081 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395081&action=review

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:903
> +                ASSERT_NOT_REACHED();

I don’t really like using assertions like this. As a project we intend to use assertions for things that must *never* happen, not for things that *normally* don’t happen.

The other cases in this patch seem to be truly "should never happen" cases, although I might be wrong about that.

[Jer Noble]

Comment on attachment 395081 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395081&action=review

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:903
>> +                ASSERT_NOT_REACHED();
> 
> I don’t really like using assertions like this. As a project we intend to use assertions for things that must *never* happen, not for things that *normally* don’t happen.
> 
> The other cases in this patch seem to be truly "should never happen" cases, although I might be wrong about that.

My goal here is to catch this case in layout tests; this scenario could easily cause layout test flakiness in a totally un-diagnosable way. If we upgrade a bot to a new OS and this ASSERTion starts getting hit and generates crashlogs, we can immediately trace the cause to a particular OS revision.

But maybe we can achieve this some other way, like through a WKTR or DRT only console log that would show up in the test .diff?

[Darin Adler]

(In reply to Jer Noble from comment #5)
> My goal here is to catch this case in layout tests

Got it!

> But maybe we can achieve this some other way, like through a WKTR or DRT only console log that would show up in the test .diff?

Yes, I think there is quite a bit of logging that shows up in regression test results. Let's check on that.

[Jer Noble]

Created attachment 395117 [details]
Patch for landing

[Jer Noble]

Created attachment 395191 [details]
Patch for landing

[EWS]

Committed r259363: <https://trac.webkit.org/changeset/259363>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395191 [details].