Bug 63558 – Media element loads blocked by a resource load delegate or beforeload handler do not generate an error event

• Component: Media
• Status: REOPENED

[Eric Carlson]

If a resource load delegate blocks a media element from loading a file, no error event is generated.

[Eric Carlson]

Created attachment 99099 [details]
Proposed patch.

[Darin Adler]

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?

[Eric Carlson]

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

[Eric Carlson]

http://trac.webkit.org/changeset/90039

[Andrew Scherkus]

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?

[Eric Carlson]

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.

[Shadi]

Created attachment 115727 [details]
Check error generated by beforeload

[Shadi]

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

[Shadi]

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?

[WebKit Review Bot]

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.

[Shadi]

I am reopening the bug because the test is flaky. 

Eric, please let me know if you have suggestions for a fix.

[Eric Carlson]

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

[Eric Carlson]

Comment on attachment 115727 [details]
Check error generated by beforeload 

This attachment is not a diff.

[Shadi]

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.

[Eric Carlson]

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.

[Shadi]

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.

[Eric Carlson]

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.

[Shadi]

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.

[Eric Carlson]

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.

[Shadi]

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.

[Eric Carlson]

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

[Shadi]

Created attachment 120210 [details]
Patch

Added checkEndTest() that ends the test when both 'loaded' and 'loadmetadata' are called.

[Shadi]

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.

[Eric Carlson]

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.

[Shadi]

Created attachment 120216 [details]
Patch

Nit fixed.

[WebKit Review Bot]

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.

[Shadi]

Eric, 

I do not know why the review bot failed, or do I have to perform certain steps on my side?

[Eric Carlson]

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

[WebKit Review Bot]

Comment on attachment 120216 [details]
Patch

Clearing flags on attachment: 120216

Committed r103509: <http://trac.webkit.org/changeset/103509>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Shadi]

Reopening to attach new patch.

[Shadi]

Created attachment 121139 [details]
Patch

Test is not flaky, remove bug from expectation.

[Shadi]

Created attachment 121143 [details]
Patch

Test is not flaky, remove bug from expectation.

[Eric Carlson]

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