Bug 157655 - Don't execute JavaScript within HTMLMediaElement::stop()
Summary: Don't execute JavaScript within HTMLMediaElement::stop()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ada Chan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-05-12 19:39 PDT by Ada Chan
Modified: 2016-07-30 00:12 PDT (History)
3 users (show)

See Also:


Attachments
Patch (1.65 KB, patch)
2016-05-12 19:43 PDT, Ada Chan
no flags Details | Formatted Diff | Diff
Patch (2.97 KB, patch)
2016-05-16 12:30 PDT, Ada Chan
cdumez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ada Chan 2016-05-12 19:39:15 PDT
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>
Comment 1 Ada Chan 2016-05-12 19:43:23 PDT
Created attachment 278802 [details]
Patch
Comment 2 Eric Carlson 2016-05-12 21:09:01 PDT
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.
Comment 3 Eric Carlson 2016-05-13 08:02:34 PDT
(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.
Comment 4 Ada Chan 2016-05-16 12:29:24 PDT
(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.
Comment 5 Ada Chan 2016-05-16 12:30:38 PDT
Created attachment 279035 [details]
Patch
Comment 6 Chris Dumez 2016-05-16 13:35:09 PDT
Comment on attachment 279035 [details]
Patch

r=me as long as Eric is fine with it as well.
Comment 7 Chris Dumez 2016-05-16 13:36:12 PDT
We probably want to rename to boolean at some point to something like areActiveDOMObjectsSuspendingOrSuspended for clarity though.
Comment 8 Ada Chan 2016-05-16 13:50:35 PDT
(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.