Bug 143954 – Refactor WebKit1 specific threading code out of WebVideoFullscreen code that is shared with WebKit2.

• Component: Media
• Status: RESOLVED FIXED

[Jeremy Jones]

Refactor WebKit1 specific threading code out of WebVideoFullscreen code that is shared with WebKit2.

[Jeremy Jones]

Created attachment 251167 [details]
Patch

[Jeremy Jones]

Created attachment 253248 [details]
Patch

[Jeremy Jones]

*** Bug 145086 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

Attachment 253248 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1015:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1021:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1167:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:531:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jeremy Jones]

Created attachment 253250 [details]
Patch

[WebKit Commit Bot]

Attachment 253250 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1015:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1021:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1167:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 253250 [details]
Patch

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

I’m going to say review- because there are many small thread safety mistakes. Otherwise, this patch looks good to me.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:81
> +class WebVideoFullscreenControllerContext
> +: public WebVideoFullscreenInterface
> +, public WebVideoFullscreenModel
> +, public WebVideoFullscreenChangeObserver
> +, public ThreadSafeRefCounted<WebVideoFullscreenControllerContext> {

This is not the traditional formatting for this in WebKit. You should either indent this all by 4 spaces, or put it all on one single super-long line.

Might also consider if any of the inheritance can be private or protected rather than public.

Can we mark this class final?

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:90
> +private:
> +    RefPtr<WebVideoFullscreenInterfaceAVKit> m_interface;
> +    RefPtr<WebVideoFullscreenModelVideoElement> m_model;
> +    RefPtr<HTMLVideoElement> m_videoElement;
> +    RetainPtr<PlatformLayer> m_videoFullscreenLayer;
> +    RetainPtr<WebVideoFullscreenController> m_controller;
> +    FloatRect m_frame;
> +    VideoGravity m_gravity;

In WebKit class definitions we normally put the data members last, not at the start.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:92
> +    WebVideoFullscreenControllerContext() { };

Extra semicolon here. Also, we normally put everything private after everything public.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:97
> +        return adoptRef(*new WebVideoFullscreenControllerContext());

Could omit the () here.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:140
> +    // WebVideoFullscreenChangeObserver
> +    virtual void didSetupFullscreen() override;
> +    virtual void didEnterFullscreen() override { }
> +    virtual void didExitFullscreen() override;
> +    virtual void didCleanupFullscreen() override;
> +    virtual void fullscreenMayReturnToInline() override;
> +    
> +    // WebVideoFullscreenInterface
> +    virtual void resetMediaState() override;
> +    virtual void setDuration(double) override;
> +    virtual void setCurrentTime(double currentTime, double anchorTime) override;
> +    virtual void setBufferedTime(double) override;
> +    virtual void setRate(bool isPlaying, float playbackRate) override;
> +    virtual void setVideoDimensions(bool hasVideo, float width, float height) override;
> +    virtual void setSeekableRanges(const TimeRanges&) override;
> +    virtual void setCanPlayFastReverse(bool) override;
> +    virtual void setAudioMediaSelectionOptions(const Vector<WTF::String>& options, uint64_t selectedIndex) override;
> +    virtual void setLegibleMediaSelectionOptions(const Vector<WTF::String>& options, uint64_t selectedIndex) override;
> +    virtual void setExternalPlayback(bool enabled, ExternalPlaybackTargetType, WTF::String localizedDeviceName) override;
> +    
> +    // WebVideoFullscreenModel
> +    virtual void play() override;
> +    virtual void pause() override;
> +    virtual void togglePlayState() override;
> +    virtual void beginScrubbing() override;
> +    virtual void endScrubbing() override;
> +    virtual void seekToTime(double time) override;
> +    virtual void fastSeek(double time) override;
> +    virtual void beginScanningForward() override;
> +    virtual void beginScanningBackward() override;
> +    virtual void endScanning() override;
> +    virtual void requestExitFullscreen() override;
> +    virtual void setVideoLayerFrame(FloatRect) override;
> +    virtual FloatRect videoLayerFrame() const override { return m_frame; }
> +    virtual void setVideoLayerGravity(WebVideoFullscreenModel::VideoGravity) override;
> +    virtual VideoGravity videoLayerGravity() const override { return m_gravity; }
> +    virtual void selectAudioMediaOption(uint64_t index) override;
> +    virtual void selectLegibleMediaOption(uint64_t index) override;
> +    virtual void fullscreenModeChanged(HTMLMediaElement::VideoFullscreenMode) override;

I think all these overrides should be private instead of public.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:143
> +    void setupFullscreen(HTMLVideoElement*, UIView *, HTMLMediaElement::VideoFullscreenMode);

The verb "set up" is two words, so it should be "setUp" not "setup". Not sure if we mean "fullscreen" to be one word or two. I imagine HTMLVideoElement is required to be non-null here and if so, it should be HTMLVideoElement& instead of *.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:277
> +    RefPtr<TimeRanges> timeRangesRef = timeRanges.copy();
> +    dispatch_async(dispatch_get_main_queue(), [strongThis, this, timeRangesRef] {
> +        if (m_interface)
> +            m_interface->setSeekableRanges(*timeRangesRef.get());
> +    });

Unless there is something magic about the web thread and main thread that I don’t understand, I am pretty sure this isn’t thread safe.

The code will manipulate the reference count of timeRangesRef both on the web thread and the main thread, and the two could try to access it at the same time; ~RefPtr on the web thread and ~RefPtr on the main thread for example. I see here that we are trying to copy the time ranges in an effort to avoid this problem, but unfortunately it won’t. Antti solved this for WTF::String with the class StringCapture, but replicating something like that for time ranges is probably overkill.

I can think of three ways to avoid this problem:

1) Use a raw pointer instead of a RefPtr, doing an explicit leakRef and then balancing that out with either adoptRef or deref in the lambda.
2) Make TimeRanges use ThreadSafeRefCounted. Annoying because it could be costly for all uses of TimeRanges.
3) Write the equivalent of StringCapture for TimeRanges.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:290
> +void WebVideoFullscreenControllerContext::setAudioMediaSelectionOptions(const Vector<WTF::String>& options, uint64_t selectedIndex)

Should just be String, not WTF::String.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:297
> +    dispatch_async(dispatch_get_main_queue(), [strongThis, this, options, selectedIndex] {
> +        if (m_interface)
> +            m_interface->setAudioMediaSelectionOptions(options, selectedIndex);
> +    });

Unless there is something magic about the web thread and main thread that I don’t understand, I am pretty sure this isn’t thread safe. Since options is a vector of strings with non-thread-safe reference counts, we have to make isolated copies of them to move them across threads. One way to avoid the problem would be to make an isolated copy of the vector and pass that over to the other thread. The old code avoided this by making an NSArray of NSString copy of the vector, which was safe since Objective-C reference counts are all thread safe.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:300
> +void WebVideoFullscreenControllerContext::setLegibleMediaSelectionOptions(const Vector<WTF::String>& options, uint64_t selectedIndex)

Should be String, not WTF::String.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:307
> +    dispatch_async(dispatch_get_main_queue(), [strongThis, this, options, selectedIndex] {
> +        if (m_interface)
> +            m_interface->setLegibleMediaSelectionOptions(options, selectedIndex);
> +    });

Same problem as above for options.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:314
> +    dispatch_async(dispatch_get_main_queue(), [strongThis, this, enabled, type, localizedDeviceName] {

Unless there is something magic about the web thread and main thread that I don’t understand, I am pretty sure this isn’t thread safe. Need to use StringCapture to safely pass localizedDeviceName across threads. Plenty of examples of how to do that if you grep for StringCapture.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:506
> +        RefPtr<WebVideoFullscreenControllerContext> strongThis(this);

Don’t need another strongThis. Can just capture the one that already exists.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:577
> +    UNUSED_PARAM(context);

Might be more elegant to do:

    ASSERT_UNUSED(context, context == _context);

Unless that assertion is not guaranteed!

[Jeremy Jones]

(In reply to comment #7)
> Comment on attachment 253250 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253250&action=review
> 
> I’m going to say review- because there are many small thread safety
> mistakes. Otherwise, this patch looks good to me.
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:81
> > +class WebVideoFullscreenControllerContext
> > +: public WebVideoFullscreenInterface
> > +, public WebVideoFullscreenModel
> > +, public WebVideoFullscreenChangeObserver
> > +, public ThreadSafeRefCounted<WebVideoFullscreenControllerContext> {
> 
> This is not the traditional formatting for this in WebKit. You should either
> indent this all by 4 spaces, or put it all on one single super-long line.

Indention added.

> 
> Might also consider if any of the inheritance can be private or protected
> rather than public.

All inheritance marked private except ThreadSafeRef.

> 
> Can we mark this class final?

Marked final.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:90
> > +private:
> > +    RefPtr<WebVideoFullscreenInterfaceAVKit> m_interface;
> > +    RefPtr<WebVideoFullscreenModelVideoElement> m_model;
> > +    RefPtr<HTMLVideoElement> m_videoElement;
> > +    RetainPtr<PlatformLayer> m_videoFullscreenLayer;
> > +    RetainPtr<WebVideoFullscreenController> m_controller;
> > +    FloatRect m_frame;
> > +    VideoGravity m_gravity;
> 
> In WebKit class definitions we normally put the data members last, not at
> the start.

Data members moved to the bottom.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:92
> > +    WebVideoFullscreenControllerContext() { };
> 
> Extra semicolon here. Also, we normally put everything private after
> everything public.

Semicolon moved. Private methods and data moved to the bottom. 

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:97
> > +        return adoptRef(*new WebVideoFullscreenControllerContext());
> 
> Could omit the () here.

Parens removed.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:140
> > +    // WebVideoFullscreenChangeObserver
> > +    virtual void didSetupFullscreen() override;
> > +    virtual void didEnterFullscreen() override { }
> > +    virtual void didExitFullscreen() override;
> > +    virtual void didCleanupFullscreen() override;
> > +    virtual void fullscreenMayReturnToInline() override;
> > +    
> > +    // WebVideoFullscreenInterface
> > +    virtual void resetMediaState() override;
> > +    virtual void setDuration(double) override;
> > +    virtual void setCurrentTime(double currentTime, double anchorTime) override;
> > +    virtual void setBufferedTime(double) override;
> > +    virtual void setRate(bool isPlaying, float playbackRate) override;
> > +    virtual void setVideoDimensions(bool hasVideo, float width, float height) override;
> > +    virtual void setSeekableRanges(const TimeRanges&) override;
> > +    virtual void setCanPlayFastReverse(bool) override;
> > +    virtual void setAudioMediaSelectionOptions(const Vector<WTF::String>& options, uint64_t selectedIndex) override;
> > +    virtual void setLegibleMediaSelectionOptions(const Vector<WTF::String>& options, uint64_t selectedIndex) override;
> > +    virtual void setExternalPlayback(bool enabled, ExternalPlaybackTargetType, WTF::String localizedDeviceName) override;
> > +    
> > +    // WebVideoFullscreenModel
> > +    virtual void play() override;
> > +    virtual void pause() override;
> > +    virtual void togglePlayState() override;
> > +    virtual void beginScrubbing() override;
> > +    virtual void endScrubbing() override;
> > +    virtual void seekToTime(double time) override;
> > +    virtual void fastSeek(double time) override;
> > +    virtual void beginScanningForward() override;
> > +    virtual void beginScanningBackward() override;
> > +    virtual void endScanning() override;
> > +    virtual void requestExitFullscreen() override;
> > +    virtual void setVideoLayerFrame(FloatRect) override;
> > +    virtual FloatRect videoLayerFrame() const override { return m_frame; }
> > +    virtual void setVideoLayerGravity(WebVideoFullscreenModel::VideoGravity) override;
> > +    virtual VideoGravity videoLayerGravity() const override { return m_gravity; }
> > +    virtual void selectAudioMediaOption(uint64_t index) override;
> > +    virtual void selectLegibleMediaOption(uint64_t index) override;
> > +    virtual void fullscreenModeChanged(HTMLMediaElement::VideoFullscreenMode) override;
> 
> I think all these overrides should be private instead of public.
> 

Overrides moved down to private section of the class.

> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:143
> > +    void setupFullscreen(HTMLVideoElement*, UIView *, HTMLMediaElement::VideoFullscreenMode);
> 
> The verb "set up" is two words, so it should be "setUp" not "setup". Not
> sure if we mean "fullscreen" to be one word or two. I imagine
> HTMLVideoElement is required to be non-null here and if so, it should be
> HTMLVideoElement& instead of *.

"setup" is one word in many related places. I'll make the change here and file
a but to change it elsewhere.

Change HTMLVideoElement to be a & instead of *.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:277
> > +    RefPtr<TimeRanges> timeRangesRef = timeRanges.copy();
> > +    dispatch_async(dispatch_get_main_queue(), [strongThis, this, timeRangesRef] {
> > +        if (m_interface)
> > +            m_interface->setSeekableRanges(*timeRangesRef.get());
> > +    });
> 
> Unless there is something magic about the web thread and main thread that I
> don’t understand, I am pretty sure this isn’t thread safe.
> 
> The code will manipulate the reference count of timeRangesRef both on the
> web thread and the main thread, and the two could try to access it at the
> same time; ~RefPtr on the web thread and ~RefPtr on the main thread for
> example. I see here that we are trying to copy the time ranges in an effort
> to avoid this problem, but unfortunately it won’t. Antti solved this for
> WTF::String with the class StringCapture, but replicating something like
> that for time ranges is probably overkill.
> 
> I can think of three ways to avoid this problem:
> 
> 1) Use a raw pointer instead of a RefPtr, doing an explicit leakRef and then
> balancing that out with either adoptRef or deref in the lambda.

This will work, but requires the lambda only be executed once or you risk over-releasing.

Instead, I make copies of the time ranges, create the block (which captures the time ranges), release the time ranges, then dispatch the block.

It looks like this:

    RefPtr<TimeRanges> timeRangesRef = timeRanges.copy();
    auto block = [strongThis, this, timeRangesRef] {
        if (m_interface)
            m_interface->setSeekableRanges(*timeRangesRef.get());
    };
    timeRangesRef.release();
    dispatch_async(dispatch_get_main_queue(), block);


> 2) Make TimeRanges use ThreadSafeRefCounted. Annoying because it could be
> costly for all uses of TimeRanges.
> 3) Write the equivalent of StringCapture for TimeRanges.
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:290
> > +void WebVideoFullscreenControllerContext::setAudioMediaSelectionOptions(const Vector<WTF::String>& options, uint64_t selectedIndex)
> 
> Should just be String, not WTF::String.

Changed in several places.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:297
> > +    dispatch_async(dispatch_get_main_queue(), [strongThis, this, options, selectedIndex] {
> > +        if (m_interface)
> > +            m_interface->setAudioMediaSelectionOptions(options, selectedIndex);
> > +    });
> 
> Unless there is something magic about the web thread and main thread that I
> don’t understand, I am pretty sure this isn’t thread safe. Since options is
> a vector of strings with non-thread-safe reference counts, we have to make
> isolated copies of them to move them across threads. One way to avoid the
> problem would be to make an isolated copy of the vector and pass that over
> to the other thread. The old code avoided this by making an NSArray of
> NSString copy of the vector, which was safe since Objective-C reference
> counts are all thread safe.

I use a similar method here as above. I make a deep copy, capture it, then release it.

It looks like this:

    Vector<String> optionsDeepCopy;
    optionsDeepCopy.reserveInitialCapacity(options.size());
    for (auto& option : options) {
        optionsDeepCopy.append(option.isolatedCopy());
    }
    
    auto block = [strongThis, this, optionsDeepCopy, selectedIndex] {
        if (m_interface)
            m_interface->setAudioMediaSelectionOptions(optionsDeepCopy, selectedIndex);
    };
    
    optionsDeepCopy.clear();
    dispatch_async(dispatch_get_main_queue(), block);


> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:300
> > +void WebVideoFullscreenControllerContext::setLegibleMediaSelectionOptions(const Vector<WTF::String>& options, uint64_t selectedIndex)
> 
> Should be String, not WTF::String.

Changed here and other places.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:307
> > +    dispatch_async(dispatch_get_main_queue(), [strongThis, this, options, selectedIndex] {
> > +        if (m_interface)
> > +            m_interface->setLegibleMediaSelectionOptions(options, selectedIndex);
> > +    });
> 
> Same problem as above for options.

Same solution as above for options.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:314
> > +    dispatch_async(dispatch_get_main_queue(), [strongThis, this, enabled, type, localizedDeviceName] {
> 
> Unless there is something magic about the web thread and main thread that I
> don’t understand, I am pretty sure this isn’t thread safe. Need to use
> StringCapture to safely pass localizedDeviceName across threads. Plenty of
> examples of how to do that if you grep for StringCapture.

Done.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:506
> > +        RefPtr<WebVideoFullscreenControllerContext> strongThis(this);
> 
> Don’t need another strongThis. Can just capture the one that already exists.

Removed.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:577
> > +    UNUSED_PARAM(context);
> 
> Might be more elegant to do:
> 
>     ASSERT_UNUSED(context, context == _context);
> 
> Unless that assertion is not guaranteed!

Done.

[Jeremy Jones]

Created attachment 253567 [details]
Patch

[WebKit Commit Bot]

Attachment 253567 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1015:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1021:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1164:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

<rdar://problem/19242538>

[Jeremy Jones]

(In reply to comment #8)
> (In reply to comment #7)
> > > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:277
> > > +    RefPtr<TimeRanges> timeRangesRef = timeRanges.copy();
> > > +    dispatch_async(dispatch_get_main_queue(), [strongThis, this, timeRangesRef] {
> > > +        if (m_interface)
> > > +            m_interface->setSeekableRanges(*timeRangesRef.get());
> > > +    });
> > 
> > Unless there is something magic about the web thread and main thread that I
> > don’t understand, I am pretty sure this isn’t thread safe.
> > 
> > The code will manipulate the reference count of timeRangesRef both on the
> > web thread and the main thread, and the two could try to access it at the
> > same time; ~RefPtr on the web thread and ~RefPtr on the main thread for
> > example. I see here that we are trying to copy the time ranges in an effort
> > to avoid this problem, but unfortunately it won’t. Antti solved this for
> > WTF::String with the class StringCapture, but replicating something like
> > that for time ranges is probably overkill.
> > 
> > I can think of three ways to avoid this problem:
> > 
> > 1) Use a raw pointer instead of a RefPtr, doing an explicit leakRef and then
> > balancing that out with either adoptRef or deref in the lambda.
> 
> This will work, but requires the lambda only be executed once or you risk
> over-releasing.
> 
> Instead, I make copies of the time ranges, create the block (which captures
> the time ranges), release the time ranges, then dispatch the block.
> 
> It looks like this:
> 
>     RefPtr<TimeRanges> timeRangesRef = timeRanges.copy();
>     auto block = [strongThis, this, timeRangesRef] {
>         if (m_interface)
>             m_interface->setSeekableRanges(*timeRangesRef.get());
>     };
>     timeRangesRef.release();
>     dispatch_async(dispatch_get_main_queue(), block);
> 

This won't work. The block is copied, leaving two refs to the time ranges.

> > > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:297
> > > +    dispatch_async(dispatch_get_main_queue(), [strongThis, this, options, selectedIndex] {
> > > +        if (m_interface)
> > > +            m_interface->setAudioMediaSelectionOptions(options, selectedIndex);
> > > +    });
> > 
> > Unless there is something magic about the web thread and main thread that I
> > don’t understand, I am pretty sure this isn’t thread safe. Since options is
> > a vector of strings with non-thread-safe reference counts, we have to make
> > isolated copies of them to move them across threads. One way to avoid the
> > problem would be to make an isolated copy of the vector and pass that over
> > to the other thread. The old code avoided this by making an NSArray of
> > NSString copy of the vector, which was safe since Objective-C reference
> > counts are all thread safe.
> 
> I use a similar method here as above. I make a deep copy, capture it, then
> release it.
> 
> It looks like this:
> 
>     Vector<String> optionsDeepCopy;
>     optionsDeepCopy.reserveInitialCapacity(options.size());
>     for (auto& option : options) {
>         optionsDeepCopy.append(option.isolatedCopy());
>     }
>     
>     auto block = [strongThis, this, optionsDeepCopy, selectedIndex] {
>         if (m_interface)
>             m_interface->setAudioMediaSelectionOptions(optionsDeepCopy,
> selectedIndex);
>     };
>     
>     optionsDeepCopy.clear();
>     dispatch_async(dispatch_get_main_queue(), block);
> 
> 

This won't work as the block is copied, leaving two vectors referring to the same string impls.

[Jeremy Jones]

(In reply to comment #12)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:277
> > > > +    RefPtr<TimeRanges> timeRangesRef = timeRanges.copy();
> > > > +    dispatch_async(dispatch_get_main_queue(), [strongThis, this, timeRangesRef] {
> > > > +        if (m_interface)
> > > > +            m_interface->setSeekableRanges(*timeRangesRef.get());
> > > > +    });
> > > 
> > > Unless there is something magic about the web thread and main thread that I
> > > don’t understand, I am pretty sure this isn’t thread safe.
> > > 
> > > The code will manipulate the reference count of timeRangesRef both on the
> > > web thread and the main thread, and the two could try to access it at the
> > > same time; ~RefPtr on the web thread and ~RefPtr on the main thread for
> > > example. I see here that we are trying to copy the time ranges in an effort
> > > to avoid this problem, but unfortunately it won’t. Antti solved this for
> > > WTF::String with the class StringCapture, but replicating something like
> > > that for time ranges is probably overkill.
> > > 
> > > I can think of three ways to avoid this problem:
> > > 
> > > 1) Use a raw pointer instead of a RefPtr, doing an explicit leakRef and then
> > > balancing that out with either adoptRef or deref in the lambda.
> > 
> > This will work, but requires the lambda only be executed once or you risk
> > over-releasing.
> > 
> > Instead, I make copies of the time ranges, create the block (which captures
> > the time ranges), release the time ranges, then dispatch the block.
> > 
> > It looks like this:
> > 
> >     RefPtr<TimeRanges> timeRangesRef = timeRanges.copy();
> >     auto block = [strongThis, this, timeRangesRef] {
> >         if (m_interface)
> >             m_interface->setSeekableRanges(*timeRangesRef.get());
> >     };
> >     timeRangesRef.release();
> >     dispatch_async(dispatch_get_main_queue(), block);
> > 
> 
> This won't work. The block is copied, leaving two refs to the time ranges.

The solution is to use PlatformTimeRanges, which is a value type.

> 
> > > > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:297
> > > > +    dispatch_async(dispatch_get_main_queue(), [strongThis, this, options, selectedIndex] {
> > > > +        if (m_interface)
> > > > +            m_interface->setAudioMediaSelectionOptions(options, selectedIndex);
> > > > +    });
> > > 
> > > Unless there is something magic about the web thread and main thread that I
> > > don’t understand, I am pretty sure this isn’t thread safe. Since options is
> > > a vector of strings with non-thread-safe reference counts, we have to make
> > > isolated copies of them to move them across threads. One way to avoid the
> > > problem would be to make an isolated copy of the vector and pass that over
> > > to the other thread. The old code avoided this by making an NSArray of
> > > NSString copy of the vector, which was safe since Objective-C reference
> > > counts are all thread safe.
> > 
> > I use a similar method here as above. I make a deep copy, capture it, then
> > release it.
> > 
> > It looks like this:
> > 
> >     Vector<String> optionsDeepCopy;
> >     optionsDeepCopy.reserveInitialCapacity(options.size());
> >     for (auto& option : options) {
> >         optionsDeepCopy.append(option.isolatedCopy());
> >     }
> >     
> >     auto block = [strongThis, this, optionsDeepCopy, selectedIndex] {
> >         if (m_interface)
> >             m_interface->setAudioMediaSelectionOptions(optionsDeepCopy,
> > selectedIndex);
> >     };
> >     
> >     optionsDeepCopy.clear();
> >     dispatch_async(dispatch_get_main_queue(), block);
> > 
> > 
> 
> This won't work as the block is copied, leaving two vectors referring to the
> same string impls.

I now convert to NSArray of NSString to get thread safe ref counting.

[Jeremy Jones]

Created attachment 253609 [details]
Patch

[WebKit Commit Bot]

Attachment 253609 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1015:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1021:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1164:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 253609 [details]
Patch

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

Looks good. I don’t see any threading mistakes.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:276
> +        RefPtr<TimeRanges> timeRanges = TimeRanges::create(platformTimeRanges);
> +        if (m_interface)
> +            m_interface->setSeekableRanges(*timeRanges.get());

I’d write this without the local variable. Something like this:

    if (m_interface)
        m_interface->setSeekableRanges(*TimeRanges::create(platformTimeRanges));

[Jeremy Jones]

(In reply to comment #16)
> Comment on attachment 253609 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253609&action=review
> 
> Looks good. I don’t see any threading mistakes.
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:276
> > +        RefPtr<TimeRanges> timeRanges = TimeRanges::create(platformTimeRanges);
> > +        if (m_interface)
> > +            m_interface->setSeekableRanges(*timeRanges.get());
> 
> I’d write this without the local variable. Something like this:
> 
>     if (m_interface)
>        
> m_interface->setSeekableRanges(*TimeRanges::create(platformTimeRanges));

Changed.

[Jeremy Jones]

Created attachment 253796 [details]
Patch for landing.

[WebKit Commit Bot]

Attachment 253796 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1012:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1018:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1161:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 253796 [details]
Patch for landing.

Clearing flags on attachment: 253796

Committed r184922: <http://trac.webkit.org/changeset/184922>