Don't execute JavaScript within HTMLMediaElement::stop()

• Component: Media
• Status: RESOLVED FIXED

[Ada Chan]

HTMLMediaElement::updateMediaControlsAfterPresentationModeChange() added in r200462 can be called by HTMLMediaElement::stop(). Executing JavaScript after the element has been stopped/suspended is not allowed and we need to fix that.

<rdar://problem/26237934>

[Ada Chan]

Created attachment 278802 [details]
Patch

[Eric Carlson]

Comment on attachment 278802 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:6549
> +    if (document().activeDOMObjectsAreSuspended() || document().activeDOMObjectsAreStopped())

You should also return if the controls script hasn't been injected yet, because there can't be a placeholder image of the script isn't running.

[Eric Carlson]

(In reply to comment #2)
> Comment on attachment 278802 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278802&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:6549
> > +    if (document().activeDOMObjectsAreSuspended() || document().activeDOMObjectsAreStopped())
> 
> You should also return if the controls script hasn't been injected yet,
> because there can't be a placeholder image of the script isn't running.

IOW, there is no reason to inject the controls script just to tell it to hide an image that isn't visible.

[Ada Chan]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 278802 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=278802&action=review
> > 
> > > Source/WebCore/html/HTMLMediaElement.cpp:6549
> > > +    if (document().activeDOMObjectsAreSuspended() || document().activeDOMObjectsAreStopped())
> > 
> > You should also return if the controls script hasn't been injected yet,
> > because there can't be a placeholder image of the script isn't running.
> 
> IOW, there is no reason to inject the controls script just to tell it to
> hide an image that isn't visible.

Thanks, I'll also bail early if !m_mediaControlsHost as you suggested.

It turns out the suspended check doesn't really work because in ScriptExecutionContext::suspendActiveDOMObjects(), m_activeDOMObjectsAreSuspended is only set to true after suspending all objects. So we won't prevent script from executing while suspending the objects. I've talked to Chris Dumez about this, and he suggested moving the setting of that boolean to true earlier before suspending the objects.

I'll be submitting a new patch for EWS.

[Ada Chan]

Created attachment 279035 [details]
Patch

[Chris Dumez]

Comment on attachment 279035 [details]
Patch

r=me as long as Eric is fine with it as well.

[Chris Dumez]

We probably want to rename to boolean at some point to something like areActiveDOMObjectsSuspendingOrSuspended for clarity though.

[Ada Chan]

(In reply to comment #7)
> We probably want to rename to boolean at some point to something like
> areActiveDOMObjectsSuspendingOrSuspended for clarity though.

Yes, Eric has seen the change and he said it looks OK as long as someone familiar with ScriptExecutionContext looks at it, which I guess it's you, Chris. :-)

Change has been committed:
http://trac.webkit.org/changeset/200965

Agreed with your suggestion on the rename.  I think we should probably do the same for the stopping case too.  I've filed https://bugs.webkit.org/show_bug.cgi?id=157753.