Bug 79239 – [Chromium] video-referer.html fails

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

[Eric Carlson]

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.

[Eric Carlson]

Test skipped in r108503.

[Dale Curtis]

Created attachment 128351 [details]
Patch

[Dale Curtis]

Created attachment 128352 [details]
Patch

[Alexey Proskuryakov]

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?

[Dale Curtis]

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.

[Alexey Proskuryakov]

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.

[Eric Carlson]

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?

[Dale Curtis]

Created attachment 128545 [details]
Patch

[Dale Curtis]

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.

[Dale Curtis]

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

[Alexey Proskuryakov]

I still suspect that this is a legitimate chromium bug, not a test issue.

[James Robinson]

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.

[Dale Curtis]

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?

[Eric Carlson]

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

[Eric Carlson]

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.

[Ami Fischman]

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 Proskuryakov]

> @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!

[Dale Curtis]

Created attachment 128612 [details]
Patch

[Dale Curtis]

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.

[Dale Curtis]

Created attachment 128619 [details]
Patch

[Eric Carlson]

Comment on attachment 128619 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 128619 [details]
Patch

Clearing flags on attachment: 128619

Committed r108762: <http://trac.webkit.org/changeset/108762>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.