Bug 186880

Summary: [GStreamer] ASSERTION FAILED: end.isValid() in PlatformTimeRanges::add
Product: WebKit Reporter: Fujii Hironori <Hironori.Fujii>
Component: WebKitGTKAssignee: Fujii Hironori <Hironori.Fujii>
Status: RESOLVED FIXED    
Severity: Normal CC: aboya, bugs-noreply, calvaris, mcatanzaro
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=180253
Attachments:
Description Flags
Patch calvaris: review+

Description Fujii Hironori 2018-06-21 01:10:02 PDT 
[GStreamer] ASSERTION FAILED: end.isValid() in PlatformTimeRanges::add

Some LayoutTests fails.
See Bug 180253 and Bug 116977 Comment 2 for detail.

Callstack:

> Thread 1 (Thread 0x7f0b1787af80 (LWP 4274)):
> #0  0x00007f0b021173da in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:267
> #1  0x00007f0b115bebab in WebCore::PlatformTimeRanges::add (this=0x55e71ced1de0, start=..., end=...) at ../../Source/WebCore/platform/graphics/PlatformTimeRanges.cpp:139
> #2  0x00007f0b11edb993 in WebCore::MediaPlayerPrivateGStreamer::buffered (this=0x7f0a8b078000) at ../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1116
> #3  0x00007f0b115a6e5c in WebCore::MediaPlayer::buffered (this=0x7f0af0054a80) at ../../Source/WebCore/platform/graphics/MediaPlayer.cpp:818
> #4  0x00007f0b10e761c2 in WebCore::HTMLMediaElement::buffered (this=0x7f0a8a2007b0) at ../../Source/WebCore/html/HTMLMediaElement.cpp:5081
> #5  0x00007f0b0fbd3613 in WebCore::jsHTMLMediaElementBufferedGetter (state=..., thisObject=..., throwScope=...) at DerivedSources/WebCore/JSHTMLMediaElement.cpp:686
(...)
Comment 1 Fujii Hironori 2018-06-21 02:20:13 PDT 
Created attachment 343227 [details]
Patch
Comment 2 Fujii Hironori 2018-06-21 02:23:35 PDT 
This assertion failure is annoying. Looking into the code, I've
found strange code in MediaPlayerPrivateGStreamer::buffered. I'd
like to propose this patch.
Comment 3 Xabier Rodríguez Calvar 2018-06-21 02:30:30 PDT 
LGTM. Alicia, what do you think?
Comment 4 Alicia Boya García 2018-06-21 02:58:41 PDT 
Comment on attachment 343227 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1116
> +        if (loaded.isValid() && loaded)

LGTM. This will still create an empty [0, 0) range in some situations though. It maybe good to add `&& loaded > MediaTime::zeroTime()` to avoid that.
Comment 5 Alicia Boya García 2018-06-21 03:03:04 PDT 
(In reply to Alicia Boya García from comment #4)
> Comment on attachment 343227 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343227&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1116
> > +        if (loaded.isValid() && loaded)
> 
> LGTM. This will still create an empty [0, 0) range in some situations
> though. It maybe good to add `&& loaded > MediaTime::zeroTime()` to avoid
> that.

Nevermind, `loaded` already is true if different than zero.
Comment 6 Fujii Hironori 2018-06-21 03:10:20 PDT 
(In reply to Alicia Boya García from comment #5)
> Nevermind, `loaded` already is true if different than zero.

Agreed. Thank you for the review, Alicia.
Comment 7 Fujii Hironori 2018-06-21 03:15:23 PDT 
Committed r233033: <https://trac.webkit.org/changeset/233033>
Comment 8 Michael Catanzaro 2018-06-21 08:38:27 PDT 
Comment on attachment 343227 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        No new tests (No behavior change).

Nit: if there was no behavior change, then the execution of the program would not be visibly different. So there is definitely a behavior change: you fixed a crash!

Instead, I would have mentioned that this fixed existing tests.