174284 – media element handle adding source immediately before src.

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

Summary: media element handle adding source immediately before src.

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	Media (show other bugs)
Version:	WebKit Nightly Build
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Jeremy Jones

URL:
Keywords:	InRadar

Duplicates (1):	174489 (view as bug list)
Depends on:
Blocks:

Reported:	2017-07-07 16:34 PDT by Jeremy Jones
Modified:	2017-11-02 11:29 PDT (History)
CC List:	6 users (show)

See Also:

Attachments
test only (1.61 KB, patch) 2017-07-07 16:47 PDT, Jeremy Jones	buildbot: commit-queue-	Details \| Formatted Diff \| Diff
Patch (4.76 KB, patch) 2017-07-07 16:53 PDT, Jeremy Jones	ddkilzer: review+	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (1009.69 KB, application/zip) 2017-07-07 18:18 PDT, Build Bot	no flags	Details
Patch for landing. (4.83 KB, patch) 2017-07-10 12:21 PDT, Jeremy Jones	no flags	Details \| Formatted Diff \| Diff
Patch (2.83 KB, patch) 2017-07-13 21:15 PDT, Jeremy Jones	ddkilzer: review+ buildbot: commit-queue-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (7.18 MB, application/zip) 2017-07-13 22:40 PDT, Build Bot	no flags	Details
Patch (3.97 KB, patch) 2017-07-14 16:41 PDT, Jeremy Jones	buildbot: commit-queue-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews101 for mac-elcapitan (983.99 KB, application/zip) 2017-07-14 17:40 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.11 MB, application/zip) 2017-07-14 17:44 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews113 for mac-elcapitan (1.82 MB, application/zip) 2017-07-14 17:59 PDT, Build Bot	no flags	Details
Patch for landing. (3.97 KB, patch) 2017-07-17 11:43 PDT, Jeremy Jones	commit-queue: commit-queue-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews100 for mac-elcapitan (1.15 MB, application/zip) 2017-07-17 12:20 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (13.32 MB, application/zip) 2017-07-17 12:49 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews115 for mac-elcapitan (1.80 MB, application/zip) 2017-07-17 13:21 PDT, Build Bot	no flags	Details
Patch for landing test fix. (3.97 KB, patch) 2017-07-17 14:45 PDT, Jeremy Jones	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (10) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Jeremy Jones 2017-07-07 16:34:24 PDT

media element handle adding source immediately before src.

Comment 1 Jeremy Jones 2017-07-07 16:38:00 PDT

rdar://problem/33115439

Comment 2 Jeremy Jones 2017-07-07 16:47:07 PDT

Created attachment 314899 [details]
test only

Comment 3 Jeremy Jones 2017-07-07 16:53:26 PDT

Created attachment 314900 [details]
Patch

Comment 4 Build Bot 2017-07-07 18:18:28 PDT

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

Comment 5 Build Bot 2017-07-07 18:18:30 PDT

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

Comment 6 David Kilzer (:ddkilzer) 2017-07-08 11:29:25 PDT

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()?

Comment 7 Eric Carlson 2017-07-10 07:12:37 PDT

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

Comment 8 Jeremy Jones 2017-07-10 12:16:08 PDT

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.

Comment 9 Jeremy Jones 2017-07-10 12:21:07 PDT

Created attachment 315005 [details]
Patch for landing.

Comment 10 WebKit Commit Bot 2017-07-10 12:49:33 PDT

Comment on attachment 315005 [details]
Patch for landing.

Clearing flags on attachment: 315005

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

Comment 11 Jeremy Jones 2017-07-13 21:06:50 PDT

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

Comment 12 Jeremy Jones 2017-07-13 21:15:32 PDT

Created attachment 315399 [details]
Patch

Comment 13 Build Bot 2017-07-13 22:40:54 PDT

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

Comment 14 Build Bot 2017-07-13 22:40:55 PDT

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

Comment 15 David Kilzer (:ddkilzer) 2017-07-14 01:47:31 PDT

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

Comment 16 David Kilzer (:ddkilzer) 2017-07-14 01:48:34 PDT

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

Comment 17 Jeremy Jones 2017-07-14 15:08:21 PDT

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

Comment 18 David Kilzer (:ddkilzer) 2017-07-14 16:40:53 PDT

Comment on attachment 315399 [details]
Patch

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

Comment 19 Jeremy Jones 2017-07-14 16:41:32 PDT

Created attachment 315501 [details]
Patch

Comment 20 Jeremy Jones 2017-07-14 16:42:47 PDT

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

Comment 21 Build Bot 2017-07-14 17:40:15 PDT

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

Comment 22 Build Bot 2017-07-14 17:40:16 PDT

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

Comment 23 Build Bot 2017-07-14 17:44:31 PDT

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

Comment 24 Build Bot 2017-07-14 17:44:32 PDT

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

Comment 25 Build Bot 2017-07-14 17:59:14 PDT

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

Comment 26 Build Bot 2017-07-14 17:59:15 PDT

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

Comment 27 David Kilzer (:ddkilzer) 2017-07-14 21:34:25 PDT

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

Comment 28 Jeremy Jones 2017-07-17 11:42:37 PDT

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

Comment 29 Jeremy Jones 2017-07-17 11:43:16 PDT

Created attachment 315685 [details]
Patch for landing.

Comment 30 Build Bot 2017-07-17 12:20:14 PDT

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

Comment 31 Build Bot 2017-07-17 12:20:16 PDT

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

Comment 32 Build Bot 2017-07-17 12:49:03 PDT

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

Comment 33 Build Bot 2017-07-17 12:49:04 PDT

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

Comment 34 Build Bot 2017-07-17 13:21:47 PDT

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

Comment 35 Build Bot 2017-07-17 13:21:48 PDT

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

Comment 36 WebKit Commit Bot 2017-07-17 14:24:00 PDT

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

Comment 37 Jeremy Jones 2017-07-17 14:45:51 PDT

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

Comment 38 WebKit Commit Bot 2017-07-17 15:35:26 PDT

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

http/tests/security/cross-origin-xsl-BLOCKED.html bug 51054 (authors: abarth@webkit.org, jochen@chromium.org, and rniwa@webkit.org)
The commit-queue is continuing to process your patch.

Comment 39 WebKit Commit Bot 2017-07-17 15:35:28 PDT

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

The commit-queue is continuing to process your patch.

Comment 40 WebKit Commit Bot 2017-07-17 15:49:05 PDT

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

Clearing flags on attachment: 315716

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