Bug 180253

Created attachment 328108 [details] Patch

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

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

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&)

std::unique_ptr<PlatformTimeRanges> MediaPlayerPrivateMediaSourceAVFObjC::seekable() const { return std::make_unique<PlatformTimeRanges>(minMediaTimeSeekable(), maxMediaTimeSeekable()); } Looks like either minMediaTimeSeekable() or maxMediaTimeSeekable() is invalid.

minMediaTimeSeekable() -> startTime() -> MediaTime::zeroTime(). So it must be... maxMediaTimeSeekable() -> durationMediaTime() -> m_mediaSourcePrivate->duration() -> m_client->duration() -> MediaSource::m_duration, which can be InvalidTime.

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!

Created attachment 337736 [details] Patch

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

Comment on attachment 337736 [details] Patch Clearing flags on attachment: 337736 Committed r230584: <https://trac.webkit.org/changeset/230584>

All reviewed patches have been landed. Closing bug.

<rdar://problem/39386772>

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.

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

(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.

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.

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.