Bug 174284

Summary:

media element handle adding source immediately before src.

Product:

WebKit

Reporter:

Jeremy Jones <jeremyj-wk>

Component:

Media

Assignee:

Jeremy Jones <jeremyj-wk>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, commit-queue, ddkilzer, eric.carlson, rniwa, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
test only	buildbot: commit-queue-
Patch	ddkilzer: review+
Archive of layout-test-results from ews122 for ios-simulator-wk2	none
Patch for landing.	none
Patch	ddkilzer: review+, buildbot: commit-queue-
Archive of layout-test-results from ews123 for ios-simulator-wk2	none
Patch	buildbot: commit-queue-
Archive of layout-test-results from ews101 for mac-elcapitan	none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2	none
Archive of layout-test-results from ews113 for mac-elcapitan	none
Patch for landing.	commit-queue: commit-queue-
Archive of layout-test-results from ews100 for mac-elcapitan	none
Archive of layout-test-results from ews123 for ios-simulator-wk2	none
Archive of layout-test-results from ews115 for mac-elcapitan	none
Patch for landing test fix.	none

Jeremy Jones

Reported 2017-07-07 16:34:24 PDT

media element handle adding source immediately before src.

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 attachment proposed patch, testcase, etc.

Jeremy Jones

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

rdar://problem/33115439

Jeremy Jones

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

Created attachment 314899 [details] test only

Jeremy Jones

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

Created attachment 314900 [details] Patch

Build Bot

Comment 4 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

Build Bot

Comment 5 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

David Kilzer (:ddkilzer)

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

Eric Carlson

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

Jeremy Jones

Comment 8 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.

Jeremy Jones

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

Created attachment 315005 [details] Patch for landing.

WebKit Commit Bot

Comment 10 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>

Jeremy Jones

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

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

Jeremy Jones

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

Created attachment 315399 [details] Patch

Build Bot

Comment 13 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

Build Bot

Comment 14 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

David Kilzer (:ddkilzer)

Comment 15 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

David Kilzer (:ddkilzer)

Comment 16 2017-07-14 01:48:34 PDT

Jeremy Jones

Comment 17 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.

David Kilzer (:ddkilzer)

Comment 18 2017-07-14 16:40:53 PDT

Comment on attachment 315399 [details] Patch r=me if you fix the iOS expectations before landing.

Jeremy Jones

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

Created attachment 315501 [details] Patch

Jeremy Jones

Comment 20 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.

Build Bot

Comment 21 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

Build Bot

Comment 22 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

Build Bot

Comment 23 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

Build Bot

Comment 24 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

Build Bot

Comment 25 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

Build Bot

Comment 26 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

David Kilzer (:ddkilzer)

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

Jeremy Jones

Comment 28 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.

Jeremy Jones

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

Created attachment 315685 [details] Patch for landing.

Build Bot

Comment 30 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

Build Bot

Comment 31 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

Build Bot

Comment 32 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

Build Bot

Comment 33 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

Build Bot

Comment 34 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

Build Bot

Comment 35 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

WebKit Commit Bot

Comment 36 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

Jeremy Jones

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

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

WebKit Commit Bot

Comment 38 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.

WebKit Commit Bot

Comment 39 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.

WebKit Commit Bot

Comment 40 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>

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