Full Text Bug Listing

Eugene Nalimov

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

Andrew Scherkus

Comment 1 2011-09-20 11:05:14 PDT

Where we able to repro it in Safari or other ports that don't use V8?

Eugene Nalimov

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

Eric Carlson

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

Eugene Nalimov

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

Eugene Nalimov

Comment 5 2011-09-27 10:24:17 PDT

Created attachment 108865 [details] Patch

Adam Barth

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

Eugene Nalimov

Comment 7 2011-10-04 13:11:10 PDT

Created attachment 109669 [details] Patch

Adam Barth

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

Eugene Nalimov

Comment 9 2011-10-06 10:15:09 PDT

Created attachment 109972 [details] Patch

Collabora GTK+ EWS bot

Comment 10 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

Early Warning System Bot

Comment 11 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

Eugene Nalimov

Comment 12 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

Adam Barth

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

Adam Barth

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

Eugene Nalimov

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

Eugene Nalimov

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

Eugene Nalimov

Comment 17 2011-10-06 13:18:59 PDT

Created attachment 110005 [details] Patch

Adam Barth

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

Adam Barth

Comment 19 2011-10-10 17:11:53 PDT

Ok. That patch as landed. Hopefully that unblocks this patch.

Eugene Nalimov

Comment 20 2011-10-11 14:43:07 PDT

Created attachment 110580 [details] Patch

Adam Barth

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

Eugene Nalimov

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

Eugene Nalimov

Comment 23 2011-10-11 15:07:03 PDT

Created attachment 110585 [details] Patch

Adam Barth

Comment 24 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)?

Adam Barth

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

Eugene Nalimov

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

Adam Barth

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

Eugene Nalimov

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

Alexey Proskuryakov

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

Adam Barth

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

Alexey Proskuryakov

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

Adam Barth

Comment 32 2011-10-12 11:14:45 PDT

Thanks Alexey. I agree that the changes to the XPath test are incorrect.

Eugene Nalimov

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

Adam Barth

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

Eugene Nalimov

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

Adam Barth

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

Eugene Nalimov

Comment 37 2011-10-13 08:15:28 PDT

Created attachment 110845 [details] Patch

Eric Carlson

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

Eugene Nalimov

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

Eugene Nalimov

Comment 40 2011-10-13 09:30:26 PDT

More, both fixes touch exactly the same file -- HTMLAudioElement.idl...

Eric Carlson

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

Eugene Nalimov

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

Darin Adler

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

Eugene Nalimov

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

Eugene Nalimov

Comment 45 2011-10-19 09:29:55 PDT

Created attachment 111634 [details] Patch

Eric Carlson

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

Eugene Nalimov

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

Adam Barth

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

Eugene Nalimov

Comment 49 2011-10-20 07:09:10 PDT

Created attachment 111764 [details] Patch

WebKit Review Bot

Comment 50 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>

WebKit Review Bot

Comment 51 2011-10-20 11:25:36 PDT

All reviewed patches have been landed. Closing bug.

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
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Bug 66878