Bug 45306 – Media elements should derive from ActiveDOMObjects

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

[Eric Carlson]

Media elements should derive from ActiveDOMObjects instead of implementing documentWillBecomeInactive/documentDidBecomeActive. It will allow us to prevent the object from being collected when it has pending events in the async event queue, which causes an assert in a debug build and a potential crash in a release build, and to remove the <audio> specific code from isObservableThroughDOM.

[Eric Carlson]

<rdar://problem/7929062>

[Eric Carlson]

Created attachment 66739 [details]
proposed patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/html/HTMLMediaElement.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


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

[Eric Carlson]

This patch doesn't add a test case because https://bugs.webkit.org/show_bug.cgi?id=45309 allows the object to be collected even when it has pending events. Getting these changes in now will make it easier to reproduce that crash.

[Simon Fraser (smfr)]

Want to take bug 24030 too?

[Darin Adler]

Comment on attachment 66739 [details]
proposed patch

Do we already have tests that cover a media element with pending events where we force the last reference to be garbage collected?

> +    // ActiveDOMObject methods.

I prefer that we use the C++ terminology. So "functions" rather than "methods".

> +    virtual bool canSuspend() const { return true; }

There doesn’t seem to be significant benefit to putting this definition in the header, since it’s a virtual function that will only be called through a function pointer. I suggest putting it in the .cpp file instead.

It's too bad that the stop function is calling another virtual function, suspend, doing an unnecessary virtual dispatch. Not a major problem, but slightly unpleasant. Instead it could call HTMLMediaElement::suspend(), but that would strange enough that we probably shouldn't do it.

The headers in HTMLMediaElement.h should stay sorted alphabetically. I bet that is what our stylebot is complaining about.

[Eric Carlson]

(In reply to comment #6)
> (From update of attachment 66739 [details])
> Do we already have tests that cover a media element with pending events where we force the last reference 
> to be garbage collected?
> 
The crashing test case attached to https://bugs.webkit.org/show_bug.cgi?id=45309 does exactly that.

> > +    // ActiveDOMObject methods.
> 
> I prefer that we use the C++ terminology. So "functions" rather than "methods".
> 
Done.

> > +    virtual bool canSuspend() const { return true; }
> 
> There doesn’t seem to be significant benefit to putting this definition in the header, since it’s a virtual function 
> that will only be called through a function pointer. I suggest putting it in the .cpp file instead.
> 
Done.

> 
> The headers in HTMLMediaElement.h should stay sorted alphabetically. I bet that is what our stylebot is 
> complaining about.
Fixed that too.

Thanks!

[Eric Carlson]

http://trac.webkit.org/changeset/66895

[Alexey Proskuryakov]

As discussed in person, a problem with this is that ActiveDOMObjects are also suspended below modal dialogs and alerts, as well as by Web Inspector. We probably don't want that for media elements, although we do want to consolidate document inactivation code.

[Eric Carlson]

(In reply to comment #9)
> As discussed in person, a problem with this is that ActiveDOMObjects are also suspended below 
> modal dialogs and alerts, as well as by Web Inspector. We probably don't want that for media 
> elements, although we do want to consolidate document inactivation code.

http://trac.webkit.org/changeset/67432 fix this.

[Ademar Reis]

Revision r66895 cherry-picked into qtwebkit-2.2 with commit e5e1392 <http://gitorious.org/webkit/qtwebkit/commit/e5e1392>