Bug 73865

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.

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

Created attachment 123818 [details] Patch

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.

(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.

Created attachment 125683 [details] Patch

Comment on attachment 125683 [details] Patch I'll resolve the merge conflicts and upload a new patch for review.

Created attachment 125878 [details] Patch

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?

(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.

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.

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

Created attachment 126133 [details] Patch

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.

(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.

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.

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

(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 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.

(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

(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

Are you hoping that this will be reviewed? If not, please clear the r? flag.

Created attachment 170454 [details] Patch

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 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 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.

Created attachment 170985 [details] Moved suspendIfNeeded() into TextTrackCue::create().

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 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 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?

Looks like you also have a compile problem on apple-mac.

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.

There is no documentation. You need to read the code to understand how it works.

*** Bug 105520 has been marked as a duplicate of this bug. ***

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

Added Mac test expectation in http://trac.webkit.org/changeset/138330.

This bug is critical. It’s making all media element track tests intermittently crash.

(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.

(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.

V8 is no longer in WebKit.