Bug 174284 – media element handle adding source immediately before src.

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

[Jeremy Jones]

media element handle adding source immediately before src.

[Jeremy Jones]

rdar://problem/33115439

[Jeremy Jones]

Created attachment 314899 [details]
test only

[Jeremy Jones]

Created attachment 314900 [details]
Patch

[Build Bot]

Comment on attachment 314899 [details]
test only

Attachment 314899 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4074140

New failing tests:
media/video-source-before-src.html

[Build Bot]

Created attachment 314904 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[David Kilzer (:ddkilzer)]

Comment on attachment 314900 [details]
Patch

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

r=me (although maybe you want someone with more WebKit media experience to review)

> Source/WebCore/ChangeLog:9
> +        Test: media/video-source-before-src.html

The test crashes without the fix!  Yay!

> Source/WebCore/ChangeLog:11
> +        Adding a soruce causes selectMediaResource block to be enqueued.

Typo:  soruce = > source

> Source/WebCore/ChangeLog:13
> +        If dataLoadingPermitted prevents creating the m_player but sets the srcAttr.
> +        The enqeueud selectMediaResource will be in a bad state, with a srcAttr but no m_player.

These read best like a single sentence:

    If ... srcAttr, then
    the enqueued ....

Note that "enqueued" is also typoed.

> Source/WebCore/html/HTMLMediaElement.cpp:1328
> +                LOG(Media, "HTMLMediaElement::selectMediaResource(%p) - has srcAttr but m_player is not created", this);

Should this be a RELEASE_LOG_ERROR() or at least a LOG_ERROR()?

[Eric Carlson]

Comment on attachment 314900 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        Adding a soruce causes selectMediaResource block to be enqueued.
> 
> Typo:  soruce = > source

... causes *a* selectMediaResource block ...

> Source/WebCore/ChangeLog:15
> +        This fix prevents selectMediaResource when adding a source to match how it prevents player

This fix prevents selectMediaResource *from being called if data loading is not permitted* when adding a source *element* ...

[Jeremy Jones]

Comment on attachment 314900 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        Test: media/video-source-before-src.html
> 
> The test crashes without the fix!  Yay!

+1

>>> Source/WebCore/ChangeLog:11
>>> +        Adding a soruce causes selectMediaResource block to be enqueued.
>> 
>> Typo:  soruce = > source
> 
> ... causes *a* selectMediaResource block ...

Done. Done.

>> Source/WebCore/ChangeLog:13
>> +        The enqeueud selectMediaResource will be in a bad state, with a srcAttr but no m_player.
> 
> These read best like a single sentence:
> 
>     If ... srcAttr, then
>     the enqueued ....
> 
> Note that "enqueued" is also typoed.

Done.

>> Source/WebCore/ChangeLog:15
>> +        This fix prevents selectMediaResource when adding a source to match how it prevents player
> 
> This fix prevents selectMediaResource *from being called if data loading is not permitted* when adding a source *element* ...

Done.

>> Source/WebCore/html/HTMLMediaElement.cpp:1328
>> +                LOG(Media, "HTMLMediaElement::selectMediaResource(%p) - has srcAttr but m_player is not created", this);
> 
> Should this be a RELEASE_LOG_ERROR() or at least a LOG_ERROR()?

Done.

[Jeremy Jones]

Created attachment 315005 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 315005 [details]
Patch for landing.

Clearing flags on attachment: 315005

Committed r219305: <http://trac.webkit.org/changeset/219305>

[Jeremy Jones]

*** Bug 174489 has been marked as a duplicate of this bug. ***

[Jeremy Jones]

Created attachment 315399 [details]
Patch

[Build Bot]

Comment on attachment 315399 [details]
Patch

Attachment 315399 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4117130

New failing tests:
media/video-source-before-src.html

[Build Bot]

Created attachment 315402 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[David Kilzer (:ddkilzer)]

Comment on attachment 315399 [details]
Patch

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

r=me assuming iOS sim failure isn’t this.

> LayoutTests/media/video-source-before-src.html:12
> +var video

Nit: Add semi-colon.

> LayoutTests/media/video-source-before-src.html:15
> +    video = document.getElementsByTagName("video")[0]

Nit: semi-colon

> LayoutTests/media/video-source-before-src.html:19
> +    video.src = "src.mp4"

Nit on above e lines: semi-colon

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #15)
> Comment on attachment 315399 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315399&action=review
> 
> r=me assuming iOS sim failure isn’t this.

Oops, it is this test.

[Jeremy Jones]

(In reply to David Kilzer (:ddkilzer) from comment #16)
> (In reply to David Kilzer (:ddkilzer) from comment #15)
> > Comment on attachment 315399 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=315399&action=review
> > 
> > r=me assuming iOS sim failure isn’t this.
> 
> Oops, it is this test.

iOS needs different expectations.

[David Kilzer (:ddkilzer)]

Comment on attachment 315399 [details]
Patch

r=me if you fix the iOS expectations before landing.

[Jeremy Jones]

Created attachment 315501 [details]
Patch

[Jeremy Jones]

(In reply to David Kilzer (:ddkilzer) from comment #15)
> Comment on attachment 315399 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315399&action=review
> 
> r=me assuming iOS sim failure isn’t this.

It is. :-(

> 
> > LayoutTests/media/video-source-before-src.html:12
> > +var video
> 
> Nit: Add semi-colon.

Done.

> 
> > LayoutTests/media/video-source-before-src.html:15
> > +    video = document.getElementsByTagName("video")[0]
> 
> Nit: semi-colon

Done.

> 
> > LayoutTests/media/video-source-before-src.html:19
> > +    video.src = "src.mp4"
> 
> Nit on above e lines: semi-colon

Done.

[Build Bot]

Comment on attachment 315501 [details]
Patch

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

New failing tests:
media/video-source-before-src.html

[Build Bot]

Created attachment 315509 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

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

[Build Bot]

Comment on attachment 315501 [details]
Patch

Attachment 315501 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4122475

New failing tests:
media/video-source-before-src.html

[Build Bot]

Created attachment 315510 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

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

[Build Bot]

Comment on attachment 315501 [details]
Patch

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

New failing tests:
media/video-source-before-src.html

[Build Bot]

Created attachment 315511 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

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

[David Kilzer (:ddkilzer)]

Comment on attachment 315501 [details]
Patch

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

> LayoutTests/media/video-source-before-src-expected.txt:4
> +Append source element before setting src attribute.
> +Platforms that support setVideoPlaybackRequiresUserGesture will have the expected, empty, currentSrc. Platforms that don't should have the unexpected "src.mp4".
> +
> +EXPECTED (relativeURL(video.currentSrc) == ''), OBSERVED 'src.mp4' FAIL

Now the mac results are failing.  :(

[Jeremy Jones]

(In reply to David Kilzer (:ddkilzer) from comment #27)
> Comment on attachment 315501 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315501&action=review
> 
> > LayoutTests/media/video-source-before-src-expected.txt:4
> > +Append source element before setting src attribute.
> > +Platforms that support setVideoPlaybackRequiresUserGesture will have the expected, empty, currentSrc. Platforms that don't should have the unexpected "src.mp4".
> > +
> > +EXPECTED (relativeURL(video.currentSrc) == ''), OBSERVED 'src.mp4' FAIL
> 
> Now the mac results are failing.  :(

Missing a blank line in -expected. Fixed.

[Jeremy Jones]

Created attachment 315685 [details]
Patch for landing.

[Build Bot]

Comment on attachment 315685 [details]
Patch for landing.

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

New failing tests:
security/contentSecurityPolicy/video-with-data-url-allowed-by-media-src-star.html

[Build Bot]

Created attachment 315692 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

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

[Build Bot]

Comment on attachment 315685 [details]
Patch for landing.

Attachment 315685 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4136887

New failing tests:
http/tests/canvas/canvas-tainted-after-draw-image.html

[Build Bot]

Created attachment 315697 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Build Bot]

Comment on attachment 315685 [details]
Patch for landing.

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

New failing tests:
security/contentSecurityPolicy/video-with-data-url-allowed-by-media-src-star.html

[Build Bot]

Created attachment 315701 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

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

[WebKit Commit Bot]

Comment on attachment 315685 [details]
Patch for landing.

Rejecting attachment 315685 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 315685, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/4137477

[Jeremy Jones]

Created attachment 315716 [details]
Patch for landing test fix.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 315716 [details]:

http/tests/security/cross-origin-xsl-BLOCKED.html bug 51054 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 315716 [details]:

The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 315716 [details]
Patch for landing test fix.

Clearing flags on attachment: 315716

Committed r219579: <http://trac.webkit.org/changeset/219579>