Bug 103258 – onload callback for <track> element attached to <video> does not fire

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

[Antoine Quint]

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.

[Antoine Quint]

<rdar://problem/12756200>

[Antoine Quint]

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.

[Antoine Quint]

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?

[Eric Carlson]

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

[Antoine Quint]

Created attachment 180343 [details]
Patch

[WebKit Review Bot]

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

[Antoine Quint]

Created attachment 180350 [details]
Patch

[Eric Carlson]

Comment on attachment 180350 [details]
Patch

Nice patch, great documentation - thanks!

[WebKit Review Bot]

Comment on attachment 180350 [details]
Patch

Clearing flags on attachment: 180350

Committed r138270: <http://trac.webkit.org/changeset/138270>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 105589

[Dean Jackson]

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.

[Dean Jackson]

Rollout was https://bugs.webkit.org/show_bug.cgi?id=105589

[Dean Jackson]

And it seems to have fixed the crashes.

[Dean Jackson]

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.

[Ryosuke Niwa]

Committed r138766: <http://trac.webkit.org/changeset/138766>