Prevent more script-observable cases of HTMLMediaElement GC

Prevent more script-observable cases of HTMLMediaElement GC

This is intended to cover all cases where Gecko prevents GC:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=26515#c5

Additionally, change the behavior for MediaSource so that an
HTMLMediaElement attached to a MediaSource is never collected. Since the
reverse is also true (see MediaSource::hasPendingActivity) a connected
media element + media source pair will now live as long as the document.

LayoutTests for the new cases in HTMLMediaElement::hasPendingActivity()
were added:

media/gc-while-delaying-the-load-event.html (was timeout)
http/tests/media/gc-while-network-loading.html (was timeout)
media/gc-while-playing.html (was pass)
media/gc-while-seeking.html (was timeout)
http/tests/media/media-source/mediasource-htmlmediaelement-lifetime.html (was failure)

BUG=400659
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182247

[philipj_slow, 2014-09-11 10:06:36]

philipj@opera.com changed reviewers:
+ acolwell@chromium.org, fs@opera.com, scherkus@chromium.org

[philipj_slow, 2014-09-11 10:06:37]

PTAL, kbr@ has said that fixing this is important.

This is a repeat of an earlier failed CL:
https://codereview.chromium.org/439723003/

The problem with
http/tests/media/media-source/mediasource-closed-on-htmlmediaelement-destruction.html
remains:
FAIL Tests that the MediaSource is closed when the HTMLMediaElement is
destroyed. assert_equals: MediaSource object is closed. expected "closed" but
got "open"

My take is that this test is asserting that GC behavior is observable to script,
which is exactly what shouldn't happen. I think the pass condition should be
changed, but something more is probably needed for the MSE case. From the
original review:

On 2014/08/04 15:41:16, acolwell wrote:
> Wouldn't this prevent HTMLMediaElements from ever getting GCed in the
> preload=metadata case? I don't believe the HTMLMediaElement is required to
> transition to HAVE_CURRENT_DATA in that situation. That particular situation
is
> really easy to get into with MSE if you only end up appending the init segment
> and never append any media segments. Also if a MediaSource is attached and no
> bytes are appended, I believe this change would prevent GC as well even if
> nothing holds a reference to either. That seems dangerous to me. 
> 
> I don't have an alternate proposal yet, but I think the assumption that you
will
> always get to HAVE_CURRENT_DATA is not quite sound. In the non-MSE case you'll
> always have the resource loader to get you out of trouble by signalling some
> sort of error or eventually timing out. In the MSE case you don't have such an
> automatic signal since the element is always waiting for data from the MSE
> object.

For MSE, as long as a script has a reference to an open MediaSource which could
append more data, I think preventing GC of the HTMLMediaElement is the right
thing. However, HTMLMediaElement has a RefPtrWillBeMember<HTMLMediaSource>
m_mediaSource which keep the MediaSource alive, so I don't know how to detect
this situation.

Should things be turned around, so that HTMLMediaSource has a
RefPtrWillBeMember<HTMLMediaElement> and HTMLMediaElement has
WeakPtr<HTMLMediaSource>?

[fs, 2014-09-11 11:22:37]

On 2014/09/11 10:06:37, philipj wrote:
> PTAL, kbr@ has said that fixing this is important.
> 
> This is a repeat of an earlier failed CL:
> https://codereview.chromium.org/439723003/
> 
> The problem with
>
http/tests/media/media-source/mediasource-closed-on-htmlmediaelement-destruction.html
> remains:
> FAIL Tests that the MediaSource is closed when the HTMLMediaElement is
> destroyed. assert_equals: MediaSource object is closed. expected "closed" but
> got "open"
> 
> My take is that this test is asserting that GC behavior is observable to
script,
> which is exactly what shouldn't happen. I think the pass condition should be
> changed, but something more is probably needed for the MSE case.

Agree. Not sure what "more" you're potentially envisioning.

> From the original review:
> 
> On 2014/08/04 15:41:16, acolwell wrote:
> > Wouldn't this prevent HTMLMediaElements from ever getting GCed in the
> > preload=metadata case? I don't believe the HTMLMediaElement is required to
> > transition to HAVE_CURRENT_DATA in that situation. That particular situation
> is
> > really easy to get into with MSE if you only end up appending the init
segment
> > and never append any media segments. Also if a MediaSource is attached and
no
> > bytes are appended, I believe this change would prevent GC as well even if
> > nothing holds a reference to either. That seems dangerous to me. 
> > 
> > I don't have an alternate proposal yet, but I think the assumption that you
> will
> > always get to HAVE_CURRENT_DATA is not quite sound. In the non-MSE case
you'll
> > always have the resource loader to get you out of trouble by signalling some
> > sort of error or eventually timing out. In the MSE case you don't have such
an
> > automatic signal since the element is always waiting for data from the MSE
> > object.
> 
> For MSE, as long as a script has a reference to an open MediaSource which
could
> append more data, I think preventing GC of the HTMLMediaElement is the right
> thing. However, HTMLMediaElement has a RefPtrWillBeMember<HTMLMediaSource>
> m_mediaSource which keep the MediaSource alive, so I don't know how to detect
> this situation.

m_mediaSource->hasOneRef()? (!OILPAN; If I interpreted the above correctly...)

> Should things be turned around, so that HTMLMediaSource has a
> RefPtrWillBeMember<HTMLMediaElement> and HTMLMediaElement has
> WeakPtr<HTMLMediaSource>?

I don't think there ought to be any weak-reference at all here really - the
current presence of one seems more like a reference-loop hack. But as a
resource-use optimization I can see how your scheme could be useful.

[philipj_slow, 2014-09-11 12:44:07]

On 2014/09/11 11:22:37, fs wrote:
> On 2014/09/11 10:06:37, philipj wrote:
> > 
> > something more is probably needed for the MSE case.
> 
> Agree. Not sure what "more" you're potentially envisioning.

Me neither :)

> > For MSE, as long as a script has a reference to an open MediaSource which
> could
> > append more data, I think preventing GC of the HTMLMediaElement is the right
> > thing. However, HTMLMediaElement has a RefPtrWillBeMember<HTMLMediaSource>
> > m_mediaSource which keep the MediaSource alive, so I don't know how to
detect
> > this situation.
> 
> m_mediaSource->hasOneRef()? (!OILPAN; If I interpreted the above correctly...)

Ah, I didn't know about hasOneRef. Looks like it could be part of the solution.

> > Should things be turned around, so that HTMLMediaSource has a
> > RefPtrWillBeMember<HTMLMediaElement> and HTMLMediaElement has
> > WeakPtr<HTMLMediaSource>?
> 
> I don't think there ought to be any weak-reference at all here really - the
> current presence of one seems more like a reference-loop hack. But as a
> resource-use optimization I can see how your scheme could be useful.

I didn't look before I typed, now I see that MediaSource already has a
RawPtrWillBeWeakMember<HTMLMediaElement> m_attachedElement.

I also see that MediaSource::hasPendingActivity() always returns true when
attached to a media element, which makes the risk of "this change would prevent
GC as well even if nothing holds a reference to either" very real.

Taking a step back, this is how I think it ought to work when an open
MediaSource is attached to an HTMLMediaElement.

* The media source must keep the media element alive, as the media source can
cause e.g. durationchange events to fire on the media element.
* The media element must keep the media source alive, as seeking the media
element will interact with the media source and potentially be visible in
readyState:
https://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html...
* If neither is referenced by script, both are safe to GC when the media element
is paused or waiting for more data and can fire no more events, not even the
stalled event. That's different from the condition for normal loads.

As written, this is a bit complicated. I think something with hasOneRef() might
work now, but how to make it work with Oilpan?

[fs, 2014-09-11 13:28:33]

On 2014/09/11 12:44:07, philipj wrote:
> On 2014/09/11 11:22:37, fs wrote:
> > On 2014/09/11 10:06:37, philipj wrote:
> > > For MSE, as long as a script has a reference to an open MediaSource which
> > could
> > > append more data, I think preventing GC of the HTMLMediaElement is the
right
> > > thing. However, HTMLMediaElement has a RefPtrWillBeMember<HTMLMediaSource>
> > > m_mediaSource which keep the MediaSource alive, so I don't know how to
> detect
> > > this situation.
> > 
> > m_mediaSource->hasOneRef()? (!OILPAN; If I interpreted the above
correctly...)
> 
> Ah, I didn't know about hasOneRef. Looks like it could be part of the
solution.
> 
> > > Should things be turned around, so that HTMLMediaSource has a
> > > RefPtrWillBeMember<HTMLMediaElement> and HTMLMediaElement has
> > > WeakPtr<HTMLMediaSource>?
> > 
> > I don't think there ought to be any weak-reference at all here really - the
> > current presence of one seems more like a reference-loop hack. But as a
> > resource-use optimization I can see how your scheme could be useful.
> 
> I didn't look before I typed, now I see that MediaSource already has a
> RawPtrWillBeWeakMember<HTMLMediaElement> m_attachedElement.
> 
> I also see that MediaSource::hasPendingActivity() always returns true when
> attached to a media element, which makes the risk of "this change would
prevent
> GC as well even if nothing holds a reference to either" very real.

Yes, that seems more counterproductive than not. Wild guess is that it's related
to the current setup (weak ref to HTMLMediaElement). Preferably the "keep me
alive" predicate for MediaSource should be independent of the reference to
HTMLMediaElement (which, by holding a reference to the MS should provide the
lifetime guarantee.)

> Taking a step back, this is how I think it ought to work when an open
> MediaSource is attached to an HTMLMediaElement.
> 
> * The media source must keep the media element alive, as the media source can
> cause e.g. durationchange events to fire on the media element.
> * The media element must keep the media source alive, as seeking the media
> element will interact with the media source and potentially be visible in
> readyState:
>
https://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html...
> * If neither is referenced by script, both are safe to GC when the media
element
> is paused or waiting for more data and can fire no more events, not even the
> stalled event. That's different from the condition for normal loads.
> 
> As written, this is a bit complicated. I think something with hasOneRef()
might
> work now, but how to make it work with Oilpan?

If it boils down to knowing when HTMLMediaElement has the only reference, I
suppose some form of weak-reference scheme might do the trick. Using non-weak
references would be preferable though. Maybe you want to ask some Oilpan person
what they think about it.

[acolwell GONE FROM CHROMIUM, 2014-09-11 16:33:34]

On 2014/09/11 13:28:33, fs wrote:
> On 2014/09/11 12:44:07, philipj wrote:
> > On 2014/09/11 11:22:37, fs wrote:
> > > On 2014/09/11 10:06:37, philipj wrote:
> > > > For MSE, as long as a script has a reference to an open MediaSource
which
> > > could
> > > > append more data, I think preventing GC of the HTMLMediaElement is the
> right
> > > > thing. However, HTMLMediaElement has a
RefPtrWillBeMember<HTMLMediaSource>
> > > > m_mediaSource which keep the MediaSource alive, so I don't know how to
> > detect
> > > > this situation.
> > > 
> > > m_mediaSource->hasOneRef()? (!OILPAN; If I interpreted the above
> correctly...)
> > 
> > Ah, I didn't know about hasOneRef. Looks like it could be part of the
> solution.
> > 
> > > > Should things be turned around, so that HTMLMediaSource has a
> > > > RefPtrWillBeMember<HTMLMediaElement> and HTMLMediaElement has
> > > > WeakPtr<HTMLMediaSource>?
> > > 
> > > I don't think there ought to be any weak-reference at all here really -
the
> > > current presence of one seems more like a reference-loop hack. But as a
> > > resource-use optimization I can see how your scheme could be useful.
> > 
> > I didn't look before I typed, now I see that MediaSource already has a
> > RawPtrWillBeWeakMember<HTMLMediaElement> m_attachedElement.
> > 
> > I also see that MediaSource::hasPendingActivity() always returns true when
> > attached to a media element, which makes the risk of "this change would
> prevent
> > GC as well even if nothing holds a reference to either" very real.
> 
> Yes, that seems more counterproductive than not. Wild guess is that it's
related
> to the current setup (weak ref to HTMLMediaElement). Preferably the "keep me
> alive" predicate for MediaSource should be independent of the reference to
> HTMLMediaElement (which, by holding a reference to the MS should provide the
> lifetime guarantee.)

The MediaSource needs to be kept alive if it is still attached for the same
reasons that changes here are being made to keep HTMLMediaElement alive. It is
possible for webapp to register event handlers that could be called and not keep
a reference to the MediaSource object. The initial 'sourceopen' event is a
perfect example of this. Also as long as the MediaSource is attached to the
HTMLMediaElement, it needs to keep the MediaSource JS wrapper alive so that any
properties added by the webapp don't disappear.

Unless GC is prevented when a MediaSource is attached, GC will always be
"observable" because of the 'sourceclose' event firing when the MediaSource is
detached from the HTMLMediaElement. Destruction of the HTMLMediaElement
currently causes a detach to occur. Current MediaSource best practices encourage
setting mediaElement.src = '' when you are done with an HTMLMediaElement that is
using MediaSource to force a close and immediately clear the memory holding the
media data instead of waiting for GC. We could simply add m_mediaSource to
HTMLMediaElement::hasPendingActivity() to prevent observable GC.

[acolwell GONE FROM CHROMIUM, 2014-09-11 16:33:41]

https://codereview.chromium.org/552303006/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/552303006/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:396: setShouldDelayLoadEvent(false);
You might be able to remove this and the one below now that
m_shouldDelayLoadEvent being true prevents this object from getting destroyed.

[fs, 2014-09-11 17:27:35]

On 2014/09/11 16:33:34, acolwell_leaving_chromium_9-23 wrote:
> On 2014/09/11 13:28:33, fs wrote:
> > On 2014/09/11 12:44:07, philipj wrote:
> > > On 2014/09/11 11:22:37, fs wrote:
> > > > On 2014/09/11 10:06:37, philipj wrote:
> > > > > For MSE, as long as a script has a reference to an open MediaSource
> which
> > > > could
> > > > > append more data, I think preventing GC of the HTMLMediaElement is the
> > right
> > > > > thing. However, HTMLMediaElement has a
> RefPtrWillBeMember<HTMLMediaSource>
> > > > > m_mediaSource which keep the MediaSource alive, so I don't know how to
> > > detect
> > > > > this situation.
> > > > 
> > > > m_mediaSource->hasOneRef()? (!OILPAN; If I interpreted the above
> > correctly...)
> > > 
> > > Ah, I didn't know about hasOneRef. Looks like it could be part of the
> > solution.
> > > 
> > > > > Should things be turned around, so that HTMLMediaSource has a
> > > > > RefPtrWillBeMember<HTMLMediaElement> and HTMLMediaElement has
> > > > > WeakPtr<HTMLMediaSource>?
> > > > 
> > > > I don't think there ought to be any weak-reference at all here really -
> the
> > > > current presence of one seems more like a reference-loop hack. But as a
> > > > resource-use optimization I can see how your scheme could be useful.
> > > 
> > > I didn't look before I typed, now I see that MediaSource already has a
> > > RawPtrWillBeWeakMember<HTMLMediaElement> m_attachedElement.
> > > 
> > > I also see that MediaSource::hasPendingActivity() always returns true when
> > > attached to a media element, which makes the risk of "this change would
> > prevent
> > > GC as well even if nothing holds a reference to either" very real.
> > 
> > Yes, that seems more counterproductive than not. Wild guess is that it's
> related
> > to the current setup (weak ref to HTMLMediaElement). Preferably the "keep me
> > alive" predicate for MediaSource should be independent of the reference to
> > HTMLMediaElement (which, by holding a reference to the MS should provide the
> > lifetime guarantee.)
> 
> The MediaSource needs to be kept alive if it is still attached for the same
> reasons that changes here are being made to keep HTMLMediaElement alive. It is
> possible for webapp to register event handlers that could be called and not
keep
> a reference to the MediaSource object. The initial 'sourceopen' event is a
> perfect example of this.

(Curious: How do you get a reference to the MediaSource in the 'sourceopen'
handler without using a closure in this case?)

> Also as long as the MediaSource is attached to the
> HTMLMediaElement, it needs to keep the MediaSource JS wrapper alive so that
any
> properties added by the webapp don't disappear.

Ah, the wrapper... (Not quite used to this wrapper business yet.)

> Unless GC is prevented when a MediaSource is attached, GC will always be
> "observable" because of the 'sourceclose' event firing when the MediaSource is
> detached from the HTMLMediaElement. Destruction of the HTMLMediaElement
> currently causes a detach to occur.

Where and which spec is that specified? (I don't see how anything in
https://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html...
could be interpreted like that.)
This makes it sound like registering a 'sourceclose' handler would also require
explicit detach...

> Current MediaSource best practices encourage
> setting mediaElement.src = '' when you are done with an HTMLMediaElement that
is
> using MediaSource to force a close and immediately clear the memory holding
the
> media data instead of waiting for GC.

...like this.

> We could simply add m_mediaSource to HTMLMediaElement::hasPendingActivity() to
prevent observable GC.

SGTM. The bug with observability can be fixed later...

[acolwell GONE FROM CHROMIUM, 2014-09-11 17:51:10]

On 2014/09/11 17:27:35, fs wrote:
> On 2014/09/11 16:33:34, acolwell_leaving_chromium_9-23 wrote:
> > On 2014/09/11 13:28:33, fs wrote:
> > > On 2014/09/11 12:44:07, philipj wrote:
> > > > On 2014/09/11 11:22:37, fs wrote:
> > > > > On 2014/09/11 10:06:37, philipj wrote:
> > > > > > For MSE, as long as a script has a reference to an open MediaSource
> > which
> > > > > could
> > > > > > append more data, I think preventing GC of the HTMLMediaElement is
the
> > > right
> > > > > > thing. However, HTMLMediaElement has a
> > RefPtrWillBeMember<HTMLMediaSource>
> > > > > > m_mediaSource which keep the MediaSource alive, so I don't know how
to
> > > > detect
> > > > > > this situation.
> > > > > 
> > > > > m_mediaSource->hasOneRef()? (!OILPAN; If I interpreted the above
> > > correctly...)
> > > > 
> > > > Ah, I didn't know about hasOneRef. Looks like it could be part of the
> > > solution.
> > > > 
> > > > > > Should things be turned around, so that HTMLMediaSource has a
> > > > > > RefPtrWillBeMember<HTMLMediaElement> and HTMLMediaElement has
> > > > > > WeakPtr<HTMLMediaSource>?
> > > > > 
> > > > > I don't think there ought to be any weak-reference at all here really
-
> > the
> > > > > current presence of one seems more like a reference-loop hack. But as
a
> > > > > resource-use optimization I can see how your scheme could be useful.
> > > > 
> > > > I didn't look before I typed, now I see that MediaSource already has a
> > > > RawPtrWillBeWeakMember<HTMLMediaElement> m_attachedElement.
> > > > 
> > > > I also see that MediaSource::hasPendingActivity() always returns true
when
> > > > attached to a media element, which makes the risk of "this change would
> > > prevent
> > > > GC as well even if nothing holds a reference to either" very real.
> > > 
> > > Yes, that seems more counterproductive than not. Wild guess is that it's
> > related
> > > to the current setup (weak ref to HTMLMediaElement). Preferably the "keep
me
> > > alive" predicate for MediaSource should be independent of the reference to
> > > HTMLMediaElement (which, by holding a reference to the MS should provide
the
> > > lifetime guarantee.)
> > 
> > The MediaSource needs to be kept alive if it is still attached for the same
> > reasons that changes here are being made to keep HTMLMediaElement alive. It
is
> > possible for webapp to register event handlers that could be called and not
> keep
> > a reference to the MediaSource object. The initial 'sourceopen' event is a
> > perfect example of this.
> 
> (Curious: How do you get a reference to the MediaSource in the 'sourceopen'
> handler without using a closure in this case?)

The event.target is the MediaSource object. You don't need to put a reference to
the object in a closure.

> 
> > Also as long as the MediaSource is attached to the
> > HTMLMediaElement, it needs to keep the MediaSource JS wrapper alive so that
> any
> > properties added by the webapp don't disappear.
> 
> Ah, the wrapper... (Not quite used to this wrapper business yet.)
> 
> > Unless GC is prevented when a MediaSource is attached, GC will always be
> > "observable" because of the 'sourceclose' event firing when the MediaSource
is
> > detached from the HTMLMediaElement. Destruction of the HTMLMediaElement
> > currently causes a detach to occur.
> 
> Where and which spec is that specified? (I don't see how anything in
>
https://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html...
> could be interpreted like that.)
> This makes it sound like registering a 'sourceclose' handler would also
require
> explicit detach...

I suppose as long as the HTMLMediaElement doesn't transition to NETWORK_EMPTY on
GC this doesn't technically have to happen. I'd have to look closer at the Blink
code to see what is going on here.

> 
> > Current MediaSource best practices encourage
> > setting mediaElement.src = '' when you are done with an HTMLMediaElement
that
> is
> > using MediaSource to force a close and immediately clear the memory holding
> the
> > media data instead of waiting for GC.
> 
> ...like this.
> 
> > We could simply add m_mediaSource to HTMLMediaElement::hasPendingActivity()
to
> prevent observable GC.
> 
> SGTM. The bug with observability can be fixed later...

Great. Thanks for your comments. :)

[Ken Russell (switch to Gerrit), 2014-09-11 18:20:56]

kbr@chromium.org changed reviewers:
+ kbr@chromium.org

[Ken Russell (switch to Gerrit), 2014-09-11 18:20:57]

Thanks for working to fix this. The code looks good in general, especially the
removal of hacks from the test cases. One question.

https://codereview.chromium.org/552303006/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/552303006/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:3477: if (potentiallyPlaying())
Consider the case where a video is set to loop and then to play. Then a page
navigation occurs and ActiveDOMObject::stop is called against the video element.
Will that cause potentiallyPlaying() to start returning false so that the
HTMLMediaElement will ultimately be GC'd? (Or do ActiveDOMObjects have some
other mechanism to prevent them from leaking once the page that contained them
is destroyed?)

[acolwell GONE FROM CHROMIUM, 2014-09-11 19:02:24]

https://codereview.chromium.org/552303006/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/552303006/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:3477: if (potentiallyPlaying())
On 2014/09/11 18:20:56, Ken Russell wrote:
> Consider the case where a video is set to loop and then to play. Then a page
> navigation occurs and ActiveDOMObject::stop is called against the video
element.
> Will that cause potentiallyPlaying() to start returning false so that the
> HTMLMediaElement will ultimately be GC'd? (Or do ActiveDOMObjects have some
> other mechanism to prevent them from leaking once the page that contained them
> is destroyed?)

I believe it we add an m_readyStateMaximum = HAVE_NOTHING to userCancelledLoad()
near the m_readyState assignment we'll cover most of the cases. I'm still trying
to figure out what the right thing to do is if m_completelyLoaded is true
because that assignment is skipped in that case.

[philipj_slow, 2014-09-11 19:27:38]

On 2014/09/11 16:33:34, acolwell_leaving_chromium_9-23 wrote:
> The MediaSource needs to be kept alive if it is still attached for the same
> reasons that changes here are being made to keep HTMLMediaElement alive. It is
> possible for webapp to register event handlers that could be called and not
keep
> a reference to the MediaSource object. The initial 'sourceopen' event is a
> perfect example of this. Also as long as the MediaSource is attached to the
> HTMLMediaElement, it needs to keep the MediaSource JS wrapper alive so that
any
> properties added by the webapp don't disappear.
> 
> Unless GC is prevented when a MediaSource is attached, GC will always be
> "observable" because of the 'sourceclose' event firing when the MediaSource is
> detached from the HTMLMediaElement. Destruction of the HTMLMediaElement
> currently causes a detach to occur. Current MediaSource best practices
encourage
> setting mediaElement.src = '' when you are done with an HTMLMediaElement that
is
> using MediaSource to force a close and immediately clear the memory holding
the
> media data instead of waiting for GC. We could simply add m_mediaSource to
> HTMLMediaElement::hasPendingActivity() to prevent observable GC.

If the 'sourceclose' event is scheduled due to GC, wouldn't the MediaSource
destructor be run before return to the event loop, so that the event is simply
thrown away?

Concretely, would "if (m_mediaSource) return true" be acceptable here, so that a
connected media element + media source pair will simply not be collected? Maybe
it needs to be "if (m_mediaSource && document().isActive())" so that they can at
least be GC'd when their document dies?

[acolwell GONE FROM CHROMIUM, 2014-09-11 19:32:55]

On 2014/09/11 19:27:38, philipj wrote:
> On 2014/09/11 16:33:34, acolwell_leaving_chromium_9-23 wrote:
> > The MediaSource needs to be kept alive if it is still attached for the same
> > reasons that changes here are being made to keep HTMLMediaElement alive. It
is
> > possible for webapp to register event handlers that could be called and not
> keep
> > a reference to the MediaSource object. The initial 'sourceopen' event is a
> > perfect example of this. Also as long as the MediaSource is attached to the
> > HTMLMediaElement, it needs to keep the MediaSource JS wrapper alive so that
> any
> > properties added by the webapp don't disappear.
> > 
> > Unless GC is prevented when a MediaSource is attached, GC will always be
> > "observable" because of the 'sourceclose' event firing when the MediaSource
is
> > detached from the HTMLMediaElement. Destruction of the HTMLMediaElement
> > currently causes a detach to occur. Current MediaSource best practices
> encourage
> > setting mediaElement.src = '' when you are done with an HTMLMediaElement
that
> is
> > using MediaSource to force a close and immediately clear the memory holding
> the
> > media data instead of waiting for GC. We could simply add m_mediaSource to
> > HTMLMediaElement::hasPendingActivity() to prevent observable GC.
> 
> If the 'sourceclose' event is scheduled due to GC, wouldn't the MediaSource
> destructor be run before return to the event loop, so that the event is simply
> thrown away?
Not if the webapp has a ref to the MediaSource, but not the HTMLMediaElement.
Right now HTMLMediaElement can be GCed w/o GCing the MediaSource object.

> 
> Concretely, would "if (m_mediaSource) return true" be acceptable here, so that
a
> connected media element + media source pair will simply not be collected?
Maybe
> it needs to be "if (m_mediaSource && document().isActive())" so that they can
at
> least be GC'd when their document dies?
Yes. I believe if (m_mediaSource) would be sufficient since I believe
HTMLMediaElement::stop() -> userCancelledLoad() -> clearMediaPlayer() ->
closeMediaSource() makes sure the pointer is null as a result of document
destruction.

[philipj_slow, 2014-09-11 19:36:56]

OK, I'll give "if (m_mediaSource) return true" a try tomorrow, writing the tests
for this is sure to be 90% of the work...

[philipj_slow, 2014-09-12 14:05:21]

https://codereview.chromium.org/552303006/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/552303006/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:3477: if (potentiallyPlaying())
On 2014/09/11 19:02:24, acolwell_leaving_chromium_9-23 wrote:
> On 2014/09/11 18:20:56, Ken Russell wrote:
> > Consider the case where a video is set to loop and then to play. Then a page
> > navigation occurs and ActiveDOMObject::stop is called against the video
> element.
> > Will that cause potentiallyPlaying() to start returning false so that the
> > HTMLMediaElement will ultimately be GC'd? (Or do ActiveDOMObjects have some
> > other mechanism to prevent them from leaking once the page that contained
them
> > is destroyed?)
> 
> I believe it we add an m_readyStateMaximum = HAVE_NOTHING to
userCancelledLoad()
> near the m_readyState assignment we'll cover most of the cases. I'm still
trying
> to figure out what the right thing to do is if m_completelyLoaded is true
> because that assignment is skipped in that case.

I'll add "if (!document().isActive()) return false" at the top of
hasPendingActivity() as a catch-all for this kind of thing. Sound OK?

[philipj_slow, 2014-09-12 14:14:54]

cleanup

[philipj_slow, 2014-09-12 14:15:35]

OK, I think PS3 may just work. PTAL.

[acolwell GONE FROM CHROMIUM, 2014-09-12 16:27:26]

lgtm

https://codereview.chromium.org/552303006/diff/40001/LayoutTests/media/gc-whi...
File LayoutTests/media/gc-while-playing.html (right):

https://codereview.chromium.org/552303006/diff/40001/LayoutTests/media/gc-whi...
LayoutTests/media/gc-while-playing.html:17:
setTimeout(t.step_func(gcAndAwaitTimeupdate), 0);
Isn't the reference to 'a' in gcAndAwaitTimeupdate() also keeping the media
element around? Please fix the comment or code so that they are consistent.

https://codereview.chromium.org/552303006/diff/40001/LayoutTests/media/gc-whi...
File LayoutTests/media/gc-while-seeking.html (right):

https://codereview.chromium.org/552303006/diff/40001/LayoutTests/media/gc-whi...
LayoutTests/media/gc-while-seeking.html:16: // GC async since the event target
is the media element.
ditto

[scherkus (not reviewing), 2014-09-12 19:05:01]

lgtm -- thanks for tackling this philipj!

https://codereview.chromium.org/552303006/diff/40001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/552303006/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:3456: bool
HTMLMediaElement::hasPendingActivity() const
nice!

[philipj_slow, 2014-09-12 19:11:06]

update comments

[philipj_slow, 2014-09-12 19:13:00]

Thanks all, I'll try landing this now.

https://codereview.chromium.org/552303006/diff/40001/LayoutTests/media/gc-whi...
File LayoutTests/media/gc-while-playing.html (right):

https://codereview.chromium.org/552303006/diff/40001/LayoutTests/media/gc-whi...
LayoutTests/media/gc-while-playing.html:17:
setTimeout(t.step_func(gcAndAwaitTimeupdate), 0);
On 2014/09/12 16:27:25, acolwell_leaving_chromium_9-23 wrote:
> Isn't the reference to 'a' in gcAndAwaitTimeupdate() also keeping the media
> element around? Please fix the comment or code so that they are consistent.

I've updated the comments to be accurate, hopefully.

[philipj_slow, 2014-09-12 19:56:50]

Oops, I didn't notice that the trybots had caught a real regression: the new
ASSERT(!m_shouldDelayLoadEvent) was failing in
http/tests/security/video-poster-cross-origin-crash2.html

#0  0x00000000028f1e2e in blink::HTMLMediaElement::~HTMLMediaElement
(this=0xcb266a2c010, __in_chrg=<optimized out>) at
../../third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:384
#1  0x0000000002a566f1 in blink::HTMLVideoElement::~HTMLVideoElement
(this=0xcb266a2c010, __in_chrg=<optimized out>) at
../../third_party/WebKit/Source/core/html/HTMLVideoElement.h:49
#2  0x0000000002a56744 in blink::HTMLVideoElement::~HTMLVideoElement
(this=0xcb266a2c010, __in_chrg=<optimized out>) at
../../third_party/WebKit/Source/core/html/HTMLVideoElement.h:49
#3  0x0000000002600abe in
blink::ContainerNode::removeDetachedChildrenInContainer (this=0xcb266a102b8,
container=...) at ../../third_party/WebKit/Source/core/dom/ContainerNode.cpp:469
#4  0x00000000025ff018 in blink::ContainerNode::removeDetachedChildren
(this=0xcb266a102b8) at
../../third_party/WebKit/Source/core/dom/ContainerNode.cpp:84
#5  0x00000000025ff18e in blink::ContainerNode::~ContainerNode
(this=0xcb266a102b8, __in_chrg=<optimized out>) at
../../third_party/WebKit/Source/core/dom/ContainerNode.cpp:102
#6  0x000000000266ab55 in blink::Element::~Element (this=0xcb266a102b8,
__in_chrg=<optimized out>) at
../../third_party/WebKit/Source/core/dom/Element.cpp:148
#7  0x000000000273acf2 in blink::HTMLElement::~HTMLElement (this=0xcb266a102b8,
__in_chrg=<optimized out>) at
../../third_party/WebKit/Source/core/html/HTMLElement.h:42
#8  0x00000000029c2264 in blink::HTMLDivElement::~HTMLDivElement
(this=0xcb266a102b8, __in_chrg=<optimized out>) at
../../third_party/WebKit/Source/core/html/HTMLDivElement.h:30
#9  0x00000000029c2294 in blink::HTMLDivElement::~HTMLDivElement
(this=0xcb266a102b8, __in_chrg=<optimized out>) at
../../third_party/WebKit/Source/core/html/HTMLDivElement.h:30
#10 0x00000000026b3aac in blink::Node::removedLastRef (this=0xcb266a102b8) at
../../third_party/WebKit/Source/core/dom/Node.cpp:2310
#11 0x000000000253b077 in blink::TreeShared<blink::Node>::deref
(this=0xcb266a102c8) at ../../third_party/WebKit/Source/core/dom/TreeShared.h:82
#12 0x000000000253b91c in WTF::derefIfNotNull<blink::Node> (ptr=0xcb266a102b8)
at ../../third_party/WebKit/Source/wtf/PassRefPtr.h:57
#13 0x000000000253b0a7 in WTF::RefPtr<blink::Node>::~RefPtr
(this=0x7fff96c75028, __in_chrg=<optimized out>) at
../../third_party/WebKit/Source/wtf/RefPtr.h:59
#14 0x000000000260bdcb in WTF::VectorDestructor<true, WTF::RefPtr<blink::Node>
>::destruct (begin=0x7fff96c75020, end=0x7fff96c75038) at
../../third_party/WebKit/Source/wtf/Vector.h:64
#15 0x000000000260b28c in WTF::VectorTypeOperations<WTF::RefPtr<blink::Node>
>::destruct (begin=0x7fff96c75020, end=0x7fff96c75038) at
../../third_party/WebKit/Source/wtf/Vector.h:247
#16 0x000000000260a273 in WTF::Vector<WTF::RefPtr<blink::Node>, 11ul,
WTF::DefaultAllocator>::finalize (this=0x7fff96c75010) at
../../third_party/WebKit/Source/wtf/Vector.h:596
#17 0x00000000026090e2 in
WTF::VectorDestructorBase<WTF::Vector<WTF::RefPtr<blink::Node>, 11ul,
WTF::DefaultAllocator>, WTF::RefPtr<blink::Node>, true,
false>::~VectorDestructorBase (
    this=0x7fff96c75010, __in_chrg=<optimized out>) at
../../third_party/WebKit/Source/wtf/Vector.h:519
#18 0x000000000260821a in WTF::Vector<WTF::RefPtr<blink::Node>, 11ul,
WTF::DefaultAllocator>::~Vector (this=0x7fff96c75010, __in_chrg=<optimized out>)
at ../../third_party/WebKit/Source/wtf/Vector.h:548
#19 0x0000000002601738 in blink::ContainerNode::removeChildren
(this=0xcb266a10230) at
../../third_party/WebKit/Source/core/dom/ContainerNode.cpp:652
#20 0x0000000002cc7a7e in blink::replaceChildrenWithFragment
(container=0xcb266a10230, fragment=..., exceptionState=...) at
../../third_party/WebKit/Source/core/editing/markup.cpp:1019
#21 0x00000000026731c4 in blink::Element::setInnerHTML (this=0xcb266a10230,
html=..., exceptionState=...) at
../../third_party/WebKit/Source/core/dom/Element.cpp:2180
#22 0x00000000034ef02a in blink::ElementV8Internal::innerHTMLAttributeSetter
(v8Value=..., info=...) at gen/blink/bindings/core/v8/V8Element.cpp:458
#23 0x00000000034ef0d2 in
blink::ElementV8Internal::innerHTMLAttributeSetterCallback (v8Value=...,
info=...) at gen/blink/bindings/core/v8/V8Element.cpp:466
#24 0x000000000243cee8 in v8::internal::PropertyCallbackArguments::Call
(this=this@entry=0x7fff96c75340, 
    f=f@entry=0x34ef093
<blink::ElementV8Internal::innerHTMLAttributeSetterCallback(v8::Local<v8::String>,
v8::Local<v8::Value>, v8::PropertyCallbackInfo<void> const&)>,
arg1=arg1@entry=..., arg2=..., 
    arg2@entry=...) at ../../v8/src/arguments.cc:89

In this test there are no references to the video element outside of the DOM, so
it dies synchronously with its parent, hasPendingActivity() isn't part of the
picture.

The assert isn't hit with Oilpan enabled because
ContainerNode::removeDetachedChildrenInContainer is guarded by #if
!ENABLE(OILPAN). I believe that GC is the only way for HTMLMediaElement to die
with Oilpan, so I'll keep the assert for Oilpan and revert to the old state for
non-Oilpan.

[philipj_slow, 2014-09-12 20:00:41]

assert only for oilpan

[philipj_slow, 2014-09-12 20:03:05]

philipj@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[philipj_slow, 2014-09-12 20:03:06]

Oilpan reviewers, would one of you mind taking a look at the change to the
HTMLMediaElement destructor? My assumption is that with Oilpan, the destructor
will never be called while hasPendingActivity() returns true.

[Ken Russell (switch to Gerrit), 2014-09-12 23:52:39]

Philip: thanks for working on this. I'm afraid we've found a problem with your
patch; it doesn't actually fix the problem for the test case in Issue 407976.

To reproduce, please build Chrome in Release mode with the GYP_DEFINE
blink_logging_always_on=1 set, and run (from within src/):

./content/test/gpu/run_gpu_test.py webgl_conformance --browser=release
--page-filter=conformance_textures_texture_upload_size
"--extra-browser-args=--enable-logging=stderr
--blink-platform-log-channels=Timers,Media,ScriptedAnimationController
--vmodule=thread_proxy=2,render_widget_compositor=2
--send-v8-idle-notification-after-commit" --show-stdout

The test will time out very reliably -- more reliably than it currently is. The
symptom we're seeing here is that the third video (the Ogg Theora version) is
GC'd prematurely. If you instrument HTMLMediaElement::hasPendingActivity you'll
see that potentiallyPlaying() returns false -- this seems wrong because at the
point the video element has gone out of scope in
src/third_party/webgl/src/sdk/tests/conformance/textures/texture-upload-size.html
, play() has already been called against it.

Again, the situation is that the network state has transitioned to "idle" when
the GC occurs, but playback has not started yet.

[sof, 2014-09-13 08:43:51]

On 2014/09/12 20:03:06, philipj wrote:
> Oilpan reviewers, would one of you mind taking a look at the change to the
> HTMLMediaElement destructor? My assumption is that with Oilpan, the destructor
> will never be called while hasPendingActivity() returns true.

That's a valid assumption (and the purpose of hasPendingActivity()). No longer
updating the delayed count in the destructor with the hasPendingActivity()
definition provided here is consistent, but other Oilpan folks poring over this
particular part should (also) approve. I would prefer it if some of the careful
lifetime considerations were kept in comment form though.

Re: hasPendingActivity() - is the reasoning for making it that 'strong' (i.e.,
dependent on the lifetime state of its document) clearly explained somewhere? (
It wouldn't be amiss to document where the different conditions stems from in
that predicate.. )

[haraken, 2014-09-15 05:02:13]

On 2014/09/13 08:43:51, sof wrote:
> On 2014/09/12 20:03:06, philipj wrote:
> > Oilpan reviewers, would one of you mind taking a look at the change to the
> > HTMLMediaElement destructor? My assumption is that with Oilpan, the
destructor
> > will never be called while hasPendingActivity() returns true.
> 
> That's a valid assumption (and the purpose of hasPendingActivity()). No longer
> updating the delayed count in the destructor with the hasPendingActivity()
> definition provided here is consistent, but other Oilpan folks poring over
this
> particular part should (also) approve. I would prefer it if some of the
careful
> lifetime considerations were kept in comment form though.
> 
> Re: hasPendingActivity() - is the reasoning for making it that 'strong' (i.e.,
> dependent on the lifetime state of its document) clearly explained somewhere?
(
> It wouldn't be amiss to document where the different conditions stems from in
> that predicate.. )

I think this change looks good.

Just to confirm: hasPendingActivity() is called only when the HTMLMediaElement
has a wrapper. If the wrapper is gone, hasPendingActivity() is never called. So
this change is correct if the wrapper of the HTMLMediaElement is guaranteed to
be kept alive until the HTMLMediaElement gets destructed. This assumption is
correct, right?

[Mads Ager (chromium), 2014-09-15 07:01:17]

ager@chromium.org changed reviewers:
+ ager@chromium.org

[Mads Ager (chromium), 2014-09-15 07:01:18]

https://codereview.chromium.org/552303006/diff/80001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/552303006/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:385: // If the HTMLMediaElement dies with
the document we are not
From an Oilpan perspective it is really nice to get rid of this!

One verifying question: loading on an HTMLMediaElement can only be started from
JavaScript? So we know that there is a JavaScript wrapper for the
HTMLMediaElement when m_shouldDelayLoadEvent is set to true? If that is the
case, this looks correct to me. If there is a wrapper when loading starts the
hasPendingActivity check will keep the wrapper (and therefore the
HTMLMediaElement) alive until the load is completed.

https://codereview.chromium.org/552303006/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:403: setShouldDelayLoadEvent(false);
This should be ASSERT(!m_shouldDelayLoadEvent) as well, right?

The same reasoning should apply to the non-oilpan code.

[philipj_slow, 2014-09-15 12:49:58]

On 2014/09/12 23:52:39, Ken Russell wrote:
> Philip: thanks for working on this. I'm afraid we've found a problem with your
> patch; it doesn't actually fix the problem for the test case in Issue 407976.

Thanks for pointing out that little detail, I'm looking into this.

[philipj_slow, 2014-09-15 13:07:44]

It seems like my understanding of wrapper objects was wrong and that I've made a
mistake with the Oilpan change. If hasPendingActivity() will only be called if
there is a wrapper object, then the assert I've added may not hold, even though
it does when running LayoutTests/media/ and LayoutTetsts/http/tests/media/.

I think that in order to test this case, one would need a video element with the
delaying-the-load-event flag set that doesn't have a wrapper, which is then
removed from the DOM and allowed to be GC'd.

There's a pretty narrow window in which the delaying-the-load-event flag is set,
so it would have to be a slow-loading video which doesn't reach HAVE_METADATA.
The slow-loading tests I've looked at access the video element using scripts,
which would create the wrapper object.

I'll try to add such a test to see if my assert fails, and if so I'll revert the
destructor to its original form.

https://codereview.chromium.org/552303006/diff/80001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/552303006/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:385: // If the HTMLMediaElement dies with
the document we are not
On 2014/09/15 07:01:18, Mads Ager (chromium) wrote:
> From an Oilpan perspective it is really nice to get rid of this!
> 
> One verifying question: loading on an HTMLMediaElement can only be started
from
> JavaScript? So we know that there is a JavaScript wrapper for the
> HTMLMediaElement when m_shouldDelayLoadEvent is set to true? If that is the
> case, this looks correct to me. If there is a wrapper when loading starts the
> hasPendingActivity check will keep the wrapper (and therefore the
> HTMLMediaElement) alive until the load is completed.

No, loading can start without a wrapper, all you need for that to happen is
<video src="video.webm"> and no JavaScript. (m_shouldDelayLoadEvent corresponds
to the spec's delaying-the-load-event flag, which can be triggered by the HTML
parser inserting elements.)

https://codereview.chromium.org/552303006/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:403: setShouldDelayLoadEvent(false);
On 2014/09/15 07:01:18, Mads Ager (chromium) wrote:
> This should be ASSERT(!m_shouldDelayLoadEvent) as well, right?
> 
> The same reasoning should apply to the non-oilpan code.

That was the previous CL, and the assert failed as described in
https://codereview.chromium.org/552303006/#msg23

[Ken Russell (switch to Gerrit), 2014-09-16 15:49:16]

On 2014/09/15 12:49:58, philipj wrote:
> On 2014/09/12 23:52:39, Ken Russell wrote:
> > Philip: thanks for working on this. I'm afraid we've found a problem with
your
> > patch; it doesn't actually fix the problem for the test case in Issue
407976.
> 
> Thanks for pointing out that little detail, I'm looking into this.

Any update on this? It seems to me that the bug is in potentiallyPlaying(). In
the case in Issue 407976, play() has been called, the network state is idle,
playback hasn't started or stopped, and no errors have occurred. The element
should consider itself potentially playing in this case.

There's some urgent work for Chrome 39 -- see
https://codereview.chromium.org/566733004/ -- that's blocked on this fix. We'd
appreciate it if attention could be paid to this soon. Thanks.

[philipj_slow, 2014-09-16 19:00:50]

I worked on this yesterday, but my workstation doesn't have the drivers to
support WebGL (last I tried to install them things went badly) so I can't run
the tests directly as in comment #27. Instead I tried to minimize the test case
in the blink to something that doesn't involve WebGL, but I never got to
anything that would exhibit any problems. Today I've only been doing reviews,
since people are waiting for feedback.

I didn't realize anything more serious than a flaky test was blocking on this. I
don't quite understand the connection to
https://codereview.chromium.org/566733004/ but is not disabling the test until
it's non-flaky an option? Or does Chromium-side tests not have any equivalent to
TestExpectations?

In any event, the most likely problem here sounds like potentiallyPlaying().
It's pretty much bedtime here, but if you could run an experiment for me that
might help. If you can catch the HTMLMediaElement in a debugger when it's in the
state where hasPendingActivitty() erroneously returns false, can you dump out
*this, i.e. all member variables? If you can't get in in a debugger but can add
logging, knowing these would be useful:
potentiallyPlaying(), to verify that this is indeed the root cause.
m_readyStateMaximum
m_readyState
m_paused
endedPlayback()
stoppedDueToErrors()
couldPlayIfEnoughData()
isBlockedOnMediaController() (almost certainly false, but just to verify)

And, for whichever of these that's causing potentiallyPlaying() to return false,
which combination of state is causing it?

Note that the gc-while-playing.html test is already for the case where
networkState is idle and paused is false, so there has to be some additional
state here. I don't understand what "playback hasn't started or stopped" means
in your description, because if networkState is idle because the resource is
fully loaded and paused is false, how could it not be playing?

If none of this leads to the answer, I'll try installing the nvidia drivers
again...

[scherkus (not reviewing), 2014-09-16 19:03:31]

On 2014/09/16 15:49:16, Ken Russell wrote:
> On 2014/09/15 12:49:58, philipj wrote:
> > On 2014/09/12 23:52:39, Ken Russell wrote:
> > > Philip: thanks for working on this. I'm afraid we've found a problem with
> your
> > > patch; it doesn't actually fix the problem for the test case in Issue
> 407976.
> > 
> > Thanks for pointing out that little detail, I'm looking into this.
> 
> Any update on this? It seems to me that the bug is in potentiallyPlaying(). In
> the case in Issue 407976, play() has been called, the network state is idle,
> playback hasn't started or stopped, and no errors have occurred. The element
> should consider itself potentially playing in this case.
> 
> There's some urgent work for Chrome 39 -- see
> https://codereview.chromium.org/566733004/ -- that's blocked on this fix. We'd
> appreciate it if attention could be paid to this soon. Thanks.

kbr: does that CL really need to wait? it's not exactly clear to me that that CL
is making this issue any work -- it could just be bad luck with flakes (again,
I'm not a V8 expert here so feel free to enlighten me!)

[scherkus (not reviewing), 2014-09-16 19:03:45]

On 2014/09/16 19:03:31, scherkus wrote:
> On 2014/09/16 15:49:16, Ken Russell wrote:
> > On 2014/09/15 12:49:58, philipj wrote:
> > > On 2014/09/12 23:52:39, Ken Russell wrote:
> > > > Philip: thanks for working on this. I'm afraid we've found a problem
with
> > your
> > > > patch; it doesn't actually fix the problem for the test case in Issue
> > 407976.
> > > 
> > > Thanks for pointing out that little detail, I'm looking into this.
> > 
> > Any update on this? It seems to me that the bug is in potentiallyPlaying().
In
> > the case in Issue 407976, play() has been called, the network state is idle,
> > playback hasn't started or stopped, and no errors have occurred. The element
> > should consider itself potentially playing in this case.
> > 
> > There's some urgent work for Chrome 39 -- see
> > https://codereview.chromium.org/566733004/ -- that's blocked on this fix.
We'd
> > appreciate it if attention could be paid to this soon. Thanks.
> 
> kbr: does that CL really need to wait? it's not exactly clear to me that that
CL
> is making this issue any work -- it could just be bad luck with flakes (again,
> I'm not a V8 expert here so feel free to enlighten me!)

any worse*

[philipj_slow, 2014-09-17 14:39:42]

OK, I successfully installed the drivers needed for WebGL and was able to
reproduce the problem in comment #27, although only something like once every
10-100 runs of the test. I added logging to the destructor and found:

HTMLMediaElement::~HTMLMediaElement(0x1d21a028c408)
hasPendingActivity()=0
document().isActive()=1
paused()=0
potentiallyPlaying()=0
m_readyStateMaximum=0
m_readyState=1
m_networkState=1
pausedToBuffer=0
couldPlayIfEnoughData()=1
endedPlayback()=0
stoppedDueToErrors()=0
isBlockedOnMediaController()=0
isBlocked()=1

So potentiallyPlaying() is false and that's why hasPendingActivity() returns
false.

Apparently potentiallyPlaying() isn't implemented per spec, which says:

"A media element is said to be potentially playing when its paused attribute is
false, the element has not ended playback, playback has not stopped due to
errors, the element either has no current media controller or has a current
media controller but is not blocked on its media controller, and the element is
not a blocked media element."

Our implementation instead does things with pausedToBuffer. However, even
implementing the spec, it would return false because isBlocked() is true. On
that the spec says:

"A media element is a blocked media element if its readyState attribute is in
the HAVE_NOTHING state, the HAVE_METADATA state, or the HAVE_CURRENT_DATA state,
or if the element has paused for user interaction or paused for in-band
content."

That seems peculiar, because AFAICT elements which are "potentially playing" per
spec would have to be actually playing, since they have the data they need and
nothing is stopping them from playing.

That's all for today, maybe I'll file a spec bug tonight and maybe in the
interim we simply shouldn't use the potentially playing concept for determining
GC behavior.

[philipj_slow, 2014-09-17 14:50:34]

Here's a WIP spec sync for potentiallyPlaying():
https://codereview.chromium.org/576073005

That doesn't pass the existing tests or fix the problem here, but illustrates
the differences compared to the spec, which has no notion of "maximum readyState
reached since load()" which is what m_readyStateMaximum amounts to.

[philipj_slow, 2014-09-18 08:35:01]

revert oilpan misunderstanding

[philipj_slow, 2014-09-18 08:36:21]

potentiallyPlaying() -> couldPlayIfEnoughData()

[philipj_slow, 2014-09-18 09:19:21]

On 2014/09/13 08:43:51, sof wrote:
> Re: hasPendingActivity() - is the reasoning for making it that 'strong' (i.e.,
> dependent on the lifetime state of its document) clearly explained somewhere?
(
> It wouldn't be amiss to document where the different conditions stems from in
> that predicate.. )

I added the document().isActive() check in response to comment #11, "the case
where a video is set to loop and then to play" followed by page navigation. From
what I can tell HTMLMediaElement::stop() almost guarantees that
hasPendingActivity() will return false, but not quite, because m_seeking is
never reset. One could make it so and ASSERT(!hasPendingActivity()) at the end
of stop(), but checking the document state directly seemed more direct. Is it
possible for scripts in an active document to have references to a media element
in a non-active document? I suspect that it is, but stop() is written as if it's
the end of the line, by calling m_asyncEventQueue->close() the element is never
going behave normally again.

[philipj_slow, 2014-09-18 09:41:04]

documentation

[philipj_slow, 2014-09-18 09:41:38]

documentation

[philipj_slow, 2014-09-18 09:51:52]

I see that additionally, HTMLMediaElement::stop() doesn't set m_paused to true,
so couldPlayIfEnoughData() may continue to return true.

I've documented the document().isActive() bit and the rest to hopefully be less
mysterious, any feedback appreciated.

[sof, 2014-09-18 10:58:02]

On 2014/09/18 09:19:21, philipj wrote:
> On 2014/09/13 08:43:51, sof wrote:
> > Re: hasPendingActivity() - is the reasoning for making it that 'strong'
(i.e.,
> > dependent on the lifetime state of its document) clearly explained
somewhere?
> (
> > It wouldn't be amiss to document where the different conditions stems from
in
> > that predicate.. )
> 
> I added the document().isActive() check in response to comment #11, "the case
> where a video is set to loop and then to play" followed by page navigation.
From
> what I can tell HTMLMediaElement::stop() almost guarantees that
> hasPendingActivity() will return false, but not quite, because m_seeking is
> never reset. One could make it so and ASSERT(!hasPendingActivity()) at the end
> of stop(), but checking the document state directly seemed more direct. Is it
> possible for scripts in an active document to have references to a media
element
> in a non-active document? 

Thanks for the clarifications; comments looks ok.

Re: the last question - I would expect there to be some media test coverage of
cross-frame uses and removing the (i)frame holding a media element..? But your
cross-frame example begs the question of where that !isActive() check should be
performed within this predicate -- i.e., if you have audio playing and you
detach the media element's document&frame, shouldn't the audio-backed element
remain alive if you hold onto it from a script? Will it now?

[jochen (gone - plz use gerrit), 2014-09-18 11:00:38]

On 2014/09/18 at 10:58:02, sigbjornf wrote:
> On 2014/09/18 09:19:21, philipj wrote:
> > On 2014/09/13 08:43:51, sof wrote:
> > > Re: hasPendingActivity() - is the reasoning for making it that 'strong'
(i.e.,
> > > dependent on the lifetime state of its document) clearly explained
somewhere?
> > (
> > > It wouldn't be amiss to document where the different conditions stems from
in
> > > that predicate.. )
> > 
> > I added the document().isActive() check in response to comment #11, "the
case
> > where a video is set to loop and then to play" followed by page navigation.
From
> > what I can tell HTMLMediaElement::stop() almost guarantees that
> > hasPendingActivity() will return false, but not quite, because m_seeking is
> > never reset. One could make it so and ASSERT(!hasPendingActivity()) at the
end
> > of stop(), but checking the document state directly seemed more direct. Is
it
> > possible for scripts in an active document to have references to a media
element
> > in a non-active document? 
> 
> Thanks for the clarifications; comments looks ok.
> 
> Re: the last question - I would expect there to be some media test coverage of
cross-frame uses and removing the (i)frame holding a media element..? But your
cross-frame example begs the question of where that !isActive() check should be
performed within this predicate -- i.e., if you have audio playing and you
detach the media element's document&frame, shouldn't the audio-backed element
remain alive if you hold onto it from a script? Will it now?

if you hold onto a dom element from script, it'll always survive

did you confirm that this fixes the webgl test?

[philipj_slow, 2014-09-18 11:12:48]

On 2014/09/18 11:00:38, jochen wrote:
> did you confirm that this fixes the webgl test?

Yes. It wasn't easily reproducible before, but running the test in a loop I
would usually get it to time out within a minute or so. After the fix I let it
run for maybe 10 minutes without incident. Given the state listed in comment #38
I'm pretty sure that potentiallyPlaying() was indeed the culprit, and
couldPlayIfEnoughData() was correctly returning true in the log output.

[jochen (gone - plz use gerrit), 2014-09-18 11:17:52]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2014-09-18 11:17:53]

lgtm then

thanks for fixing

[philipj_slow, 2014-09-18 11:43:53]

On 2014/09/18 10:58:02, sof wrote:
> On 2014/09/18 09:19:21, philipj wrote:
> > On 2014/09/13 08:43:51, sof wrote:
> > > Re: hasPendingActivity() - is the reasoning for making it that 'strong'
> (i.e.,
> > > dependent on the lifetime state of its document) clearly explained
> somewhere?
> > (
> > > It wouldn't be amiss to document where the different conditions stems from
> in
> > > that predicate.. )
> > 
> > I added the document().isActive() check in response to comment #11, "the
case
> > where a video is set to loop and then to play" followed by page navigation.
> From
> > what I can tell HTMLMediaElement::stop() almost guarantees that
> > hasPendingActivity() will return false, but not quite, because m_seeking is
> > never reset. One could make it so and ASSERT(!hasPendingActivity()) at the
end
> > of stop(), but checking the document state directly seemed more direct. Is
it
> > possible for scripts in an active document to have references to a media
> element
> > in a non-active document? 
> 
> Thanks for the clarifications; comments looks ok.
> 
> Re: the last question - I would expect there to be some media test coverage of
> cross-frame uses and removing the (i)frame holding a media element..? But your
> cross-frame example begs the question of where that !isActive() check should
be
> performed within this predicate -- i.e., if you have audio playing and you
> detach the media element's document&frame, shouldn't the audio-backed element
> remain alive if you hold onto it from a script? Will it now?

This actually doesn't work before or after this CL. Try this:

<!DOCTYPE html>
<iframe></iframe>
<script>
var doc = document.querySelector("iframe").contentDocument;
var audio = doc.createElement("audio");
doc.body.appendChild(audio);
audio.src = "ACDC_-_Back_In_Black-sample.ogg";
audio.loop = true;
audio.play();
setTimeout(function() { document.querySelector("iframe").remove(); }, 1000);
</script>

As soon as the iframe is removed, the audio will stop playing and the audio
element is permanently incapacitated. audio.paused will be false, but it won't
play, and it's not going to fire any more events.

This is not per spec and the element is in an odd state, but as long as things
are this way I think that the document().isActive() check in
hasPendingActivity() makes sense.

[philipj_slow, 2014-09-18 12:10:43]

Before landing I want to acknowledge a few things that I could have done but did
not:

- I didn't add a test that would trigger the probably-invalid oilpan-only assert
discussed in comment #33, I'm lazy and it doesn't seem worth the effort just to
verify that the reverted code was wrong.

- I didn't add a test that would catch the distinction between
potentiallyPlaying() and couldPlayIfEnoughData() in hasPendingActivity(), as
that would require running out of buffered data and the video-throttled-load.cgi
isn't a very exact tool. This would be more suitable for unit testing if there
was such a thing for HTMLMediaElement :/

- I didn't come up with a fix for the "media element + media source pair will
now live as long as the document" leak. Fixing that after Oilpan should be a
simple matter of letting MediaSource have a Member<HTMLMediaElement> as opposed
to the current weak ptr. Fixing it without Oilpan would be uglier, probably
involving hasOneRef(), so I'm hoping that by the time the problem is found in
the wild we'll be Oilpan only and can fix it then.

If the bots are happy I'm going to land this to unblock
https://crbug.com/414815, but if anyone thinks that any of the above or anything
else should have been done in this CL, just let me know and I'll try to do
resolve it with a follow-up.

[philipj_slow, 2014-09-18 13:19:41]

make gc-while-seeking.html non-flaky

[Ken Russell (switch to Gerrit), 2014-09-18 13:32:25]

LGTM. Thank you for fixing this thoroughly, and adding the new test cases.

[philipj_slow, 2014-09-18 14:06:48]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-09-18 14:07:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/552303006/170001

[commit-bot: I haz the power, 2014-09-18 15:55:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-18 15:55:22]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/27707)

[acolwell GONE FROM CHROMIUM, 2014-09-18 15:56:49]

lgtm. Thanks for working on this.

[Ken Russell (switch to Gerrit), 2014-09-18 15:58:09]

On 2014/09/18 15:55:22, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   win_blink_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/27707)

Guessing those failures are not related to this patch. CQ'ing again.

[Ken Russell (switch to Gerrit), 2014-09-18 15:58:16]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2014-09-18 15:58:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/552303006/170001

[commit-bot: I haz the power, 2014-09-18 16:31:32]

Committed patchset #10 (id:170001) as 182247

[Dirk Pranke, 2014-09-18 18:05:54]

On 2014/09/18 16:31:32, I haz the power (commit-bot) wrote:
> Committed patchset #10 (id:170001) as 182247

Looks like gc-while-playing is failing or flaking on at least the Mac 10.9 bot:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpec...

I won't revert this just yet, on the off chance that you're still around to see
this and/or we can see where else it might fail to get more data.

[Dirk Pranke, 2014-09-18 18:27:17]

On 2014/09/18 18:05:54, Dirk Pranke wrote:
> On 2014/09/18 16:31:32, I haz the power (commit-bot) wrote:
> > Committed patchset #10 (id:170001) as 182247
> 
> Looks like gc-while-playing is failing or flaking on at least the Mac 10.9
bot:
> 
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpec...
> 
> I won't revert this just yet, on the off chance that you're still around to
see
> this and/or we can see where else it might fail to get more data.

I've suppressed the failure for now (r182255), so you can investigate (per
jochen@'s request). Looks like it might just be 10.9-only.

[philipj_slow, 2014-09-18 18:50:38]

Thanks Dirk. The failure was this:
This is a testharness.js-based test.
FAIL GC while playing assert_greater_than: expected a number greater than 0 but
got 0
Harness: the test ran to completion.

In other words this failed:
assert_greater_than(e.target.currentTime, 0);

That makes me suspect https://codereview.chromium.org/578113002/ which was also
10.9 and a problem of too many timeupdate events...

I'll try to verify locally, but given how slow a build is maybe that CL will
land first :)

[Dirk Pranke, 2014-09-18 19:07:34]

On 2014/09/18 18:50:38, philipj wrote:
> Thanks Dirk. The failure was this:
> This is a testharness.js-based test.
> FAIL GC while playing assert_greater_than: expected a number greater than 0
but
> got 0
> Harness: the test ran to completion.
> 
> In other words this failed:
> assert_greater_than(e.target.currentTime, 0);
> 
> That makes me suspect https://codereview.chromium.org/578113002/ which was
also
> 10.9 and a problem of too many timeupdate events...
> 
> I'll try to verify locally, but given how slow a build is maybe that CL will
> land first :)

Looks like we may also be seeing some crashes on the dbg bots:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=und...

[jochen (gone - plz use gerrit), 2014-09-18 19:22:50]

that's the assertion: ASSERTION FAILED: !m_asyncEventQueue->hasPendingEvents()

guess it's wrong after all. Philip, should we just remove it?

[Dirk Pranke, 2014-09-18 20:25:27]

On 2014/09/18 19:22:50, jochen wrote:
> that's the assertion: ASSERTION FAILED: !m_asyncEventQueue->hasPendingEvents()
> 
> guess it's wrong after all. Philip, should we just remove it?

Unless I hear something within, say, the next 30 minutes, I'll revert this
change ...

[philipj_slow, 2014-09-18 21:05:47]

Hmm, if that assert is failing then something is indeed not as I expected. It's
close to bedtime for me, so if you want to revert it I can investigate tomorrow.

[philipj_slow, 2014-09-18 21:05:47]

Hmm, if that assert is failing then something is indeed not as I expected. It's
close to bedtime for me, so if you want to revert it I can investigate tomorrow.