Bug 79239 - [Chromium] video-referer.html fails
Summary: [Chromium] video-referer.html fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-22 07:28 PST by Eric Carlson
Modified: 2012-02-24 02:40 PST (History)
7 users (show)

See Also:


Attachments
Patch (3.48 KB, patch)
2012-02-22 17:49 PST, Dale Curtis
no flags Details | Formatted Diff | Diff
Patch (3.44 KB, patch)
2012-02-22 17:52 PST, Dale Curtis
no flags Details | Formatted Diff | Diff
Patch (3.14 KB, patch)
2012-02-23 13:33 PST, Dale Curtis
no flags Details | Formatted Diff | Diff
Patch (3.92 KB, patch)
2012-02-23 17:17 PST, Dale Curtis
no flags Details | Formatted Diff | Diff
Patch (3.86 KB, patch)
2012-02-23 17:36 PST, Dale Curtis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2012-02-22 07:28:28 PST
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.
Comment 1 Eric Carlson 2012-02-22 07:44:44 PST
Test skipped in r108503.
Comment 2 Dale Curtis 2012-02-22 17:49:40 PST
Created attachment 128351 [details]
Patch
Comment 3 Dale Curtis 2012-02-22 17:52:51 PST
Created attachment 128352 [details]
Patch
Comment 4 Alexey Proskuryakov 2012-02-22 18:07:53 PST
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 5 Dale Curtis 2012-02-22 18:16:19 PST
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 6 Alexey Proskuryakov 2012-02-22 19:31:22 PST
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.
Comment 7 Eric Carlson 2012-02-23 07:31:20 PST
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?
Comment 8 Dale Curtis 2012-02-23 13:33:12 PST
Created attachment 128545 [details]
Patch
Comment 9 Dale Curtis 2012-02-23 13:35:12 PST
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.
Comment 10 Dale Curtis 2012-02-23 13:37:02 PST
@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.
Comment 11 Alexey Proskuryakov 2012-02-23 13:57:11 PST
I still suspect that this is a legitimate chromium bug, not a test issue.
Comment 12 James Robinson 2012-02-23 13:58:50 PST
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.
Comment 13 Dale Curtis 2012-02-23 14:10:05 PST
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?
Comment 14 Eric Carlson 2012-02-23 14:33:37 PST
(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 15 Eric Carlson 2012-02-23 14:36:17 PST
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 16 Ami Fischman 2012-02-23 14:52:22 PST
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.
Comment 17 Alexey Proskuryakov 2012-02-23 15:06:38 PST
> @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!
Comment 18 Dale Curtis 2012-02-23 17:17:58 PST
Created attachment 128612 [details]
Patch
Comment 19 Dale Curtis 2012-02-23 17:18:11 PST
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.
Comment 20 Dale Curtis 2012-02-23 17:36:29 PST
Created attachment 128619 [details]
Patch
Comment 21 Eric Carlson 2012-02-23 20:39:54 PST
Comment on attachment 128619 [details]
Patch

Thanks!
Comment 22 WebKit Review Bot 2012-02-24 02:40:19 PST
Comment on attachment 128619 [details]
Patch

Clearing flags on attachment: 128619

Committed r108762: <http://trac.webkit.org/changeset/108762>
Comment 23 WebKit Review Bot 2012-02-24 02:40:25 PST
All reviewed patches have been landed.  Closing bug.