Bug 73865 - V8 Wrappers for TextTrack and TextTrackCue should not be collected on event dispatch and when parent/owner are still reachable
Summary: V8 Wrappers for TextTrack and TextTrackCue should not be collected on event d...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Major
Assignee: Erik Arvidsson
URL:
Keywords:
: 105520 (view as bug list)
Depends on: 78052 78149 88541
Blocks: 43668
  Show dependency treegraph
 
Reported: 2011-12-05 14:13 PST by Eric Carlson
Modified: 2013-05-02 12:11 PDT (History)
22 users (show)

See Also:


Attachments
Patch (16.83 KB, patch)
2012-01-24 14:56 PST, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (11.45 KB, patch)
2012-02-06 12:04 PST, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (14.14 KB, patch)
2012-02-07 10:49 PST, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (8.77 KB, patch)
2012-02-08 12:19 PST, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (7.34 KB, patch)
2012-10-24 12:58 PDT, Aaron Colwell
no flags Details | Formatted Diff | Diff
Moved suspendIfNeeded() into TextTrackCue::create(). (7.35 KB, patch)
2012-10-26 12:45 PDT, Aaron Colwell
abarth: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2011-12-05 14:13:41 PST
Every non-DOM object that is an EventTarget needs code to make sure that the wrapper isn't collected during event dispatch or while the parent/owner object is reachable. For example, see tracklist-is-reachable.html test added in https://bugs.webkit.org/show_bug.cgi?id=71123.


Split off from https://bugs.webkit.org/show_bug.cgi?id=72179 for the V8 changes.
Comment 1 John Knottenbelt 2012-01-09 05:54:01 PST
Until we implement the corresponding V8 changes, we can expect media/track/text-track-is-reachable.html to fail or crash: 

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=media%2Ftrack%2Ftext-track-is-reachable.html

Will adjust test expectations accordingly.
Comment 2 John Knottenbelt 2012-01-09 06:01:45 PST
I think the full list of affected tests are:

media/track/tracklist-is-reachable.html
media/track/text-track-cue-is-reachable.html
media/track/text-track-is-reachable.html
Comment 3 Erik Arvidsson 2012-01-24 14:56:10 PST
Created attachment 123818 [details]
Patch
Comment 4 Erik Arvidsson 2012-01-24 15:02:35 PST
Vitaly, this adds back something similar to what you removed in bug 64467. In this case we add new hidden references to the items of TextTrackList and TextTrackCueList to an HTMLMediaElement. These will be kept alive as long as the media element is alive.
Comment 5 Vitaly Repeshko 2012-01-24 16:06:28 PST
(In reply to comment #4)
> Vitaly, this adds back something similar to what you removed in bug 64467. In this case we add new hidden references to the items of TextTrackList and TextTrackCueList to an HTMLMediaElement. These will be kept alive as long as the media element is alive.

There's a better mechanism for unbounded lists like that. Please search for V8::AddImplicitReferences in V8GCController.cpp. I'm worried that with the current patch it's possible to retain an unbounded number of objects by repeatedly adding and removing new elements to the lists. In general, having addNamedHiddenReference() without a corresponding delete is dangerous.
Comment 6 Erik Arvidsson 2012-02-06 12:04:51 PST
Created attachment 125683 [details]
Patch
Comment 7 Erik Arvidsson 2012-02-06 13:29:42 PST
Comment on attachment 125683 [details]
Patch

I'll resolve the merge conflicts and upload a new patch for review.
Comment 8 Erik Arvidsson 2012-02-07 10:49:55 PST
Created attachment 125878 [details]
Patch
Comment 9 anton muhin 2012-02-08 06:19:10 PST
Comment on attachment 125878 [details]
Patch

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

> Source/WebCore/bindings/v8/V8GCController.cpp:413
> +    void visitDOMWrapper(DOMDataStore* store, TextTrack* textTrack, v8::Persistent<v8::Object> wrapper)

do we need another overload?  Maybe visitTextTrackDOMWrapper?

> Source/WebCore/bindings/v8/V8GCController.cpp:416
> +        if (!mediaElement)

may we have detached TextTrack object?  I mean one which has no mediaElement, but referenced directly from JS?  If yes, don't we want to retain its cues and in this case?

> Source/WebCore/bindings/v8/V8GCController.cpp:433
> +        appendToGrouperList(mediaElement, wrapper);

is TextTrack's media element reachable from text track itself in JavaScript?  if not, you should use implicit references instead.

> Source/WebCore/bindings/v8/custom/V8TextTrackListCustom.cpp:42
> +{

why not use implicit references for this?

> Source/WebCore/bindings/v8/custom/V8TextTrackListCustom.cpp:49
> +    if (!wrapper.IsEmpty() && element) {

you will update hidden property on every toV8 invocation which is not necessary---you only need to update it when you first create a wrapper.

> Source/WebCore/bindings/v8/custom/V8TextTrackListCustom.cpp:52
> +            V8DOMWrapper::setNamedHiddenReference(elementValue.As<v8::Object>(), "textTracks", wrapper);

may TextTrackList change the owner?  if yes, you've got a problem.  and I really think you should use implicit references.

> Source/WebCore/html/TextTrackCue.idl:37
> +        V8DependentLifetime

shouldn't both TextTrack and TextTrackList interfaces in IDLs be marked with V9DependentLifetime as well?
Comment 10 Eric Carlson 2012-02-08 10:26:23 PST
(In reply to comment #9)
> 
> > Source/WebCore/bindings/v8/V8GCController.cpp:416
> > +        if (!mediaElement)
> 
> may we have detached TextTrack object?  I mean one which has no mediaElement, but referenced directly from JS?  If yes, don't we want to retain its cues and in this case?
> 
  A reference to a TextTrack can outlive the parent media element.


> > Source/WebCore/bindings/v8/V8GCController.cpp:433
> > +        appendToGrouperList(mediaElement, wrapper);
> 
> is TextTrack's media element reachable from text track itself in JavaScript?  if not, you should use implicit references instead.
> 
  Not from JavaScript.
Comment 11 anton muhin 2012-02-08 10:27:52 PST
Thanks a lot, Eric!

(In reply to comment #10)
> (In reply to comment #9)
> > 
> > > Source/WebCore/bindings/v8/V8GCController.cpp:416
> > > +        if (!mediaElement)
> > 
> > may we have detached TextTrack object?  I mean one which has no mediaElement, but referenced directly from JS?  If yes, don't we want to retain its cues and in this case?
> > 
>   A reference to a TextTrack can outlive the parent media element.
> 
> 
> > > Source/WebCore/bindings/v8/V8GCController.cpp:433
> > > +        appendToGrouperList(mediaElement, wrapper);
> > 
> > is TextTrack's media element reachable from text track itself in JavaScript?  if not, you should use implicit references instead.
> > 
>   Not from JavaScript.
Comment 12 Erik Arvidsson 2012-02-08 10:58:06 PST
I think I need to back to the drawing board again. I realize how little I understand here and need more time to understand how this all works.

(In reply to comment #9)
> (From update of attachment 125878 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125878&action=review
> 
> > Source/WebCore/bindings/v8/V8GCController.cpp:413
> > +    void visitDOMWrapper(DOMDataStore* store, TextTrack* textTrack, v8::Persistent<v8::Object> wrapper)
> 
> do we need another overload?  Maybe visitTextTrackDOMWrapper?

I'll rename if I end up keeping this

> > Source/WebCore/bindings/v8/V8GCController.cpp:416
> > +        if (!mediaElement)
> 
> may we have detached TextTrack object?  I mean one which has no mediaElement, but referenced directly from JS?  If yes, don't we want to retain its cues and in this case?

No. All TextTracks are associated with a media element. The TextTrackCue objects on the other hand may not be associated with any TextTrackCueList since it can be constructed directly:

new TextTrackCue(...)

> 
> > Source/WebCore/bindings/v8/V8GCController.cpp:433
> > +        appendToGrouperList(mediaElement, wrapper);
> 
> is TextTrack's media element reachable from text track itself in JavaScript?  if not, you should use implicit references instead.

No. 

The TextTrackCue has a reference to the track if it is associated with one


> > Source/WebCore/bindings/v8/custom/V8TextTrackListCustom.cpp:42
> > +{
> 
> why not use implicit references for this?

This whole file is gone now due to http://trac.webkit.org/changeset/107035

> > Source/WebCore/html/TextTrackCue.idl:37
> > +        V8DependentLifetime
> 
> shouldn't both TextTrack and TextTrackList interfaces in IDLs be marked with V9DependentLifetime as well?

TextTrackList is kept alive by a hidden named reference from the HTMLMediaElement
TextTrackCueList is kept alive by a hidden named reference from TextTrack
Comment 13 Erik Arvidsson 2012-02-08 12:19:02 PST
Created attachment 126133 [details]
Patch
Comment 14 Erik Arvidsson 2012-02-08 12:24:37 PST
This still seems wrong to me. If we need to do this for TextTrackList and TextTrackCueList we should probably do this for all other list types that are not list of nodes. If this is the case we should just make the code gen do this for us.
Comment 15 Eric Carlson 2012-02-08 12:25:51 PST
(In reply to comment #12)
> 
> > > Source/WebCore/bindings/v8/V8GCController.cpp:416
> > > +        if (!mediaElement)
> > 
> > may we have detached TextTrack object?  I mean one which has no mediaElement, but referenced directly from JS?  If yes, don't we want to retain its cues and in this case?
> 
> No. All TextTracks are associated with a media element. 

A TextTrack is *initially* associated with a media element, but the media element can be deleted and have its JS object collected while a JS reference to the TextTrack still exists.
Comment 16 Erik Arvidsson 2012-02-08 13:51:20 PST
I added a more generic bug that covers all lists. I believe we should fix this with the code generator instead of adding custom code for all of these.
Comment 17 anton muhin 2012-02-09 03:33:02 PST
Erik, sorry, I got somewhat confused, do you want this patch to be reviewed?

(In reply to comment #13)
> Created an attachment (id=126133) [details]
> Patch
Comment 18 Erik Arvidsson 2012-02-09 10:04:08 PST
(In reply to comment #17)
> Erik, sorry, I got somewhat confused, do you want this patch to be reviewed?

Yes please.

I would like someone to look at it to see if the solution is correct. If it is I would like the code generator to generate this code for all list like wrappers. See bug 78149
Comment 19 anton muhin 2012-02-10 05:53:47 PST
Comment on attachment 126133 [details]
Patch

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

Neat, LGTM.

> Source/WebCore/bindings/v8/V8GCController.cpp:38
> +#include "Element.h"

do you need this include?

> Source/WebCore/bindings/v8/V8GCController.cpp:40
> +#include "HTMLMediaElement.h"

do you need this include?

> Source/WebCore/bindings/v8/V8GCController.cpp:46
> +#include "TextTrack.h"

won't V8<WebCoreClass> include WebCoreClass.h?

> Source/WebCore/bindings/v8/V8GCController.cpp:350
> +        WrapperTypeInfo* type = V8DOMWrapper::domWrapperType(wrapper);

as an option: we may place a flag into WrapperTypeInfo which says if it's a list like type, that would make this method nice and simple.

> Source/WebCore/bindings/v8/V8GCController.cpp:405
> +        Vector<v8::Persistent<v8::Value> > wrappers;

nit: do we place a space after < to match the corresponding closing > sequence?

> Source/WebCore/bindings/v8/V8GCController.cpp:406
> +        for (unsigned i = 0; i < list->length(); ++i) {

nit: I believe in WebCore people rarely use ++i form despite common C wisdom, I might be easily wrong.

> Source/WebCore/bindings/v8/V8GCController.cpp:408
> +            v8::Handle<v8::Object> wrapper = getDOMObjectMap().get(item);

you may want to prefetch DOMObjectMap.
Comment 20 Erik Arvidsson 2012-02-10 10:10:32 PST
(In reply to comment #19)
> do you need this include?
> do you need this include?

Probably not.

> won't V8<WebCoreClass> include WebCoreClass.h?

I thought it was encouraged to include your direct dependencies?

> > Source/WebCore/bindings/v8/V8GCController.cpp:350
> > +        WrapperTypeInfo* type = V8DOMWrapper::domWrapperType(wrapper);
> 
> as an option: we may place a flag into WrapperTypeInfo which says if it's a list like type, that would make this method nice and simple.

I want this code to be generated by the code generator. I haven't yet figured out the cleanest way to do this. Maybe adding a flag to the WrapperTypeInfo is the right way to go there.

> > Source/WebCore/bindings/v8/V8GCController.cpp:405
> > +        Vector<v8::Persistent<v8::Value> > wrappers;
> 
> nit: do we place a space after < to match the corresponding closing > sequence?
> 
> > Source/WebCore/bindings/v8/V8GCController.cpp:406
> > +        for (unsigned i = 0; i < list->length(); ++i) {
> 
> nit: I believe in WebCore people rarely use ++i form despite common C wisdom, I might be easily wrong.
> 
> > Source/WebCore/bindings/v8/V8GCController.cpp:408
> > +            v8::Handle<v8::Object> wrapper = getDOMObjectMap().get(item);
> 
> you may want to prefetch DOMObjectMap.

ok
Comment 21 anton muhin 2012-02-13 05:29:30 PST
(In reply to comment #20)
> (In reply to comment #19)
> > do you need this include?
> > do you need this include?
> 
> Probably not.
> 
> > won't V8<WebCoreClass> include WebCoreClass.h?
> 
> I thought it was encouraged to include your direct dependencies?

I don't know for sure.  I thought we tried to minimize includes to reduce build time, etc., but if you heard otherwise, it's ok too.

> 
> > > Source/WebCore/bindings/v8/V8GCController.cpp:350
> > > +        WrapperTypeInfo* type = V8DOMWrapper::domWrapperType(wrapper);
> > 
> > as an option: we may place a flag into WrapperTypeInfo which says if it's a list like type, that would make this method nice and simple.
> 
> I want this code to be generated by the code generator. I haven't yet figured out the cleanest way to do this. Maybe adding a flag to the WrapperTypeInfo is the right way to go there.
> 
> > > Source/WebCore/bindings/v8/V8GCController.cpp:405
> > > +        Vector<v8::Persistent<v8::Value> > wrappers;
> > 
> > nit: do we place a space after < to match the corresponding closing > sequence?
> > 
> > > Source/WebCore/bindings/v8/V8GCController.cpp:406
> > > +        for (unsigned i = 0; i < list->length(); ++i) {
> > 
> > nit: I believe in WebCore people rarely use ++i form despite common C wisdom, I might be easily wrong.
> > 
> > > Source/WebCore/bindings/v8/V8GCController.cpp:408
> > > +            v8::Handle<v8::Object> wrapper = getDOMObjectMap().get(item);
> > 
> > you may want to prefetch DOMObjectMap.
> 
> ok
Comment 22 Eric Carlson 2012-04-19 10:44:42 PDT
Are you hoping that this will be reviewed? If not, please clear the r? flag.
Comment 23 Aaron Colwell 2012-10-24 12:58:09 PDT
Created attachment 170454 [details]
Patch
Comment 24 Aaron Colwell 2012-10-24 13:05:27 PDT
I've just uploaded my attempt to fix this bug for V8. I fully admit don't really understand the tradeoff between using V8CustomIsReachable and ActiveDOMObject for TextTrackCue. My solution looks simpler than the custom bindings in WebCore/bindings/js/JSTextTrackCueCustom.cpp but it is possible I am missing something critical. Advice is definitely welcome.
Comment 25 Adam Barth 2012-10-24 13:10:09 PDT
Comment on attachment 170454 [details]
Patch

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

> Source/WebCore/html/track/TextTrackCue.cpp:235
> +    suspendIfNeeded();

We usually call this function in create().  Take a look at how the other subclasses of ActiveDOMObject handle this issue.

> Source/WebCore/html/track/TextTrackCue.cpp:1063
> +    return m_track;

Are we sure that m_track will become false at some point?  Otherwise we'll leak.
Comment 26 Aaron Colwell 2012-10-24 14:07:46 PDT
Comment on attachment 170454 [details]
Patch

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

>> Source/WebCore/html/track/TextTrackCue.cpp:235
>> +    suspendIfNeeded();
> 
> We usually call this function in create().  Take a look at how the other subclasses of ActiveDOMObject handle this issue.

Whoops. I didn't see the create() function in the .h file. Done.

>> Source/WebCore/html/track/TextTrackCue.cpp:1063
>> +    return m_track;
> 
> Are we sure that m_track will become false at some point?  Otherwise we'll leak.

TextTrack::removeCue() calls setTrack(0) on the cue, but it looks like that is the only scenario where this happens. I believe this may mean there are already leaks since there appears to be the following circular reference :

TextTrack -> TextTrackCueList
    ^               |
    |               V
    +----------TextTrackCue

I'll dig deeper into this and file a separate bug & patch if I find something.
Comment 27 Aaron Colwell 2012-10-26 12:45:13 PDT
Created attachment 170985 [details]
Moved suspendIfNeeded() into TextTrackCue::create().
Comment 28 Aaron Colwell 2012-10-26 12:48:04 PDT
Just posted a new patch that moves the suspendIfNeeded() call to the create() method.

I just fixed the TextTrack & TextTrackCue circular reference (https://bugs.webkit.org/show_bug.cgi?id=100300) so I'm pretty confident now that m_track will always get cleared.
Comment 29 Build Bot 2012-10-26 13:09:10 PDT
Comment on attachment 170985 [details]
Moved suspendIfNeeded() into TextTrackCue::create().

Attachment 170985 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14614120
Comment 30 Adam Barth 2012-10-26 14:23:30 PDT
Comment on attachment 170985 [details]
Moved suspendIfNeeded() into TextTrackCue::create().

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

This patch looks good, but I'm not sure why V8 and JSC are using different mechanisms.  Can JSC use the same mechanism you're adding to this patch?  How does the existing JSC mechanism differ from the one you're adding here?

> Source/WebCore/html/track/TextTrack.idl:31
>      JSCustomMarkFunction,
> -    JSCustomIsReachable
> +    JSCustomIsReachable,

Should JSC use ImplOwnerRoot as well?  If you use the attribute GenerateIsReachable rather than V8GenerateIsReachable, it will be applied to both JSC and V8.

> Source/WebCore/html/track/TextTrackCue.cpp:1062
> +    return m_track;

Can we add an ASSERT, perhaps to stop() but perhaps elsewhere, that this eventually becomes 0?

> Source/WebCore/html/track/TextTrackCue.idl:35
>      JSCustomMarkFunction,
> -    JSCustomIsReachable
> +    JSCustomIsReachable,
> +    ActiveDOMObject

Does JSC still need these CustomMark, CustomIsReachable attributes, or is ActiveDOMObject sufficient for JSC as well?
Comment 31 Adam Barth 2012-10-26 14:24:05 PDT
Looks like you also have a compile problem on apple-mac.
Comment 32 Aaron Colwell 2012-10-26 15:03:44 PDT
Comment on attachment 170985 [details]
Moved suspendIfNeeded() into TextTrackCue::create().

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

>> Source/WebCore/html/track/TextTrack.idl:31
>> +    JSCustomIsReachable,
> 
> Should JSC use ImplOwnerRoot as well?  If you use the attribute GenerateIsReachable rather than V8GenerateIsReachable, it will be applied to both JSC and V8.

I'll dig deeper. It does seem like both should be able to use the same mechanism. Documentation on these different options are a little thin so it is hard to know what is the best path.

>> Source/WebCore/html/track/TextTrackCue.idl:35
>> +    ActiveDOMObject
> 
> Does JSC still need these CustomMark, CustomIsReachable attributes, or is ActiveDOMObject sufficient for JSC as well?

I'm investigating whether ActiveDOMObject can replace the custom reachable function. One thing that isn't clear to me is whether ActiveDOMObject takes into account whether event handlers are firing. The custom reachability handler (http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/js/JSTextTrackCueCustom.cpp?order=name#L44) appears to take this into account when it is determining reachability. Unfortunately this method isn't const so I can't call it from hasPendingActivity().

Any docs or pointers about custom reachable handlers vs ActiveDOMObject would be greatly appreciated.
Comment 33 Adam Barth 2012-10-26 15:05:22 PDT
There is no documentation.  You need to read the code to understand how it works.
Comment 34 Ilya Tikhonovsky 2012-12-20 02:55:38 PST
*** Bug 105520 has been marked as a duplicate of this bug. ***
Comment 35 Ryosuke Niwa 2012-12-20 17:35:48 PST
Comment on attachment 170985 [details]
Moved suspendIfNeeded() into TextTrackCue::create().

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

>>> Source/WebCore/html/track/TextTrack.idl:31
>>> +    JSCustomIsReachable,
>> 
>> Should JSC use ImplOwnerRoot as well?  If you use the attribute GenerateIsReachable rather than V8GenerateIsReachable, it will be applied to both JSC and V8.
> 
> I'll dig deeper. It does seem like both should be able to use the same mechanism. Documentation on these different options are a little thin so it is hard to know what is the best path.

FWIW, we have the same crash in JSC:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=media%2Ftrack%2Ftracklist-is-reachable.html
Comment 36 Ryosuke Niwa 2012-12-20 17:41:12 PST
Added Mac test expectation in http://trac.webkit.org/changeset/138330.
Comment 37 Ryosuke Niwa 2012-12-20 19:02:27 PST
This bug is critical. It’s making all media element track tests intermittently crash.
Comment 38 Eric Carlson 2012-12-20 19:37:12 PST
(In reply to comment #37)
> This bug is critical. It’s making all media element track tests intermittently crash.

This bug is V8 specific, the corresponding JSC changes were in 
https://bugs.webkit.org/show_bug.cgi?id=72179 and have worked until now so something else is causing the new failures.
Comment 39 Ryosuke Niwa 2012-12-20 20:04:33 PST
(In reply to comment #38)
> (In reply to comment #37)
> > This bug is critical. It’s making all media element track tests intermittently crash.
> 
> This bug is V8 specific, the corresponding JSC changes were in 
> https://bugs.webkit.org/show_bug.cgi?id=72179 and have worked until now so something else is causing the new failures.

Okay. Let me file a bug new then.
Comment 40 Eric Carlson 2013-05-02 12:11:12 PDT
V8 is no longer in WebKit.