Bug 45306 - Media elements should derive from ActiveDOMObjects
Summary: Media elements should derive from ActiveDOMObjects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks: 51249
  Show dependency treegraph
 
Reported: 2010-09-07 09:57 PDT by Eric Carlson
Modified: 2011-01-24 12:18 PST (History)
4 users (show)

See Also:


Attachments
proposed patch (4.63 KB, patch)
2010-09-07 10:39 PDT, Eric Carlson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2010-09-07 09:57:47 PDT
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.
Comment 1 Eric Carlson 2010-09-07 09:58:54 PDT
<rdar://problem/7929062>
Comment 2 Eric Carlson 2010-09-07 10:39:35 PDT
Created attachment 66739 [details]
proposed patch
Comment 3 WebKit Review Bot 2010-09-07 10:41:34 PDT
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.
Comment 4 Eric Carlson 2010-09-07 10:42:26 PDT
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.
Comment 5 Simon Fraser (smfr) 2010-09-07 10:50:13 PDT
Want to take bug 24030 too?
Comment 6 Darin Adler 2010-09-07 10:59:14 PDT
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.
Comment 7 Eric Carlson 2010-09-07 11:09:13 PDT
(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!
Comment 8 Eric Carlson 2010-09-07 11:30:34 PDT
http://trac.webkit.org/changeset/66895
Comment 9 Alexey Proskuryakov 2010-09-08 13:13:35 PDT
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.
Comment 10 Eric Carlson 2010-09-14 13:11:48 PDT
(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.
Comment 11 Ademar Reis 2011-01-24 12:18:00 PST
Revision r66895 cherry-picked into qtwebkit-2.2 with commit e5e1392 <http://gitorious.org/webkit/qtwebkit/commit/e5e1392>