Bug 103258 - onload callback for <track> element attached to <video> does not fire
Summary: onload callback for <track> element attached to <video> does not fire
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Antoine Quint
URL: http://w3c-test.org/html/tests/submis...
Keywords: InRadar
Depends on: 105589
Blocks: 105536
  Show dependency treegraph
 
Reported: 2012-11-26 08:21 PST by Antoine Quint
Modified: 2013-01-06 09:15 PST (History)
13 users (show)

See Also:


Attachments
Patch (24.93 KB, patch)
2012-12-20 08:04 PST, Antoine Quint
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (26.86 KB, patch)
2012-12-20 09:15 PST, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2012-11-26 08:21:43 PST
The callback registered via a <track> element's onload property does not fire under this scenario, taken from http://w3c-test.org/html/tests/submission/Opera/media/interfaces/TextTrackCue/align.html:

    var video = document.createElement('video');
    document.body.appendChild(video);
    var t = document.createElement('track');
    t.onload = this.step_func(function(){
      // onload callback code that doesn't fire
    });
    t.src = 'data:text/vtt,'+encodeURIComponent('WEBVTT\n\n00:00:00.000 --> 00:00:00.001\ntest\n\n'+
                                                '00:00:00.000 --> 00:00:00.001 align:start\ntest\n\n'+
                                                '00:00:00.000 --> 00:00:00.001 align:middle\ntest\n\n'+
                                                '00:00:00.000 --> 00:00:00.001 align:end\ntest');
    video.appendChild(t);

I suppose this might be related to the <video> not having an "src" attribute and not loading, but the HTML5 spec says that the only reason a <track> element shouldn't be loaded is if it wasn't attached to a media element, as per the discussion at https://www.w3.org/Bugs/Public/show_bug.cgi?id=20063.

This bug blocks testing of a lot of the Opera-submitted tests for HTML5 captions support as the onload callback is widely used across those tests.
Comment 1 Antoine Quint 2012-11-27 01:53:02 PST
<rdar://problem/12756200>
Comment 2 Antoine Quint 2012-12-07 01:19:12 PST
We're only starting the load of a TextTrack upon setting its "src" attribute (by calling HTMLTrackElement::scheduleLoad). In the case of most of the Opera tests, the "src" attribute is set before the mode or before the element is attached to a media element. Because of that, we never proceed with the actual loading of the resource.

The spec is clear however that changing the mode or attaching the element to a media element should attempt to start the resource load (section 4.8.10.12.3):

"When a text track corresponding to a track element experiences any of the following circumstances, the user agent must start the track processing model for that text track and its track element:

- The track element is created.
- The text track has its text track mode changed.
- The track element's parent element changes and the new parent is a media element."

The conditions checked in HTMLTrackElement::scheduleLoad seem legit, it should just be a matter of hooking up calls to that function in more places as mode and a media ancestor changes.
Comment 3 Antoine Quint 2012-12-07 01:37:53 PST
So it turns out we were already attempting the load of a text track whose mode changed in HTMLMediaElement::textTrackModeChanged. However, we were not attempting the load of the text track in HTMLMediaElement::didAddTrack. I reckon we need to add a call to `trackElement->scheduleLoad();` at the end of that method, like so:

    // Do not schedule the track loading until parsing finishes so we don't start before all tracks
    // in the markup have been added.
    if (!m_parsingInProgress) {
        scheduleLoad(TextTrackResource);
        trackElement->scheduleLoad();
    }

Eric, do you know if the order of these two calls matter?
Comment 4 Eric Carlson 2012-12-07 06:24:14 PST
(In reply to comment #3)
> So it turns out we were already attempting the load of a text track whose mode changed in HTMLMediaElement::textTrackModeChanged. However, we were not attempting the load of the text track in HTMLMediaElement::didAddTrack. I reckon we need to add a call to `trackElement->scheduleLoad();` at the end of that method, like so:
> 
>     // Do not schedule the track loading until parsing finishes so we don't start before all tracks
>     // in the markup have been added.
>     if (!m_parsingInProgress) {
>         scheduleLoad(TextTrackResource);
>         trackElement->scheduleLoad();
>     }
> 
> Eric, do you know if the order of these two calls matter?

I think it makes more sense for HTMLTrackElement to schedule the load itself, so I would do it in HTMLTrackElement::textTrackModeChanged before it calls parent->textTrackModeChanged. At the same time I would move the call to scheduleLoad() from HTMLMediaElement::didAddTrack to HTMLTrackElement::insertedInto.
Comment 5 Antoine Quint 2012-12-20 08:04:06 PST
Created attachment 180343 [details]
Patch
Comment 6 WebKit Review Bot 2012-12-20 08:55:01 PST
Comment on attachment 180343 [details]
Patch

Attachment 180343 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15452060

New failing tests:
fast/events/constructors/track-event-constructor.html
http/tests/security/text-track-crossorigin.html
Comment 7 Antoine Quint 2012-12-20 09:15:23 PST
Created attachment 180350 [details]
Patch
Comment 8 Eric Carlson 2012-12-20 09:58:37 PST
Comment on attachment 180350 [details]
Patch

Nice patch, great documentation - thanks!
Comment 9 WebKit Review Bot 2012-12-20 10:30:05 PST
Comment on attachment 180350 [details]
Patch

Clearing flags on attachment: 180350

Committed r138270: <http://trac.webkit.org/changeset/138270>
Comment 10 WebKit Review Bot 2012-12-20 10:30:10 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 WebKit Review Bot 2012-12-20 16:16:26 PST
Re-opened since this is blocked by bug 105589
Comment 12 Dean Jackson 2012-12-20 16:17:51 PST
I'm going to roll this out. Unfortunately I don't know if it is the cause, but many track tests have started to crash today.
Comment 13 Dean Jackson 2012-12-20 16:21:10 PST
Rollout was https://bugs.webkit.org/show_bug.cgi?id=105589
Comment 14 Dean Jackson 2012-12-20 17:18:27 PST
And it seems to have fixed the crashes.
Comment 15 Dean Jackson 2012-12-20 17:37:49 PST
Although we're still seeing some crashes:

http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r138324%20(5063)/media/track/tracklist-is-reachable-crash-log.txt

But rniwa points at https://bugs.webkit.org/show_bug.cgi?id=73865#c2 so we're not sure if this is unrelated.
Comment 16 Ryosuke Niwa 2013-01-03 16:34:41 PST
Committed r138766: <http://trac.webkit.org/changeset/138766>