| Summary: | Media element loads blocked by a resource load delegate or beforeload handler do not generate an error event | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||||||||||||
| Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||||||||||||
| Status: | REOPENED --- | ||||||||||||||||||
| Severity: | Normal | CC: | eric.carlson, jer.noble, joepeck, rakuco, shadi, webkit.review.bot, zoltan | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | All | ||||||||||||||||||
| OS: | All | ||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||
| Bug Blocks: | 75599 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Eric Carlson
2011-06-28 13:28:03 PDT
Created attachment 99099 [details]
Proposed patch.
Comment on attachment 99099 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=99099&action=review > Source/WebCore/html/HTMLMediaElement.cpp:659 > + ContentType contentType(""); Might want a why comment for this. > Source/WebCore/html/HTMLMediaElement.cpp:775 > + if (!m_player->load(url.string(), contentType)) > + mediaLoadingFailed(MediaPlayer::FormatError); No return here? (In reply to comment #2) > (From update of attachment 99099 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=99099&action=review > > > Source/WebCore/html/HTMLMediaElement.cpp:659 > > + ContentType contentType(""); > > Might want a why comment for this. > Will do. > > Source/WebCore/html/HTMLMediaElement.cpp:775 > > + if (!m_player->load(url.string(), contentType)) > > + mediaLoadingFailed(MediaPlayer::FormatError); > > No return here? No, this is at the end of HTMLMediaElement::loadResource and the remainder of the function updates internal state and updates the renderer, both of which we always want to do. Thanks for the review! Comment on attachment 99099 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=99099&action=review > LayoutTests/media/media-blocked-by-beforeload.html:105 > + function loaded(evt) Is this supposed to get executed? I'm not seeing it executed inside the expected results. It gets triggered occasionally on Chromium builds, but it looks like it's a race between window load event firing and loadedmetadata ending the test. Do you know what the expected result is supposed to be? Comment on attachment 99099 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=99099&action=review >> LayoutTests/media/media-blocked-by-beforeload.html:105 >> + function loaded(evt) > > Is this supposed to get executed? I'm not seeing it executed inside the expected results. > > It gets triggered occasionally on Chromium builds, but it looks like it's a race between window load event firing and loadedmetadata ending the test. > > Do you know what the expected result is supposed to be? The intention was for it to be executed, but as you note the output isn't in the results. I think you are correct that the problem is 'load' and 'loadedmetadata' can fire in any order. One fix would be to not log 'loadedmetadata' and to only end the test when BOTH 'loadedmetadata' and 'load' have fired. Created attachment 115727 [details]
Check error generated by beforeload
(Sorry, uploaded without comments.) I am trying to fix this test since it is flaky. However, if we just want to check if the error event is generated by media blocked in beforeload, can't a fix like the one I attach work? (why do we need to change the src/source tags) Comment on attachment 115727 [details]
Check error generated by beforeload
Eric,
If we just want to check if the error event is generated by media blocked in beforeload, do we need to change the src/source tags during the test?
Attachment 115727 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files
If any of these errors are false positives, please file a bug against check-webkit-style.
I am reopening the bug because the test is flaky. Eric, please let me know if you have suggestions for a fix. (In reply to comment #8) > (Sorry, uploaded without comments.) > > I am trying to fix this test since it is flaky. > > However, if we just want to check if the error event is generated by media blocked in beforeload, can't a fix like the one I attach work? (why do we need to change the src/source tags) This test is unacceptable because it does not test the feature. Please read HTMLMediaElement to understand what needs to be tested. (In reply to comment #11) > I am reopening the bug because the test is flaky. > > Eric, please let me know if you have suggestions for a fix. See my suggestion in comment #6. Comment on attachment 115727 [details]
Check error generated by beforeload
This attachment is not a diff.
Created attachment 120072 [details]
Changes based on Eric's suggestions.
I removed the log from 'loaded' event and end the test when both 'loadedmetadata' and 'loaded' are fired.
Comment on attachment 120072 [details] Changes based on Eric's suggestions. View in context: https://bugs.webkit.org/attachment.cgi?id=120072&action=review > LayoutTests/media/media-blocked-by-beforeload.html:35 > + if (loadedFired) > + checkEndTest(); Where is "checkEndTest"? What is it supposed to do? > LayoutTests/media/media-blocked-by-beforeload.html:112 > + if (loadedmetadataFired) > + endTest(); Isn't the theory that that we can't predict the order that 'load' and 'loadedmetadata' will fire? If so, the results will have "EVENT('loadedmetadata')" when 'loadedmetadata' first first, but not if 'load' first first. Comment on attachment 120072 [details] Changes based on Eric's suggestions. View in context: https://bugs.webkit.org/attachment.cgi?id=120072&action=review >> LayoutTests/media/media-blocked-by-beforeload.html:35 >> + checkEndTest(); > > Where is "checkEndTest"? What is it supposed to do? Typo, this should be endTest(); >> LayoutTests/media/media-blocked-by-beforeload.html:112 >> + endTest(); > > Isn't the theory that that we can't predict the order that 'load' and 'loadedmetadata' will fire? If so, the results will have "EVENT('loadedmetadata')" when 'loadedmetadata' first first, but not if 'load' first first. I think that the window 'loaded' event can't be predictable. In addition, the test results show that only the 'loaded' event happens at different points while the 'loadedmetadata' always happens at the end of the test after the good source is loaded. Comment on attachment 120072 [details] Changes based on Eric's suggestions. View in context: https://bugs.webkit.org/attachment.cgi?id=120072&action=review > LayoutTests/media/media-blocked-by-beforeload.html:22 > + var loadedmetadataFired = loadedFired = false Nit: WebKit style is to declare each variable separately. >>> LayoutTests/media/media-blocked-by-beforeload.html:35 >>> + checkEndTest(); >> >> Where is "checkEndTest"? What is it supposed to do? > > Typo, this should be endTest(); How did this test work? Did you run it? >>> LayoutTests/media/media-blocked-by-beforeload.html:112 >>> + endTest(); >> >> Isn't the theory that that we can't predict the order that 'load' and 'loadedmetadata' will fire? If so, the results will have "EVENT('loadedmetadata')" when 'loadedmetadata' first first, but not if 'load' first first. > > I think that the window 'loaded' event can't be predictable. In addition, the test results show that only the 'loaded' event happens at different points while the 'loadedmetadata' always happens at the end of the test after the good source is loaded. Exactly, that is why the existing test sometimes fails - and it is still a problem with your modifications. As I said above, if the 'load' event fires first the results will not have "EVENT('loadedmetadata')". The comment at the top of this test is "Test to ensure that a media file blocked by a beforeload handler generates an error and does not block the document's 'load' event.", ergo it *must* not finish until the 'load' event has fired. Comment on attachment 120072 [details] Changes based on Eric's suggestions. View in context: https://bugs.webkit.org/attachment.cgi?id=120072&action=review >>>> LayoutTests/media/media-blocked-by-beforeload.html:35 >>>> + checkEndTest(); >>> >>> Where is "checkEndTest"? What is it supposed to do? >> >> Typo, this should be endTest(); > > How did this test work? Did you run it? It worked because non-deterministic 'loaded' event happened after 'loadedmetadata' thus it didn't execute this code. >>>> LayoutTests/media/media-blocked-by-beforeload.html:112 >>>> + endTest(); >>> >>> Isn't the theory that that we can't predict the order that 'load' and 'loadedmetadata' will fire? If so, the results will have "EVENT('loadedmetadata')" when 'loadedmetadata' first first, but not if 'load' first first. >> >> I think that the window 'loaded' event can't be predictable. In addition, the test results show that only the 'loaded' event happens at different points while the 'loadedmetadata' always happens at the end of the test after the good source is loaded. > > Exactly, that is why the existing test sometimes fails - and it is still a problem with your modifications. > > As I said above, if the 'load' event fires first the results will not have "EVENT('loadedmetadata')". The comment at the top of this test is "Test to ensure that a media file blocked by a beforeload handler generates an error and does not block the document's 'load' event.", ergo it *must* not finish until the 'load' event has fired. Sorry, but I must be missing something here. If 'loaded' is fired, the test will *not* end unless 'loadedmetadata' has already fired (line 111), thus in both cases whether 'loaded' fires first or not, the "EVENT('loadedmetadata')" will appear in the results. Comment on attachment 120072 [details] Changes based on Eric's suggestions. View in context: https://bugs.webkit.org/attachment.cgi?id=120072&action=review >>>>> LayoutTests/media/media-blocked-by-beforeload.html:112 >>>>> + endTest(); >>>> >>>> Isn't the theory that that we can't predict the order that 'load' and 'loadedmetadata' will fire? If so, the results will have "EVENT('loadedmetadata')" when 'loadedmetadata' first first, but not if 'load' first first. >>> >>> I think that the window 'loaded' event can't be predictable. In addition, the test results show that only the 'loaded' event happens at different points while the 'loadedmetadata' always happens at the end of the test after the good source is loaded. >> >> Exactly, that is why the existing test sometimes fails - and it is still a problem with your modifications. >> >> As I said above, if the 'load' event fires first the results will not have "EVENT('loadedmetadata')". The comment at the top of this test is "Test to ensure that a media file blocked by a beforeload handler generates an error and does not block the document's 'load' event.", ergo it *must* not finish until the 'load' event has fired. > > Sorry, but I must be missing something here. > > If 'loaded' is fired, the test will *not* end unless 'loadedmetadata' has already fired (line 111), thus in both cases whether 'loaded' fires first or not, the "EVENT('loadedmetadata')" will appear in the results. No, it was me that missed something. The test does wait for both event, but it will produce different results unless they always fire in the same order. Comment on attachment 120072 [details] Changes based on Eric's suggestions. View in context: https://bugs.webkit.org/attachment.cgi?id=120072&action=review >>>>>> LayoutTests/media/media-blocked-by-beforeload.html:112 >>>>>> + endTest(); >>>>> >>>>> Isn't the theory that that we can't predict the order that 'load' and 'loadedmetadata' will fire? If so, the results will have "EVENT('loadedmetadata')" when 'loadedmetadata' first first, but not if 'load' first first. >>>> >>>> I think that the window 'loaded' event can't be predictable. In addition, the test results show that only the 'loaded' event happens at different points while the 'loadedmetadata' always happens at the end of the test after the good source is loaded. >>> >>> Exactly, that is why the existing test sometimes fails - and it is still a problem with your modifications. >>> >>> As I said above, if the 'load' event fires first the results will not have "EVENT('loadedmetadata')". The comment at the top of this test is "Test to ensure that a media file blocked by a beforeload handler generates an error and does not block the document's 'load' event.", ergo it *must* not finish until the 'load' event has fired. >> >> Sorry, but I must be missing something here. >> >> If 'loaded' is fired, the test will *not* end unless 'loadedmetadata' has already fired (line 111), thus in both cases whether 'loaded' fires first or not, the "EVENT('loadedmetadata')" will appear in the results. > > No, it was me that missed something. The test does wait for both event, but it will produce different results unless they always fire in the same order. I do not believe there will be different results because I removed any logging information from 'loaded' function and the 'loadmetadata' always happens at the end of the test. (In reply to comment #20) > > I do not believe there will be different results because I removed any logging information from 'loaded' function and the 'loadmetadata' always happens at the end of the test. The existing test succeeds when 'loadedmetadata' fires before 'load'. It succeeds every time on some bots (including Chromium Win - http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=media%2Fmedia-blocked-by-beforeload.html ), but I can get it to fail on OS X by changing the amount of logging it emits. Clearly the order that 'loaded' and 'loadmetadata' are fired and/or processed is timing dependent. Your patch will make the test fail if 'loaded' fires first, because loadmetadata() will try to call a function that does not exist. There are lots of ways to fix this. I would add a new function that is called from loaded() and loadedmetadata(), and have it call endTest() when both have called. Created attachment 120210 [details]
Patch
Added checkEndTest() that ends the test when both 'loaded' and 'loadmetadata' are called.
Comment on attachment 120210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120210&action=review > LayoutTests/media/media-blocked-by-beforeload.html:33 > consoleWrite(""); Even if 'loaded' happens before 'loadedmetadata', the logEvent here will not fail the test. Comment on attachment 120210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120210&action=review > LayoutTests/media/media-blocked-by-beforeload.html:114 > + function checkEndTest(){ Nit: WebKit style is to put the brace on a new line. Created attachment 120216 [details]
Patch
Nit fixed.
Attachment 120216 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource From git://git.webkit.org/WebKit + 5374bc3...5282799 master -> origin/master (forced update) Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 103439 = 5374bc345f9a23599193421e9fb1b650c3ca6486 r103408 = b4a2a08694f97ad1eb528a336c9bbae99f741ca3 r103409 = f475b49ac3603607c0d00a231e0299a4a22d4cc5 r103410 = 70ea0d9cab2d1b46c2f42d01e6c91cfcc10e1b1f r103411 = bce55f8034d5283f8ea17f60c42bd80d475808d9 r103412 = 499a989b1c8f9b58f9796a03d3201cf7fdcce09c r103413 = 5ee5b3403551c022bfac223c0cebdff100df1c21 r103414 = d5de0c179e7b0673fc8bb85fa5ef975ea4113314 r103417 = 69cec406c80ee71a5ccb1f5cf6c550ad48dbaaac r103418 = de9a48de0b5d6ee92def6407ea06096eb39e873b r103419 = 7a2b1e59459b762b502c7d4628542bf8f44003a7 r103421 = 77cb9c274f45e807ed8416fcf2f9241f9ac8e524 r103426 = 52c0cd967658499eb51faf388a89688aa51546c0 r103427 = af6e7265db605a1b858f2a7e5c5e50b15ed7b1f7 r103428 = d88a2b9824f9e0fbfd82146ffb822138b6b60947 r103429 = 94d46040310e6a3d3f95886d091c8b8cbc466b69 r103435 = 135cf8bd6ba2767bfa859a01eaacda6a10363326 r103438 = d64db2ae888dfab995b4cb18c6c21014d0060c5e r103439 = 5282799a1fb6729d1acbccc6f581466f9694b675 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Applying: Inform the scrolling coordinator when scrollbar layers come and go Using index info to reconstruct a base tree... <stdin>:474806: trailing whitespace. [Chromium] DatabaseTrackerChromium: iterating DatabaseSet races with Database disposal on worker thread <stdin>:474827: trailing whitespace. Nothing to test, just removing redundant code. Correct behavior tested by <stdin>:475346: trailing whitespace. warning: 3 lines add whitespace errors. Falling back to patching base and 3-way merge... warning: too many files (created: 167249 deleted: 3), skipping inexact rename detection Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Auto-merging Source/WebKit2/ChangeLog CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog Failed to merge in the changes. Patch failed at 0001 Inform the scrolling coordinator when scrollbar layers come and go When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 158. If any of these errors are false positives, please file a bug against check-webkit-style. Eric, I do not know why the review bot failed, or do I have to perform certain steps on my side? (In reply to comment #27) > > I do not know why the review bot failed, or do I have to perform certain steps on my side? There have been problems with one or more of the bots all day today. I will mark the patch again to see if it goes through. Comment on attachment 120216 [details] Patch Clearing flags on attachment: 120216 Committed r103509: <http://trac.webkit.org/changeset/103509> All reviewed patches have been landed. Closing bug. Reopening to attach new patch. Created attachment 121139 [details]
Patch
Test is not flaky, remove bug from expectation.
Created attachment 121143 [details]
Patch
Test is not flaky, remove bug from expectation.
(In reply to comment #33) > Created an attachment (id=121143) [details] > Patch > > Test is not flaky, remove bug from expectation. (In reply to comment #31) > Reopening to attach new patch. We usually open a new bug for follow-on work like this. It would be fine to relate the new bug to this one, but opening a new bug makes it makes it easier to follow. |