Bug 180253 - [MSE] Add isValid() assertions on PlatformTimeRanges::add() range ends
Summary: [MSE] Add isValid() assertions on PlatformTimeRanges::add() range ends
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alicia Boya García
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-01 09:38 PST by Alicia Boya García
Modified: 2018-06-27 13:26 PDT (History)
8 users (show)

See Also:


Attachments
Patch (1.55 KB, patch)
2017-12-01 09:44 PST, Alicia Boya García
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-elcapitan (3.51 MB, application/zip)
2017-12-01 11:20 PST, Build Bot
no flags Details
Patch (1.72 KB, patch)
2018-04-11 14:53 PDT, Alicia Boya García
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alicia Boya García 2017-12-01 09:38:43 PST
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.
Comment 1 Alicia Boya García 2017-12-01 09:44:11 PST
Created attachment 328108 [details]
Patch
Comment 2 Build Bot 2017-12-01 11:20:29 PST
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
Comment 3 Build Bot 2017-12-01 11:20:30 PST
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
Comment 4 Alicia Boya García 2017-12-01 12:44:43 PST
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&)
Comment 5 Jer Noble 2017-12-01 13:01:54 PST
std::unique_ptr<PlatformTimeRanges> MediaPlayerPrivateMediaSourceAVFObjC::seekable() const
{
    return std::make_unique<PlatformTimeRanges>(minMediaTimeSeekable(), maxMediaTimeSeekable());
}

Looks like either minMediaTimeSeekable() or maxMediaTimeSeekable() is invalid.
Comment 6 Jer Noble 2017-12-01 13:04:27 PST
minMediaTimeSeekable() -> startTime() -> MediaTime::zeroTime(). So it must be...

maxMediaTimeSeekable() -> durationMediaTime() -> m_mediaSourcePrivate->duration() -> m_client->duration() -> MediaSource::m_duration, which can be InvalidTime.
Comment 7 Michael Catanzaro 2018-04-09 20:50:30 PDT
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!
Comment 8 Alicia Boya García 2018-04-11 14:53:27 PDT
Created attachment 337736 [details]
Patch
Comment 9 Xabier Rodríguez Calvar 2018-04-12 03:32:34 PDT
I think the patch is ok, but I'll leave Jer the final word.
Comment 10 WebKit Commit Bot 2018-04-12 11:02:04 PDT
Comment on attachment 337736 [details]
Patch

Clearing flags on attachment: 337736

Committed r230584: <https://trac.webkit.org/changeset/230584>
Comment 11 WebKit Commit Bot 2018-04-12 11:02:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2018-04-12 11:03:39 PDT
<rdar://problem/39386772>
Comment 13 Michael Catanzaro 2018-06-03 16:09:37 PDT
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.
Comment 14 Michael Catanzaro 2018-06-03 16:09:46 PDT
(In reply to Michael Catanzaro from comment #13)
> The broke the following tests:

(GTK debug bot)
Comment 15 Alicia Boya García 2018-06-21 03:01:46 PDT
(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.
Comment 16 Michael Catanzaro 2018-06-21 08:30:41 PDT
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.
Comment 17 Michael Catanzaro 2018-06-21 08:36:17 PDT
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.