Bug 180253 – [MSE] Add isValid() assertions on PlatformTimeRanges::add() range ends

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Alicia Boya García]

Right now it's checked that start <= end but it's not checked that neither of them is undefined. 

When this happens, the bug can be hard to track down.

[Alicia Boya García]

Created attachment 328108 [details]
Patch

[EWS Watchlist]

Comment on attachment 328108 [details]
Patch

Attachment 328108 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5450880

New failing tests:
media/media-source/media-source-seek-detach-crash.html

[EWS Watchlist]

Created attachment 328125 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Alicia Boya García]

The assertion has failed in one of the MSE tests in macOS. Please check what made the code generate an invalid range.

ASSERTION FAILED: end.isValid()
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/graphics/PlatformTimeRanges.cpp(138) : void WebCore::PlatformTimeRanges::add(const WTF::MediaTime &, const WTF::MediaTime &)
1   0x103bca120 WTFCrash
2   0x10e41b853 WebCore::PlatformTimeRanges::add(WTF::MediaTime const&, WTF::MediaTime const&)
3   0x10e41b7b5 WebCore::PlatformTimeRanges::PlatformTimeRanges(WTF::MediaTime const&, WTF::MediaTime const&)
4   0x10e41ba75 WebCore::PlatformTimeRanges::PlatformTimeRanges(WTF::MediaTime const&, WTF::MediaTime const&)
5   0x10be2e743 WebCore::MediaPlayerPrivateMediaSourceAVFObjC::seekable() const
6   0x10e4011f2 WebCore::MediaPlayer::seekable()
7   0x10dc01e8f WebCore::HTMLMediaElement::seekable() const
8   0x10dbfa7f4 WebCore::HTMLMediaElement::seekTask()
9   0x10dc22337 WTF::Function<void ()>::CallableWrapper<std::__1::__bind<void (WebCore::HTMLMediaElement::*)(), WebCore::HTMLMediaElement*> >::call()
10  0x10bbf15ee WTF::Function<void ()>::operator()() const
11  0x10bdbf961 WebCore::GenericTaskQueue<WebCore::Timer>::enqueueTask(WTF::Function<void ()>&&)::'lambda'()::operator()() const
12  0x10bdbf6fc WTF::Function<void ()>::CallableWrapper<WebCore::GenericTaskQueue<WebCore::Timer>::enqueueTask(WTF::Function<void ()>&&)::'lambda'()>::call()
13  0x10bbf15ee WTF::Function<void ()>::operator()() const
14  0x10e27ebeb WebCore::TaskDispatcher<WebCore::Timer>::dispatchOneTask()
15  0x10e27eb2f WebCore::TaskDispatcher<WebCore::Timer>::sharedTimerFired()
16  0x10e27f361 WebCore::TaskDispatcher<WebCore::Timer>::sharedTimer()::$_1::operator()() const
17  0x10e27f31c WTF::Function<void ()>::CallableWrapper<WebCore::TaskDispatcher<WebCore::Timer>::sharedTimer()::$_1>::call()
18  0x10bbf15ee WTF::Function<void ()>::operator()() const
19  0x10bc65e9c WebCore::Timer::fired()
20  0x10e2b95a2 WebCore::ThreadTimers::sharedTimerFiredInternal()
21  0x10e2c2c91 WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0::operator()() const
22  0x10e2c2c3c WTF::Function<void ()>::CallableWrapper<WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0>::call()
23  0x10bbf15ee WTF::Function<void ()>::operator()() const
24  0x10e290529 WebCore::MainThreadSharedTimer::fired()
25  0x10e331eb9 WebCore::timerFired(__CFRunLoopTimer*, void*)
26  0x7fff8cfecae4 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__
27  0x7fff8cfec773 __CFRunLoopDoTimer
28  0x7fff8cfec2ca __CFRunLoopDoTimers
29  0x7fff8cfe37c1 __CFRunLoopRun
30  0x7fff8cfe2e28 CFRunLoopRunSpecific
31  0x101ed61de runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&)

[Jer Noble]

std::unique_ptr<PlatformTimeRanges> MediaPlayerPrivateMediaSourceAVFObjC::seekable() const
{
    return std::make_unique<PlatformTimeRanges>(minMediaTimeSeekable(), maxMediaTimeSeekable());
}

Looks like either minMediaTimeSeekable() or maxMediaTimeSeekable() is invalid.

[Jer Noble]

minMediaTimeSeekable() -> startTime() -> MediaTime::zeroTime(). So it must be...

maxMediaTimeSeekable() -> durationMediaTime() -> m_mediaSourcePrivate->duration() -> m_client->duration() -> MediaSource::m_duration, which can be InvalidTime.

[Michael Catanzaro]

Comment on attachment 328108 [details]
Patch

I don't like giving r- to good patches, but it can't go in if mac-debug is hitting the assertions!

[Alicia Boya García]

Created attachment 337736 [details]
Patch

[Xabier Rodríguez Calvar]

I think the patch is ok, but I'll leave Jer the final word.

[WebKit Commit Bot]

Comment on attachment 337736 [details]
Patch

Clearing flags on attachment: 337736

Committed r230584: <https://trac.webkit.org/changeset/230584>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39386772>

[Michael Catanzaro]

The broke the following tests:

compositing/geometry/clipped-video-controller.html
compositing/video/poster.html
fast/canvas/webgl/oes-texture-half-float-with-video.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-video-rgb565.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-video-rgba5551.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-video.html
fast/events/media-focus-in-standalone-media-document.html
fullscreen/video-controls-timeline.html
http/tests/appcache/video.html

If the patch did anything other than add assertions, I would roll it out, but since it just adds asserts I guess it's fine. I do not think the assertion should be enabled only for GTK, though. Reopening and updating expectations.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #13)
> The broke the following tests:

(GTK debug bot)

[Alicia Boya García]

(In reply to Michael Catanzaro from comment #13)
> I do not think the assertion should be enabled only for GTK, though.

It's not ideal indeed, but unfortunately it was being hit by Apple platform code, so until it is fixed there it can't be enabled for these ports.

[Michael Catanzaro]

Hm, I don't agree. I didn't expect you to land the assertions in this way. Currently we have this:

#if !PLATFORM(MAC) // https://bugs.webkit.org/show_bug.cgi?id=180253
    ASSERT(start.isValid());
    ASSERT(end.isValid());
#endif

The assertions are disabled only for Mac, but are being hit and causing crashes on GTK as well. So this doesn't make sense. We should either enable it on all platforms, or disable it for GTK as well.

Since the assertions are correct and useful for you, I don't want to suggest removing them. Instead, I suggest:

 (a) Remove the #if !PLATFORM(MAC).
 (b) Add debug crash expectations for the tests that break.
 (c) Hope we can find and resolve the underlying cause in the future.

This is not a regression, it's just surfacing a preexisting issue. And it's only debug builds, so won't hurt Safari users. We just need to update the TestExpectations to avoid a rollout.

In the meantime, I've marked all the tests in comment #13 against this bug. That was arguably lazy of me. If you prefer to track them in a different bug rather than here, you could split them out into a new bug report.

[Michael Catanzaro]

Um, I see the GTK crashes have actually been resolved in r233033.

I would still enable the assertion on Mac, but I guess that's difficult for us to do if the set of crashing tests is completely different, so I'll close this.