Bug 208322 – Improve some media code

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

[Darin Adler]

Fir issues noticed in audit of media code

[Darin Adler]

Created attachment 392068 [details]
Patch

[Darin Adler]

Comment on attachment 392068 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392068&action=review

> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:77
> +void MediaControlsHost::ref()

The alternative to having MediaControlsHost having its lifetime tied completely to the HTMLMediaElement and forwarding ref/deref calls would be to use a WeakPtr and then add null checks to every function. I think this is a better solution, but I suppose there is some risk of a reference cycle if I don’t understand the ownership relationships correctly. Like what can hold a reference to the media controls host object and is there a chance of a cycle.

> Source/WebCore/loader/TextTrackLoader.h:66
>      Vector<Ref<VTTCue>> getNewCues();
> -    void getNewRegions(Vector<RefPtr<VTTRegion>>& outputRegions);
> +    Vector<Ref<VTTRegion>> getNewRegions();
>      Vector<String> getNewStyleSheets();

We could also consider renaming these three functions to take or takeNew instead of getNew, since they transfer ownership to the caller.

[Alex Christensen]

Comment on attachment 392068 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392068&action=review

r=me except one problem.

> Source/WebCore/html/track/WebVTTParser.cpp:342
> +                m_regionList.remove(i);

This is a O(n^2) algorithm that mutates the collection while iterating, so it would miss regions right after the removed region.  removeAllMatching would be faster and more correct.

[Eric Carlson]

Comment on attachment 392068 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392068&action=review

>> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:77
>> +void MediaControlsHost::ref()
> 
> The alternative to having MediaControlsHost having its lifetime tied completely to the HTMLMediaElement and forwarding ref/deref calls would be to use a WeakPtr and then add null checks to every function. I think this is a better solution, but I suppose there is some risk of a reference cycle if I don’t understand the ownership relationships correctly. Like what can hold a reference to the media controls host object and is there a chance of a cycle.

HTMLMediaElement creates MediaControlsHost and exposes it to the JavaScript that implements controls in the shadow DOM. The controls JS puts the MediaControlsHost into a global variable. 

HTMLMediaElement owns the JS, and the JS now refs HTMLMediaElement, so doesn't this create a retain cycle?

[Darin Adler]

Comment on attachment 392068 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392068&action=review

>> Source/WebCore/html/track/WebVTTParser.cpp:342
>> +                m_regionList.remove(i);
> 
> This is a O(n^2) algorithm that mutates the collection while iterating, so it would miss regions right after the removed region.  removeAllMatching would be faster and more correct.

Agreed that it’s O(n^2), but note that this breaks out of the loop when it finds a region that matches; so there are no "missed regions" because this removes only the first. All I did here was get rid of the second unnecessary loop over the vector inside the removeFirst function. While it’s OK for someone to change this to removeAllMatching I don’t think that change is necessary as part of this patch. I also don’t think removeAllMatching would be significantly faster.

[Darin Adler]

Comment on attachment 392068 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392068&action=review

>>> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:77
>>> +void MediaControlsHost::ref()
>> 
>> The alternative to having MediaControlsHost having its lifetime tied completely to the HTMLMediaElement and forwarding ref/deref calls would be to use a WeakPtr and then add null checks to every function. I think this is a better solution, but I suppose there is some risk of a reference cycle if I don’t understand the ownership relationships correctly. Like what can hold a reference to the media controls host object and is there a chance of a cycle.
> 
> HTMLMediaElement creates MediaControlsHost and exposes it to the JavaScript that implements controls in the shadow DOM. The controls JS puts the MediaControlsHost into a global variable. 
> 
> HTMLMediaElement owns the JS, and the JS now refs HTMLMediaElement, so doesn't this create a retain cycle?

Sounds like it probably does create a retain cycle. I need to find some other solution, because the old code has a potentially dangling pointer that I can’t just leave as-is. I suppose I could use a WeakPtr and add a null check to every use of m_mediaElement. Any other ideas about how to handle this back-pointer?

[Alex Christensen]

Comment on attachment 392068 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392068&action=review

>>>> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:77
>>>> +void MediaControlsHost::ref()
>>> 
>>> The alternative to having MediaControlsHost having its lifetime tied completely to the HTMLMediaElement and forwarding ref/deref calls would be to use a WeakPtr and then add null checks to every function. I think this is a better solution, but I suppose there is some risk of a reference cycle if I don’t understand the ownership relationships correctly. Like what can hold a reference to the media controls host object and is there a chance of a cycle.
>> 
>> HTMLMediaElement creates MediaControlsHost and exposes it to the JavaScript that implements controls in the shadow DOM. The controls JS puts the MediaControlsHost into a global variable. 
>> 
>> HTMLMediaElement owns the JS, and the JS now refs HTMLMediaElement, so doesn't this create a retain cycle?
> 
> Sounds like it probably does create a retain cycle. I need to find some other solution, because the old code has a potentially dangling pointer that I can’t just leave as-is. I suppose I could use a WeakPtr and add a null check to every use of m_mediaElement. Any other ideas about how to handle this back-pointer?

Sounds like this needs a WeakPtr and a bunch of null checks then.

>>> Source/WebCore/html/track/WebVTTParser.cpp:342
>>> +                m_regionList.remove(i);
>> 
>> This is a O(n^2) algorithm that mutates the collection while iterating, so it would miss regions right after the removed region.  removeAllMatching would be faster and more correct.
> 
> Agreed that it’s O(n^2), but note that this breaks out of the loop when it finds a region that matches; so there are no "missed regions" because this removes only the first. All I did here was get rid of the second unnecessary loop over the vector inside the removeFirst function. While it’s OK for someone to change this to removeAllMatching I don’t think that change is necessary as part of this patch. I also don’t think removeAllMatching would be significantly faster.

Ah, I didn't read all the way to the break. This is fine.

[Darin Adler]

Created attachment 392673 [details]
Patch

[Darin Adler]

OK, new version up for review uses WeakPtr.

[Darin Adler]

Committed r257997: <https://trac.webkit.org/changeset/257997>

[Radar WebKit Bug Importer]

<rdar://problem/60151665>