http/tests/media/video-referer.html started to fail after it was updated in r108387. The previous test succeeded if the media engine sent any referrer, now it checks for a specific referrer.
Test skipped in r108503.
Created attachment 128351 [details] Patch
Created attachment 128352 [details] Patch
Comment on attachment 128352 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128352&action=review > LayoutTests/ChangeLog:6 > + Fix races in video-referer test. Can you comment on what the races are?
Comment on attachment 128352 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128352&action=review >> LayoutTests/ChangeLog:6 >> + Fix races in video-referer test. > > Can you comment on what the races are? As far as I can tell, the event listeners are added too soon since they aren't protected by onload(). Which results in something going awry with their setup. I haven't dug too deep on the why, this is just a common problem I've run into with other layout tests.
Comment on attachment 128352 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128352&action=review I don't think that there should be any events dispatched before src attribute is set. This sounds more like an actual bug in chromium <video> implementation than a problem with the test. > LayoutTests/http/tests/media/video-referer.html:12 > + findMediaElement(); This should not be necessary, because video-test.js calls findMediaElement(), and the element is not inserted dynamically.
The failure reported, "CONSOLE MESSAGE: line 30: Uncaught TypeError: Cannot read property 'code' of null", suggests that the 'error' event is not coming from the video element. Have you checked to see what is generating the error and why?
Created attachment 128545 [details] Patch
Comment on attachment 128352 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128352&action=review >> LayoutTests/http/tests/media/video-referer.html:12 >> + findMediaElement(); > > This should not be necessary, because video-test.js calls findMediaElement(), and the element is not inserted dynamically. Correct, I added it while experimenting and left it in since most other tests call it explicitly. I've removed it in the latest patch set.
@Eric, the call is coming from the source element and it's a generic error event without any further details, this lead me to suspect the events weren't attaching to the video tag properly. Which is why I've moved these calls into the onload() handler.
I still suspect that this is a legitimate chromium bug, not a test issue.
Dale - can you reproduce the failure locally? If so, have you tried logging when the load event fires on failing runs vs passing runs? That should tell you if that's the problem.
Yes, I've repro'd the failure locally, w/o my patch the test fails 90% of the time, with my patch I haven't had a failure yet. @Alexey, can you explain why you think this is a Chromium issue and not a simple setup race? Is it not good practice to wait for onload() before starting any JavaScript?
(In reply to comment #13) > Yes, I've repro'd the failure locally, w/o my patch the test fails 90% of the time, with my patch I haven't had a failure yet. > This happens because the <source> element has no 'src' attribute, and step 2 of the section on using the source element says (http://dev.w3.org/html5/spec/media-elements.html#loading-the-media-resource): Process candidate: If candidate does not have a src attribute, or if its src attribute's value is the empty string, then end the synchronous section, and jump down to the failed step below. and the "failed" step says: Failed: Queue a task to fire a simple event named error at the candidate element > Is it not good practice to wait for onload() before starting any JavaScript? Sometimes yes, and sometimes no - I am not sure there is a general "good practice" here.
Comment on attachment 128545 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128545&action=review These changes do fix a problem with the test, but neither your ChangeLog nor your explanations in this bug fully explain why. It is much easier to review a proposed fix when a full explanation is presented. > LayoutTests/ChangeLog:6 > + Fix event setup race by moving setup into body onload() handler. Please say something more specific.
Comment on attachment 128545 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128545&action=review > LayoutTests/http/tests/media/video-referer.html:18 > frame.addEventListener('load', function () { Shouldn't this listener be added before frame.src is set in the previous line? > LayoutTests/http/tests/media/video-referer.html:25 > + waitForEventAndEnd('error', function () { Shouldn't the listeners be getting added right before the video.load() call above, if no errors are expected? > LayoutTests/http/tests/media/video-referer.html:26 > + consoleWrite('FAIL, got error when loading media.'); IWBN to use waitForEventAndFail() instead of wFEAEnd() for 'error'. (both this suggestion and the next will require you to edit the expected file). > LayoutTests/http/tests/media/video-referer.html:29 > + waitForEvent('canplay', function () { This would benefit from using waitForEventAndEnd.
> @Alexey, can you explain why you think this is a Chromium issue and not a simple setup race? Is it not good practice to wait for onload() before starting any JavaScript? So Eric explained above why it's correct to have an error event fired early in this specific case, so there is a race indeed. Thanks!
Created attachment 128612 [details] Patch
Comment on attachment 128545 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128545&action=review >> LayoutTests/ChangeLog:6 >> + Fix event setup race by moving setup into body onload() handler. > > Please say something more specific. Done. >> LayoutTests/http/tests/media/video-referer.html:18 >> frame.addEventListener('load', function () { > > Shouldn't this listener be added before frame.src is set in the previous line? Done. >> LayoutTests/http/tests/media/video-referer.html:25 >> + waitForEventAndEnd('error', function () { > > Shouldn't the listeners be getting added right before the video.load() call above, if no errors are expected? Done. >> LayoutTests/http/tests/media/video-referer.html:26 >> + consoleWrite('FAIL, got error when loading media.'); > > IWBN to use waitForEventAndFail() instead of wFEAEnd() for 'error'. > (both this suggestion and the next will require you to edit the expected file). Done. >> LayoutTests/http/tests/media/video-referer.html:29 >> + waitForEvent('canplay', function () { > > This would benefit from using waitForEventAndEnd. Done.
Created attachment 128619 [details] Patch
Comment on attachment 128619 [details] Patch Thanks!
Comment on attachment 128619 [details] Patch Clearing flags on attachment: 128619 Committed r108762: <http://trac.webkit.org/changeset/108762>
All reviewed patches have been landed. Closing bug.