Bug 66878 - HTMLAudioElement can be garbage collected while it playing
Summary: HTMLAudioElement can be garbage collected while it playing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 420+
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 69792
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-24 12:15 PDT by Eugene Nalimov
Modified: 2011-10-20 11:25 PDT (History)
14 users (show)

See Also:


Attachments
Patch (11.80 KB, patch)
2011-09-27 10:24 PDT, Eugene Nalimov
no flags Details | Formatted Diff | Diff
Patch (14.02 KB, patch)
2011-10-04 13:11 PDT, Eugene Nalimov
no flags Details | Formatted Diff | Diff
Patch (14.80 KB, patch)
2011-10-06 10:15 PDT, Eugene Nalimov
no flags Details | Formatted Diff | Diff
Patch (15.06 KB, patch)
2011-10-06 13:18 PDT, Eugene Nalimov
no flags Details | Formatted Diff | Diff
Patch (12.04 KB, patch)
2011-10-11 14:43 PDT, Eugene Nalimov
no flags Details | Formatted Diff | Diff
Patch (14.99 KB, patch)
2011-10-11 15:07 PDT, Eugene Nalimov
no flags Details | Formatted Diff | Diff
Patch (14.21 KB, patch)
2011-10-13 08:15 PDT, Eugene Nalimov
no flags Details | Formatted Diff | Diff
Patch (4.95 KB, patch)
2011-10-19 09:29 PDT, Eugene Nalimov
no flags Details | Formatted Diff | Diff
Patch (4.92 KB, patch)
2011-10-20 07:09 PDT, Eugene Nalimov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eugene Nalimov 2011-08-24 12:15:11 PDT
According to

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#playing-the-media-resource

"4.8.10.8 Playing the media resource [...] Media elements must not stop playing just because all references to them have been removed; only once a media element is in a state where no further audio could ever be played by that element may the element be garbage collected. [...]"

Unfortunately, that is not so with HTMLAudioElement. Original bug is Chrome one:

http://code.google.com/p/chromium/issues/detail?id=62604

That happens because Chrome internally uses HTMLAudioElement for its media player, and that element is garbage collected. Here is simple script that demonstrates the problem:

<html>
<body>
<hl> Test page </hl>
<script type="text/javascript">
function gc()
{
 if (window.GCController)
   return GCController.collect();

 for (var i = 0; i < 1000; i++) {
   var s = new ArrayBuffer(10000);
 }
}

var begin_ms;
var a = new Audio("file://c:/temp/bwep.wav");
a.addEventListener('ended', function() {
 var end_ms = new Date().getTime();
 var duration_ms = end_ms - begin_ms;
 document.write("<p>Ended: " + duration_ms + "</p>");
});
a.addEventListener('canplaythrough', function() {
 document.write("<p>CanPlayThrough</p>");
 begin_ms = new Date().getTime();
 a.play();
 a = null;
 gc();
});
</script>
</body>
</html>

You can use any sound file that is longer than ~1 second.

On Chrome, sound starts playing, but then abruptly stops, and no 'ended' event is ever fired.

I believe I have simple fix, will submit is shortly.
Comment 1 Andrew Scherkus 2011-09-20 11:05:14 PDT
Where we able to repro it in Safari or other ports that don't use V8?
Comment 2 Eugene Nalimov 2011-09-20 11:14:26 PDT
Bug does not demonstrate itself in Safari, or at least I was not able to cause it.

Part of the fix is in V8, so it should not affect Safari.
Comment 3 Eric Carlson 2011-09-21 08:19:23 PDT
This used to be special cased in the JS bindings, see https://bugs.webkit.org/attachment.cgi?id=32247&action=review. I am not sure when isObservableThroughDOM when away, but I would *think* that a playing media element would not be collected because HTMLMediaElement derives from ActiveDOMObject and HTMLMediaElement::hasPendingActivity returns true when there are pending events. Is this not true in Chrome with your test case?
Comment 4 Eugene Nalimov 2011-09-21 09:19:05 PDT
Unfortunately, just deriving from ActiveDomObject is not enough, at least in V8. You have to modify source/WebCore/bindings/scripts/CodeGeneratorV8.pm as well, adding type to the list of active types.

After that I had to change private inheritance to public, because otherwise some method used in the generated becomes inaccessible.

That is not enough, either -- when playing audio there are no pending events, so HTMLMediaElement::hasPendingActivity() returns false, and garbage collector happily deletes playing object. I had to implement hasPendingActivity() in  HTMLAudioElement.

When testing that change I hit another bug in V8 bindings -- listener object can be deleted while object that generates events is still alive, resulting in loss of JS events. Prolonging life of audio object exposed that bug, but it was in V8 forever. I was fortunate enough, my main repro case actively uses events. Fix is to change code generated for V8Node, and to do it in a way that would not cause every V8Node to have extra slot, usable only in small minority of objects. Anton Muhin helped me in investigation and suggested what fix might be.

I believe now I have all necessary changes, and will finally submit a patch shortly...
Comment 5 Eugene Nalimov 2011-09-27 10:24:17 PDT
Created attachment 108865 [details]
Patch
Comment 6 Adam Barth 2011-10-04 10:18:02 PDT
Comment on attachment 108865 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Playing HTMLAudioElement can be garbage collected
> +        https://bugs.webkit.org/show_bug.cgi?id=66878
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: media/audio-garbage-collect.html

This ChangeLog isn't really helping me understand why you're making this change.  :(

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1167
> +    elsif ($implClassName eq "Node") {
> +        push(@implContentDecls, <<END);
> +        args.Holder()->${hiddenValueAction}HiddenValue(v8::String::Concat(v8::String::New(\"listener/\"),args[0]->ToString())${hiddenValueExtraArgs});

I don't really understand what this part of the change is doing.  Also, why is Node a special case?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2843
>      return 1 if $type eq "IDBTransaction";
>      return 1 if $type eq "FileReader";
>      return 1 if $type eq "FileWriter";
> +    return 1 if $type eq "HTMLAudioElement";

We shouldn't have this list in the code generator!!!  We should get this information from the IDL files.
Comment 7 Eugene Nalimov 2011-10-04 13:11:10 PDT
Created attachment 109669 [details]
Patch
Comment 8 Adam Barth 2011-10-04 14:45:21 PDT
Comment on attachment 109669 [details]
Patch

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

I'm not sure the part of this change that adds the hidden property is correct.  There can be many listeners added for each event.  I thought it was the job of V8EventListener (or one of those related classes) to stop the listener from being collected by GC.

If I were making this change, I would break it down into two parts.

1) Make CodeGeneratorV8.pm respect the ActiveDOMObject IDL attribute.  That will fix the proximate bug with HTMLAudioElement.
2) Fix the bug with event listeners getting garbage collected.

I suspect (2) is better fixed in the V8EventListener objects, which are already involved in managing the lifetimes of these objects.

> Source/WebCore/ChangeLog:13
> +        (b) fix v8 bindings bug that was in the code forever, event listener
> +        could be garbage collected even if object it attached to is still
> +        alive, resulting in the loss of events

Ah!  That makes sense.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1174
> +        // value. Spec guarantees that one can have only a single event listener
> +        // per event type. 

Why do we only have a single event listener for each event type?  Can't you add more with addEventListener?  Maybe you mean the onclick-like properties in the DOM?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2866
> -    # FIXME: Consider making this an .idl attribute.
> +    # FIXME: Make this an .idl attribute.

It is already an IDL attribute.  We need to respect the IDL attribute.
Comment 9 Eugene Nalimov 2011-10-06 10:15:09 PDT
Created attachment 109972 [details]
Patch
Comment 10 Collabora GTK+ EWS bot 2011-10-06 10:30:25 PDT
Comment on attachment 109972 [details]
Patch

Attachment 109972 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9981274
Comment 11 Early Warning System Bot 2011-10-06 10:31:25 PDT
Comment on attachment 109972 [details]
Patch

Attachment 109972 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9969312
Comment 12 Eugene Nalimov 2011-10-06 10:37:40 PDT
I tried to use ActiveDOMObject .idl attribute, in the past 2 days I
learned much more about Perl or our .idl files parser than I ever
wanted to know. Unfortunately, that is not easy -- I added comment
explaining problem and (hard) way it can be attacked. Main problem is
that Perl is not the right language to do semantic analysis, full type
info may be unavailable when we need it.

About listener: yes, technically that is unrelated problem, but for
game developer it does not matter if player itself or "only" listener
object was unexpectedly collected. Again, I do't see easy way to do
all the magic in the listener -- we should do actions when
adding/removing listeners to the events source, and code that does
that is auto-generated, so we have to edit generator every time new
type needs those actions. I could reuse existing mechanism used by
other types (hidden dependencies, object has to have extra slot to
keep array of those pointers), but it will grow each and every "node",
while absolute majority of those objects would never have listener
attached. So I used slightly different approach.

Will send new patch after taking care of qt problem...

Thanks,
Eugene
Comment 13 Adam Barth 2011-10-06 11:00:34 PDT
Comment on attachment 109972 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2871
> +    # FIXME: Make this an .idl attribute.
> +    # Not easy because we do not have $dataNode, only $type, and caller may
> +    # not have $dataNode as well. Getting extendedAttributes starting from
> +    # $type is not supported. Most promising way is to iterate through all
> +    # .idl files searching for the matching type name, and then look at the
> +    # extendedAttributes, but that would require lot of changes...

How does CodeGeneratorJS solve this problem?
Comment 14 Adam Barth 2011-10-06 11:04:19 PDT
> About listener: yes, technically that is unrelated problem, but for
> game developer it does not matter if player itself or "only" listener
> object was unexpectedly collected.

Generally, we prefer to separate different issues into different patches.  Is there any technical benefit for fixing these two bugs in the same patch?  I agree that we should fix both bugs, but they should be fixed in separate patches.
Comment 15 Eugene Nalimov 2011-10-06 11:24:24 PDT
CodeGeneratorJS does not have "bad" caller, GetDomMapFunction(). It is
called when generating ${attrName}AttrGetter() -- actually,
CodeGeneratorJS does not generate ${attrName}AttrGetter() at all.

There, GetDomMapFunction() is called with $returnType, which is return
type for getter. At this point we have type name only, not pointer to
all the type info...
Comment 16 Eugene Nalimov 2011-10-06 11:25:04 PDT
Without 2nd fix I could not invent test case that demonstrates the 1st
problem is fixed. You don't want person to sit and listen to be sure
that audio played fully. Test case relies on the events...
Comment 17 Eugene Nalimov 2011-10-06 13:18:59 PDT
Created attachment 110005 [details]
Patch
Comment 18 Adam Barth 2011-10-10 14:35:02 PDT
Comment on attachment 110005 [details]
Patch

I'm working on a patch to remove IsActiveDomType.  I'll upload it on a bug that's blocking this one.
Comment 19 Adam Barth 2011-10-10 17:11:53 PDT
Ok.  That patch as landed.  Hopefully that unblocks this patch.
Comment 20 Eugene Nalimov 2011-10-11 14:43:07 PDT
Created attachment 110580 [details]
Patch
Comment 21 Adam Barth 2011-10-11 14:51:49 PDT
Comment on attachment 110580 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        (b) fix v8 bindings bug that was in the code forever, event listener
> +        could be garbage collected even if object it attached to is still
> +        alive, resulting in the loss of events.

This part of the change doesn't appear to be in this patch anymore.  Maybe update the ChangeLog?

> Source/WebCore/bindings/v8/custom/V8HTMLAudioElementConstructor.cpp:50
> -WrapperTypeInfo V8HTMLAudioElementConstructor::info = { V8HTMLAudioElementConstructor::GetTemplate, 0, 0, 0 };
> +WrapperTypeInfo V8HTMLAudioElementConstructor::info = { V8HTMLAudioElementConstructor::GetTemplate, V8HTMLAudioElement::derefObject, V8HTMLAudioElement::toActiveDOMObject, 0 };

Do we need to do this for every ActiveDOMObject?  Why doesn't the code generator do this for us?

> Source/WebCore/bindings/v8/custom/V8HTMLAudioElementConstructor.cpp:78
> -    V8DOMWrapper::setJSWrapperForDOMNode(audio.get(), v8::Persistent<v8::Object>::New(args.Holder()));
> +    V8DOMWrapper::setJSWrapperForActiveDOMObject(audio.get(), v8::Persistent<v8::Object>::New(args.Holder()));

Do we need to do this for every active dom object?  Should we check all the constructors for active DOM objects?

> LayoutTests/fast/xpath/xpath-result-eventlistener-crash.html:29
> -    for (var i = 0; i < 5000; ++i)
> +    for (var i = 0; i < 2000; ++i)

This change seems unrelated.
Comment 22 Eugene Nalimov 2011-10-11 14:58:52 PDT
(1) Oops, code generator changes somehow not included into patch... Will resubmit shortly...

>Do we need to do this for every ActiveDOMObject?  Why doesn't the code generator >do this for us?

>> Source/WebCore/bindings/v8/custom/V8HTMLAudioElementConstructor.cpp:78
>> -    V8DOMWrapper::setJSWrapperForDOMNode(audio.get(), v8::Persistent<v8::Object>::New(args.Holder()));
>> +    V8DOMWrapper::setJSWrapperForActiveDOMObject(audio.get(), v8::Persistent<v8::Object>::New(args.Holder()));

>Do we need to do this for every active dom object?  Should we check all the >constructors for active DOM objects?

(2) That is custom constructor, not generated one. If it was generated everything would be Ok.

>> LayoutTests/fast/xpath/xpath-result-eventlistener-crash.html:29
>> -    for (var i = 0; i < 5000; ++i)
>> +    for (var i = 0; i < 2000; ++i)

>This change seems unrelated.

(3) On my (slow) Mac test started to time out when running debug Chrome build, because event listener became slower. Reduced number of iterations.
Comment 23 Eugene Nalimov 2011-10-11 15:07:03 PDT
Created attachment 110585 [details]
Patch
Comment 24 Adam Barth 2011-10-11 15:11:58 PDT
> >> LayoutTests/fast/xpath/xpath-result-eventlistener-crash.html:29
> >> -    for (var i = 0; i < 5000; ++i)
> >> +    for (var i = 0; i < 2000; ++i)
> 
> >This change seems unrelated.
> 
> (3) On my (slow) Mac test started to time out when running debug Chrome build, because event listener became slower. Reduced number of iterations.

Does this mean your patch is causing a performance regression (even in release)?
Comment 25 Adam Barth 2011-10-11 15:13:27 PDT
Comment on attachment 110585 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1183
> +        char prefix[64];
> +        sprintf(prefix, \"listener/%p/\", listener.get());
> +        args.Holder()->${hiddenValueAction}HiddenValue(v8::String::Concat(v8::String::New(prefix), args[0]->ToString())${hiddenValueExtraArgs});

I'm still skeptical about this approach.  This doesn't seem like the right way of solving this problem.
Comment 26 Eugene Nalimov 2011-10-11 15:23:31 PDT
>Does this mean your patch is causing a performance regression (even in release)?

In release test passed, but I assume code became slower, too -- I am doing more when adding/removing event listener. Some other types are also have slower add/removeEventListenerCallback() because they also keep track of listener objects, but V8Node till that moment did not keep track, and that was the bug...

>I'm still skeptical about this approach.  This doesn't seem like the right way of solving this problem.

I discussed it with Anton Muhin. Initially he thought I should do what all other objects are doing, that is adding/removing hidden dependency, but later suggested that approach. He believes that is better, because it avoids adding extra slot to V8Node object -- there are lot of nodes, and absolute majority of them would never have listener attached.
Comment 27 Adam Barth 2011-10-11 15:32:59 PDT
Would it be better to do the work during GC using object groups?  Can't we just add the slot to V8AudioElement and not to all V8Nodes ?
Comment 28 Eugene Nalimov 2011-10-11 18:33:02 PDT
>Can't we just add the slot to V8AudioElement and not to all V8Nodes ? 

Take a look at the test which I had to speed up, LayoutTests/fast/xpath/xpath-result-eventlistener-crash.html. There is no single HTMLAudioElement in it, but it adds event listener to JS object that inherits from V8Node. And I bet it's possible to write similar test demonstrating the problem...
Comment 29 Alexey Proskuryakov 2011-10-11 22:47:04 PDT
Comment on attachment 110585 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.h:61
> -class HTMLMediaElement : public HTMLElement, public MediaPlayerClient, private MediaCanStartListener, private ActiveDOMObject {
> +class HTMLMediaElement : public HTMLElement, public MediaPlayerClient, private MediaCanStartListener, public ActiveDOMObject {

I think the plan is to stop inheriting from ActiveDOMObject in HTMLMediaElement.

> LayoutTests/fast/xpath/xpath-result-eventlistener-crash.html:29
> -    for (var i = 0; i < 5000; ++i)
> +    for (var i = 0; i < 2000; ++i)

Please don't make unrelated tests weaker.
Comment 30 Adam Barth 2011-10-12 09:47:49 PDT
> I think the plan is to stop inheriting from ActiveDOMObject in HTMLMediaElement.

Do you have an alternative proposal for fixing this bug without making HMLTAudioElement an active DOM object?
Comment 31 Alexey Proskuryakov 2011-10-12 10:55:30 PDT
I should have been specific about reasons for r-, which is because of XPath test changes. Changing ActiveDOMObject inheritance from private to public is not a big deal - this comment was informative. Sorry for the confusion.

In fact, it may no longer be true that we need to stop inheriting from ActiveDOMObject. I've been thinking of bug 45306 comment 9 and bug 45309, which both seem obsolete.
Comment 32 Adam Barth 2011-10-12 11:14:45 PDT
Thanks Alexey.  I agree that the changes to the XPath test are incorrect.
Comment 33 Eugene Nalimov 2011-10-12 11:31:45 PDT
As suggested, working on much smaller change that fixes only V8HTMLAudioElement, without attempts to fix V8Node addEventListenerCallback() issue.

That will avoid non-trivial changes in the code generator or changes in seemingly unrelated files.

Will send for review later today.
Comment 34 Adam Barth 2011-10-12 11:46:17 PDT
> As suggested, working on much smaller change that fixes only V8HTMLAudioElement, without attempts to fix V8Node addEventListenerCallback() issue.

I see.  This can occur even for Nodes which aren't ActiveDOMObjects if something retains the Node without retaining the EventListener.  The EventListener can then be GCed and the Node won't be GCed.

I'm curious why it isn't a better solution to group the Nodes and their EventListeners in V8GCController so they're collected together or not at all.
Comment 35 Eugene Nalimov 2011-10-12 11:52:41 PDT
I never worked with object groups, and heard that such thing exists only yesterday.

In any case, we can create object, and add event listener much later. Or we can remove event listener from the node, and add new one -- in such case it is perfectly Ok to GC first event listener, I would even argue it is bug not to collect unused event listener...
Comment 36 Adam Barth 2011-10-12 11:59:47 PDT
> I never worked with object groups, and heard that such thing exists only yesterday.

Basically they way it works is just before we run a GC, we can temporarily group objects together so that they're all collected together or not at all.  For example, that's who we keep CSSOM wrappers alive.  In this case, we'd associate event listeners with their Nodes.

There are trade-offs involved.  The approach you've got here pays a runtime and memory cost O(number of event listeners added/removed).  The GCController approach pays a runtime cost O(number of nodes) or something else if we're more clever.

I still think the right approach here is to fix HTMLAudioElement to not be GCed why playing and to worry about the event listener issue in a separate bug.  The event listener thing is subtle and needs to be done carefully.

It's unfortunate that we can't write a test for the HTMLAudioElement issue before fixing the event listener issue.  One way to handle that is to file a bug for adding the test that's blocked on the (new) event listener bug.  When we fix the event listener bug, we can land the test.
Comment 37 Eugene Nalimov 2011-10-13 08:15:28 PDT
Created attachment 110845 [details]
Patch
Comment 38 Eric Carlson 2011-10-13 09:03:29 PDT
Comment on attachment 110845 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        Fix consists of 2 parts:
> +        (a) make HTMLAudioElement an 'active' one, meaning that it cannot be
> +        garbage collected if it has panding activity
> +        (b) workaround for v8 bindings bug that was in the code forever, event
> +        listener could be garbage collected even if object it attached to is
> +        still alive, resulting in the loss of events. Workaround is to make
> +        HTMLAudioElement to implement EventTarget interface, and modify code
> +        generator to create hidden dependency between object and event listener
> +        even for types that inherit from Node (but not for Node itself).

Can't this be broken into two patches? While part (a) depends on part (b), the later is not specific to this bug and seems worthy of its own bug/patch.

> LayoutTests/media/audio-garbage-collect.html:7
> +<html>
> +<body>
> +
> +<p>Tests that we don't garbage collect playing audio object or event listener.</p>
> +<p>According to http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html,<br />

Nit: Indentation would make this file much more readable, IMO.

> LayoutTests/media/audio-garbage-collect.html:18
> +</p>
> +
> +<script src=media-file.js></script>
> +<script src=video-test.js></script>

Nit: Is there any reason to not have the script nodes in <head>?

> LayoutTests/media/audio-garbage-collect.html:26
> +var a;
> +var num_played;
> +var total_played = 0;

Nit: is there any reason to have these in the global scope?

> LayoutTests/media/audio-garbage-collect.html:46
> +  var audioFile = findMediaFile("audio", "content/click2");

1. Why do you need a new audio file, can't you use silence.wav (it and all of the existing files with audio tracks purposely have encoded silence so a computer running layout tests doesn't make noise)? If it is too long, just seek to near the end before starting playback. 
2. findMediaFile() is only necessary when you have a multiple versions of a media file with different encodings.
Comment 39 Eugene Nalimov 2011-10-13 09:24:58 PDT
>Can't this be broken into two patches?

I thought about that. Problem is that I could not (easily) reproduce 2nd problem without 1st fix, I tried and then gave up. At lest now all changes are HTMLAudioElement-only.

I'll do that if you say I need, but I'd prefer to have one checkin that fixes HTML5 GC-related problems...

>Nit: Is there any reason to not have the script nodes in <head>?
>Nit: Indentation would make this file much more readable, IMO.

I did not invented test from scratch. I started from existing one, and modified it. I believe absolute majority of our tests following that pattern...

>Nit: is there any reason to have these in the global scope?

I have to have communicating callbacks (event functions), and I thought that just using globals would be simplest way. Here you don't need smart code, you need code that can be understood by average developer in 30 seconds...

>1. Why do you need a new audio file, can't you use silence.wav (it and all of the existing files with audio tracks purposely have encoded silence so a computer running layout tests doesn't make noise)? If it is too long, just seek to near the end before starting playback.

On specific file problem demonstrates itself much more often than on other files. Maybe that is exact timing, maybe something else, I tried silence.wav and decided I'd better check in file where problem happens 95% of times.
 
>2. findMediaFile() is only necessary when you have a multiple versions of a media file with different encodings.

All audio tests use that functions to open files, I decided to do that as well.
Comment 40 Eugene Nalimov 2011-10-13 09:30:26 PDT
More, both fixes touch exactly the same file -- HTMLAudioElement.idl...
Comment 41 Eric Carlson 2011-10-13 09:51:01 PDT
(In reply to comment #39)
> 
> >Nit: Is there any reason to not have the script nodes in <head>?
> >Nit: Indentation would make this file much more readable, IMO.
> 
> I did not invented test from scratch. I started from existing one, and modified it. I believe absolute majority of our tests following that pattern...

  You may not have invented the test, but it is new to the media directory, where the absolute majority of tests do NOT follow this pattern.


> >Nit: is there any reason to have these in the global scope?
> 
> I have to have communicating callbacks (event functions), and I thought that just using globals would be simplest way. 

  Aren't all of your event listeners in start's scope?

> Here you don't need smart code you need code that can be understood by average developer in 30 seconds...
  We need to write code that can be understood by the average developer in 30 seconds? I haven't heard this "rule" before, do we have "average" developers contributing a lot of code to WebKit? 

  In any case, I definitely don't agree. IWebKit code doesn't need to be complex, but it *should* be clean and understandable by WebKit contributors. It should not be needlessly sloppy, IMO.


> >1. Why do you need a new audio file, can't you use silence.wav (it and all of the existing files with audio tracks purposely have encoded silence so a computer running layout tests doesn't make noise)? If it is too long, just seek to near the end before starting playback.
> 
> On specific file problem demonstrates itself much more often than on other files. Maybe that is exact timing, maybe something else, I tried silence.wav and decided I'd better check in file where problem happens 95% of times.
> 

  Are you saying that the problem does not happen with other files? If so, I don't think we understand the problem - how can the content of a media file change the behavior of the GC?


> >2. findMediaFile() is only necessary when you have a multiple versions of a media file with different encodings.
> 
> All audio tests use that functions to open files, I decided to do that as well.

  Read the function. It is only necessary when there are multiple encodings of a given file.
Comment 42 Eugene Nalimov 2011-10-13 10:23:39 PDT
1. Will work on cleaning up the test... 
2. Depending on exactly when GC happens -- during playback, or after playback, when event was fired but still waiting in the message queue -- we are hitting either one of 2 bugs, or if we are lucky no bug at all (for majority of test we have reference to audio player, and if GC happens after we fire the first event but before we add next event then 2nd bug would not be noticeable, too). So test behavior depends on the exact duration of audio stream, and to some degree on CPU speed, though this degree is much smaller. I'll try to use silence.wav and figure out exact position in file, hope will be able to do so.
3. I don't think closure is trivial concept, or that all developers know about it, or know exact JS semantics, but I will change the test to use closure (i.e. local scope).
4. Will send next revision for review either later today or tomorrow. Working on M15 blocker.
Comment 43 Darin Adler 2011-10-17 12:49:57 PDT
Comment on attachment 110845 [details]
Patch

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

I agree with Eric that the V8 bindings fix and the audio element fix should be separated.

> Source/WebCore/html/HTMLAudioElement.h:43
> +    virtual bool hasPendingActivity() const { return isPlaying() || HTMLMediaElement::hasPendingActivity(); }

This should be private, not public.

> Source/WebCore/html/HTMLAudioElement.idl:29
> +        EventTarget,

This is a bad idea.

> Source/WebCore/html/HTMLAudioElement.idl:40
> +        // EventTarget interface                           
> +        void addEventListener(in DOMString type,
> +                              in EventListener listener,
> +                              in [Optional] boolean useCapture);
> +        void removeEventListener(in DOMString type,
> +                                 in EventListener listener,
> +                                 in [Optional] boolean useCapture);
> +        boolean dispatchEvent(in Event event)
> +            raises(EventException);    

This is wrong. We never have to repeat the EventTarget interface like this in a Node subclass. If this was necessary we’d have it tons of other classes too.

> Source/WebCore/html/HTMLMediaElement.h:61
> +class HTMLMediaElement : public HTMLElement, public MediaPlayerClient, private MediaCanStartListener, public ActiveDOMObject {

This should stay private.

> Source/WebCore/html/HTMLMediaElement.h:208
> +    virtual bool hasPendingActivity() const;

This should be protected, not public.
Comment 44 Eugene Nalimov 2011-10-17 13:14:23 PDT
>I agree with Eric that the V8 bindings fix and the audio element fix should be separated.

Ok, will separate. Hope I'll be able to check in audio element fix without test case -- I was told it is no-no...

>This should be private, not public.

The moment you make type "officially" active -- i.e. it does not just inherit from ActiveDOMObject, but code generator knows about it -- hasPendingActivity() should be public, otherwise some generated code would not compile: http://queues.webkit.org/results/9969312. I initially had protected/private, exactly as you suggested...

>This is wrong. We never have to repeat the EventTarget interface like this in a Node subclass. If this was necessary we’d have it tons of other classes too.

I tried to limit the change to HTMLAudioElement node, without touching Node class -- reviewer wrote "... event listener thing is subtle and needs to be done carefully."... My initial attempt included Node fix, and I still think it is right one.

Will submit audio fix only, but we really need both parts of the fix, because events are being lost...
Comment 45 Eugene Nalimov 2011-10-19 09:29:55 PDT
Created attachment 111634 [details]
Patch
Comment 46 Eric Carlson 2011-10-19 13:33:59 PDT
I am not qualified to review the V8 portion of the fix, but the media element changes look fine. 

I would, however, like to see a layout test with this change. Wouldn't it be possible to include a layout test if this change is submitted after bug 70421?
Comment 47 Eugene Nalimov 2011-10-19 13:45:46 PDT
I can try submitting fix for 70421 first, but it will have exactly the same issue -- its layout test would be blocked by that bug. Garbage collection triggers both issues, and bugs manifest themselves only if timing is right. Maybe it's possible to invent separate test case for 70421, but I could not. (I was told there were bug reports about events losses, but they are not in our bug databases, and reproducing is extremely unreliable...)

That is why I tried to submit both fixes together...
Comment 48 Adam Barth 2011-10-19 14:00:34 PDT
Comment on attachment 111634 [details]
Patch

Thanks.  Please be sure to add a test once we fix Bug 70421.
Comment 49 Eugene Nalimov 2011-10-20 07:09:10 PDT
Created attachment 111764 [details]
Patch
Comment 50 WebKit Review Bot 2011-10-20 11:25:27 PDT
Comment on attachment 111764 [details]
Patch

Clearing flags on attachment: 111764

Committed r98005: <http://trac.webkit.org/changeset/98005>
Comment 51 WebKit Review Bot 2011-10-20 11:25:36 PDT
All reviewed patches have been landed.  Closing bug.