Bug 88423 – REGRESSION: Setting invalid media "src" does not cause "error" event

• Status: REOPENED
• Component: Media

[Jer Noble]

REGRESSION: Setting invalid media "src" does not cause "error" event

[Jer Noble]

Created attachment 146048 [details]
Patch

[Eric Carlson]

Comment on attachment 146048 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:85
> +SOFT_LINK_POINTER(QTKit, QTMovieOpenAsyncRequiredAttribute, NSString *)

This is will ASSERT if run on Leopard or older. Do we care?

> LayoutTests/http/tests/media/video-src-invalid-error.html:5
> +
> +

Nit: you have an extra blank line here.

> LayoutTests/http/tests/media/video-src-invalid-error.html:9
> +                failTest("stalled but should not" );

Nit: you have an extra space before the closing brace.

> LayoutTests/http/tests/media/video-src-invalid-error.html:24
> +                run("video.setAttribute('src', 'bogus.mp4')");

This will only work on ports that support MPEG-4. You should use findMediaFile() and not log the file name.

[Jer Noble]

(In reply to comment #2)
> (From update of attachment 146048 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146048&action=review
> 
> > Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:85
> > +SOFT_LINK_POINTER(QTKit, QTMovieOpenAsyncRequiredAttribute, NSString *)
> 
> This is will ASSERT if run on Leopard or older. Do we care?

Gah, that's annoying.  SOFT_LINK_POINTER_OPTIONAL then?

> > LayoutTests/http/tests/media/video-src-invalid-error.html:5
> > +
> > +
> 
> Nit: you have an extra blank line here.

Removed.

> > LayoutTests/http/tests/media/video-src-invalid-error.html:9
> > +                failTest("stalled but should not" );
> 
> Nit: you have an extra space before the closing brace.

Removed.

> > LayoutTests/http/tests/media/video-src-invalid-error.html:24
> > +                run("video.setAttribute('src', 'bogus.mp4')");
> 
> This will only work on ports that support MPEG-4. You should use findMediaFile() and not log the file name.

The whole point is to find an invalid URL.  So I could just leave off the .mp4 part entirely. ;)

[Eric Carlson]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 146048 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=146048&action=review
> > 
> > > Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:85
> > > +SOFT_LINK_POINTER(QTKit, QTMovieOpenAsyncRequiredAttribute, NSString *)
> > 
> > This is will ASSERT if run on Leopard or older. Do we care?
> 
> Gah, that's annoying.  SOFT_LINK_POINTER_OPTIONAL then?
> 
  Or just use the name like we do for QTMovieAllowPersistentCacheAttribute.

> 
> > > LayoutTests/http/tests/media/video-src-invalid-error.html:24
> > > +                run("video.setAttribute('src', 'bogus.mp4')");
> > 
> > This will only work on ports that support MPEG-4. You should use findMediaFile() and not log the file name.
> 
> The whole point is to find an invalid URL.  So I could just leave off the .mp4 part entirely. ;)
>
  That won't work if a platform checks the type internally (though I don't know if any do or not).

[Jer Noble]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 146048 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=146048&action=review
> > > 
> > > > Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:85
> > > > +SOFT_LINK_POINTER(QTKit, QTMovieOpenAsyncRequiredAttribute, NSString *)
> > > 
> > > This is will ASSERT if run on Leopard or older. Do we care?
> > 
> > Gah, that's annoying.  SOFT_LINK_POINTER_OPTIONAL then?
> > 
>   Or just use the name like we do for QTMovieAllowPersistentCacheAttribute.

Eh.  That seems to be what SOFT_LINK_POINTER_OPTIONAL is for.

> > > > LayoutTests/http/tests/media/video-src-invalid-error.html:24
> > > > +                run("video.setAttribute('src', 'bogus.mp4')");
> > > 
> > > This will only work on ports that support MPEG-4. You should use findMediaFile() and not log the file name.
> > 
> > The whole point is to find an invalid URL.  So I could just leave off the .mp4 part entirely. ;)
> >
>   That won't work if a platform checks the type internally (though I don't know if any do or not).

Because ports have to be able to load ".php" or ".aspx" URLs, I doubt any reject URLs with strange extensions without fetching them first.

[Jer Noble]

Created attachment 146062 [details]
Patch

[WebKit Review Bot]

Comment on attachment 146062 [details]
Patch

Attachment 146062 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12908366

New failing tests:
media/track/track-cue-nothing-to-render.html
media/track/track-cue-mutable-text.html
media/track/track-cue-mutable-fragment.html

[WebKit Review Bot]

Created attachment 146293 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Jer Noble]

Created attachment 146364 [details]
Patch

Fixes the failing text track tests.

[Jer Noble]

Committed r119940: <http://trac.webkit.org/changeset/119940>

(In reply to comment #10)
> Committed r119940: <http://trac.webkit.org/changeset/119940>

This caused ~10 media tests to start failing. See for example <http://build.webkit.org/results/Lion%20Release%20(Tests)/r119940%20(9220)/results.html>.

(In reply to comment #11)
> (In reply to comment #10)
> > Committed r119940: <http://trac.webkit.org/changeset/119940>
> 
> This caused ~10 media tests to start failing. See for example <http://build.webkit.org/results/Lion%20Release%20(Tests)/r119940%20(9220)/results.html>.

I reverted the change in <http://trac.webkit.org/r119978>.