Bug 140893 – Prevent crash when accessing WebAVPlayerController.delegate.

• Component: Media
• Status: RESOLVED FIXED

[Jeremy Jones]

Prevent crash when accessing WebAVPlayerController.delegate.

[Jeremy Jones]

Created attachment 245363 [details]
Patch

[Darin Adler]

Comment on attachment 245363 [details]
Patch

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

It’s a bit of a shame to do extra method calls, by getting the delegate property twice, once to check it for null and a second time to use it.

A lot of this patch seems to change from using property assignment syntax to use method call syntax. And then other parts continue to use property assignment. Why the change to explicit method calls?

> Source/WebCore/ChangeLog:44
> +        (WebVideoFullscreenInterfaceAVKit::cleanupFullscreenInternal): consoldiated cleanup code from invalidate()

Typo here.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:641
> +    m_playerController = adoptNS([[WebAVPlayerController alloc] init]);

Why use assignment instead of initialization syntax?

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:653
> +            if (strongThis->m_videoFullscreenModel)
> +                [strongThis->m_playerController setDelegate:strongThis->m_videoFullscreenModel];

I don’t think the null check here is needed. Seems no harm in calling setDelegate on nil.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:687
> +            playerController.contentDuration = 0;
> +            playerController.maxTime = 0;
> +            playerController.contentDurationWithinEndTimes = 0;
> +            playerController.loadedTimeRanges = @[];
> +            
> +            playerController.canPlay = NO;
> +            playerController.canPause = NO;
> +            playerController.canTogglePlayback = NO;
> +            playerController.hasEnabledAudio = NO;
> +            playerController.canSeek = NO;
> +            playerController.minTime = 0;
> +            playerController.status = AVPlayerControllerStatusUnknown;
> +
> +            playerController.timing = nil;
> +            playerController.rate = 0;
> +
> +            playerController.hasEnabledVideo = NO;
> +            playerController.contentDimensions = CGSizeMake(0, 0);
> +            
> +            RetainPtr<NSMutableArray> seekableRanges = adoptNS([[NSMutableArray alloc] init]);
> +            playerController.seekableTimeRanges = seekableRanges.get();
> +            
> +            playerController.canScanBackward = NO;
> +
> +            playerController.audioMediaSelectionOptions = nil;
> +            playerController.currentAudioMediaSelectionOption = nil;
> +
> +            playerController.legibleMediaSelectionOptions = nil;
> +            playerController.currentLegibleMediaSelectionOption = nil;

This seems like a hard-to-do-correctly coding style. It’s easy for us to forget one of these; I have no idea if we covered everything or missed something! It seems this should be a  method on WebAVPlayerController, so someone modifying that class will notice that it needs to be updated when they add something.

There’s no need for the local variable, seekableRanges:

    playerController.seekableTimeRanges = adoptNS([[NSMutableArray alloc] init]).get();

Or if the autorelease overhead is considered acceptable:

    playerController.seekableTimeRanges = [NSMutableArray array];

Or even:

    playerController.seekableTimeRanges = [@[] mutableCopy];

> Source/WebCore/platform/ios/WebVideoFullscreenModelVideoElement.mm:71
> +    if (m_videoFullscreenInterface)
> +        m_videoFullscreenInterface->resetMediaState();

Is it OK that the media state won’t be reset until later? What prevents a race condition? I think the name of the function should make it clear that it doesn’t actually do anything, just schedules a reset that will happen in the future.

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.messages.in:25
> +    ResetMediaState()

Is this calling an already-existing function? I don’t see the new message handling code in the patch.

[Jeremy Jones]

(In reply to comment #2)
> Comment on attachment 245363 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245363&action=review
> 
> It’s a bit of a shame to do extra method calls, by getting the delegate
> property twice, once to check it for null and a second time to use it.

A language with optionals would make this a lot more elegant.

> 
> A lot of this patch seems to change from using property assignment syntax to
> use method call syntax. And then other parts continue to use property
> assignment. Why the change to explicit method calls?

These changes go from using a function that returns an objc pointer to using a RetainPtr. Keeping property syntax would require adding .get(), where method call syntax gets automatically unwrapped.

> 
> > Source/WebCore/ChangeLog:44
> > +        (WebVideoFullscreenInterfaceAVKit::cleanupFullscreenInternal): consoldiated cleanup code from invalidate()
> 
> Typo here.

fixed.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:641
> > +    m_playerController = adoptNS([[WebAVPlayerController alloc] init]);
> 
> Why use assignment instead of initialization syntax?

Fixed.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:653
> > +            if (strongThis->m_videoFullscreenModel)
> > +                [strongThis->m_playerController setDelegate:strongThis->m_videoFullscreenModel];
> 
> I don’t think the null check here is needed. Seems no harm in calling
> setDelegate on nil.
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:687
> > +            playerController.contentDuration = 0;
> > +            playerController.maxTime = 0;
> > +            playerController.contentDurationWithinEndTimes = 0;
> > +            playerController.loadedTimeRanges = @[];
> > +            
> > +            playerController.canPlay = NO;
> > +            playerController.canPause = NO;
> > +            playerController.canTogglePlayback = NO;
> > +            playerController.hasEnabledAudio = NO;
> > +            playerController.canSeek = NO;
> > +            playerController.minTime = 0;
> > +            playerController.status = AVPlayerControllerStatusUnknown;
> > +
> > +            playerController.timing = nil;
> > +            playerController.rate = 0;
> > +
> > +            playerController.hasEnabledVideo = NO;
> > +            playerController.contentDimensions = CGSizeMake(0, 0);
> > +            
> > +            RetainPtr<NSMutableArray> seekableRanges = adoptNS([[NSMutableArray alloc] init]);
> > +            playerController.seekableTimeRanges = seekableRanges.get();
> > +            
> > +            playerController.canScanBackward = NO;
> > +
> > +            playerController.audioMediaSelectionOptions = nil;
> > +            playerController.currentAudioMediaSelectionOption = nil;
> > +
> > +            playerController.legibleMediaSelectionOptions = nil;
> > +            playerController.currentLegibleMediaSelectionOption = nil;
> 
> This seems like a hard-to-do-correctly coding style. It’s easy for us to
> forget one of these; I have no idea if we covered everything or missed
> something! It seems this should be a  method on WebAVPlayerController, so
> someone modifying that class will notice that it needs to be updated when
> they add something.

Refactored into -[WebAVPlayerController resetState]

> 
> There’s no need for the local variable, seekableRanges:
> 
>     playerController.seekableTimeRanges = adoptNS([[NSMutableArray alloc]
> init]).get();
> 
> Or if the autorelease overhead is considered acceptable:
> 
>     playerController.seekableTimeRanges = [NSMutableArray array];
> 
> Or even:
> 
>     playerController.seekableTimeRanges = [@[] mutableCopy];

Changed to [NSMutableArray array]

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenModelVideoElement.mm:71
> > +    if (m_videoFullscreenInterface)
> > +        m_videoFullscreenInterface->resetMediaState();
> 
> Is it OK that the media state won’t be reset until later? What prevents a
> race condition? I think the name of the function should make it clear that
> it doesn’t actually do anything, just schedules a reset that will happen in
> the future.

Yes it is OK. There are a few reasons for this.
1) The interaction between Model and Interface is always async in WebKit2 due to IPC.
2) The model only pushes properties onto the interface, never reads them back, so there is no expectation that property changes are immediately available to read back.
3) All property changes are scheduled this way. Compare with WebVideoFullscreenInterfaceAVKit::setCanPlayFastReverse()

> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.messages.in:25
> > +    ResetMediaState()
> 
> Is this calling an already-existing function? I don’t see the new message
> handling code in the patch.

This is implemented by the new function WebVideoFullscreenInterfaceAVKit::resetMediaState(). WebVideoFullscreenManagerProxy inherits this implementation.

[Jeremy Jones]

Created attachment 245570 [details]
Patch for landing.

[Jeremy Jones]

(In reply to comment #2)
> Comment on attachment 245363 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245363&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:653
> > +            if (strongThis->m_videoFullscreenModel)
> > +                [strongThis->m_playerController setDelegate:strongThis->m_videoFullscreenModel];
> 
> I don’t think the null check here is needed. Seems no harm in calling
> setDelegate on nil.
> 

Removed this check.

[WebKit Commit Bot]

Comment on attachment 245570 [details]
Patch for landing.

Rejecting attachment 245570 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 245570, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/5754805968437248

[Jeremy Jones]

Created attachment 245786 [details]
Patch for landing.

[Jeremy Jones]

rdar://problem/18408265

[WebKit Commit Bot]

Comment on attachment 245786 [details]
Patch for landing.

Clearing flags on attachment: 245786

Committed r179480: <http://trac.webkit.org/changeset/179480>

[Jon Lee]

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