Bug 72698

Created attachment 115760 [details] proposed patch Not setting r flag because of the second issue mentioned above.

Marked flaky in http://trac.webkit.org/changeset/100733

According to http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.htm it is perfectly Ok to run play() on audio object immediately after setting its currentTime. E.g. there is example on that page that contains function playSound(id) { sfx.currentTime = sounds.getCueById(id).startTime; sfx.play(); } So I don't believe flakiness is caused by calling play() after setting currentTime -- and you still see flakiness when you apply your patch. Can the test somehow fail because of the previous test failure? Are they using the same browser instance?

(In reply to comment #3) > According to > > http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.htm > > it is perfectly Ok to run play() on audio object immediately after setting its currentTime. E.g. there is example on that page that contains > > function playSound(id) { > sfx.currentTime = sounds.getCueById(id).startTime; > sfx.play(); > } > > So I don't believe flakiness is caused by calling play() after setting currentTime Right, indeed! > -- and you still see flakiness when you apply your patch. > > Can the test somehow fail because of the previous test failure? Are they using the same browser instance? Well yes, a single WebView is used per instance of DRT. We currently limit NRWT to one worker.

I dedicated some hours to this issue today... Here are my findings: - test executed standalone: works - test executed after a video test: works - test executed after an audio test: times out If I replace: var a = new Audio(audioFile); with: setSrcByTagName("audio", findMediaFile("audio", "content/silence")); var a = document.getElementById('audio'); (and a <audio id="audio"/> element in the body). And: a = null; with: a.parentNode.removeChild(a); The test passes in all cases. Could this be a JSC-related issue?

When you got rid of a = null; you are avoiding situation test is checking -- variable "a" still contains reference to audio object, so it would not be garbage collected regardless of its status. Test checks that audio object cannot be collected while it is playing even if there is no reference to it left. When you are saying "JSC" do you mean JS interpreter/JIT-ter used in Safari? Or is the test flaky with Chromium as well?

(In reply to comment #6) > When you got rid of > a = null; > you are avoiding situation test is checking -- variable "a" still contains reference to audio object, so it would not be garbage collected regardless of its status. Test checks that audio object cannot be collected while it is playing even if there is no reference to it left. > Alright. > When you are saying "JSC" do you mean JS interpreter/JIT-ter used in Safari? Or is the test flaky with Chromium as well? This issue happens on the GTK port of WebKit, which uses JavaScriptCore.

Then my guess would be that JSC/GTK contains bug similar to v8 one I fixed. I had to fix 2 different bugs in V8: * One in HTMLAudioElement, it was not "active" object (i.e. object that can have some hidden activity, meaning that it cannot be collected before it finishes that activity). * Second bug was is v8 bindings, code handling active objects was not designed to work with active objects derived from "Node". For details please see https://bugs.webkit.org/show_bug.cgi?id=66878 https://bugs.webkit.org/show_bug.cgi?id=70421

(In reply to comment #8) > Then my guess would be that JSC/GTK contains bug similar to v8 one I fixed. > > I had to fix 2 different bugs in V8: > * One in HTMLAudioElement, it was not "active" object (i.e. object that can have some hidden activity, meaning that it cannot be collected before it finishes that activity). > * Second bug was is v8 bindings, code handling active objects was not designed to work with active objects derived from "Node". > > For details please see > https://bugs.webkit.org/show_bug.cgi?id=66878 > https://bugs.webkit.org/show_bug.cgi?id=70421 I think this is indeed a bug in the JSC bindings, patch incoming. Thanks a lot for the hints Eugene.

Created attachment 130608 [details] Patch

Comment on attachment 130608 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130608&action=review > Source/WebCore/bindings/js/JSNodeCustom.cpp:119 > + if (!static_cast<HTMLAudioElement*>(node)->hasPendingActivity()) hasPendingActivity() is definitely better than paused(), but why it matters for that particular case?

(In reply to comment #11) > (From update of attachment 130608 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=130608&action=review > > > Source/WebCore/bindings/js/JSNodeCustom.cpp:119 > > + if (!static_cast<HTMLAudioElement*>(node)->hasPendingActivity()) > > hasPendingActivity() is definitely better than paused(), but why it matters for that particular case? Because the media element ends up paused and thus without this patch prevents the garbage collection to succeed (I think). HTMLAudioElement::hasPendingActivity() first checks isPlaying(), which is false. So it seems that witch this patch the correct code path is used during the test and it doesn't time out.

LGTM, but I don't know how to make it official, and I don't have commit rights...

Comment on attachment 130608 [details] Patch r=me since this does what the image element is already doing and fixes the test.

Comment on attachment 130608 [details] Patch Clearing flags on attachment: 130608 Committed r110064: <http://trac.webkit.org/changeset/110064>

All reviewed patches have been landed. Closing bug.

This has caused a serious regression, bug 82421. Bugs with [platform] prefixes should never have patches that change cross platform code. This is because most core developers don't look at such bugs, so they effectively get less scrutiny. Please be very careful to only use the [GTK] prefix when you expect that the issue is purely port specific, and remove the prefix if it turns out to be cross platform. Also, you should have CC'ed folks working on media elements, and on JavaScriptCore bindings. This code has been looked at many times in the past, so there may be subtle consequences to changes like this.

> Bugs with [platform] prefixes should never have patches that change cross platform code. Eugene, Philippe, Xan, a number of bad things happened here: (1) As Alexey mentioned, you marked this patch as a [platform] bug, when it's not. (2) Eugene asserted without evidence that "hasPendingActivity() is definitely better than paused()", and didn't consult any media experts or anyone in the SVN blame list for this code about his assertion. (3) Nobody tested that, after this change, audio elements could still be garbage collected. (4) Xan r+-ed this patch despite all the above problems. Please use better judgement in the future.

(In reply to comment #18) > (1) As Alexey mentioned, you marked this patch as a [platform] bug, when it's not. It is often the case that a bug will start out to be platform-specific (in this case it was some GTK+ tests showing flakiness) and later gradually turn into a platform-independent bug. People, by nature, are forgetful or busy or tired or overwhelmed with work, so often forget to remove tags like "[GTK]." I think there are some avenues for improving this situation though: 1. People could make sure to add themsevles to a watchlist for files they care about, which will CC them automatically. 2. check-webkit-style could be taught to look out for patches that contain strings like "[GTK]" and change files outside of the platform-specific directories.

Rolled out in http://trac.webkit.org/projects/webkit/changeset/112483. Reopening this bug. Eric's comments from https://bugs.webkit.org/show_bug.cgi?id=82421 Comment #9 From Eric Carlson 2012-03-28 17:41:24 PST (-) [reply] (In reply to comment #6) > I think it would be even better just to revert http://trac.webkit.org/projects/webkit/changeset/110064 and consult Eric and Jer about what the best fix is. It's not clear that HTMLAudioElement::hasPendingActivity is the right test for GC. HTMLAudioElement::hasPendingActivity is definitely NOT the correct test. The reason that we care about this is that the spec says when a media element can be collected: Media elements that are potentially playing while not in a Document must not play any video, but should play any audio component. 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. and "potentially playing" is defined as: 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. As it happens, we have HTMLMediaElement::potentiallyPlaying because "potentially playing" is used in other parts of the media element spec.

I'm so sorry for all the trouble caused. I'll use better judgement in the future, especially about bug title tags CC'ed people.

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

Created attachment 212365 [details] non-working fix Attaching my unsuccessful cut at fixing this. Changed WebCore to use potentiallyPlaying, and made the test more strict - currently it's pretty much ineffective, as it only calls gc() when a pointer is almost certain to be on the stack anyway. Looks like issues with Audio garbage collection are deeper yet.

Skipped the test in <http://trac.webkit.org/r156280>. <rdar://problem/15056308>