Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1593763002:
Keep AudioSourceNode from premature GC with ScriptWrappable::hasPendingActivity() (Closed)
DescriptionAdd ScriptWrappable to SchduledSourceNode and ScriptProcessorNode
|onended| event in OscillatorNode, AudioBufferSourceNode and
|onaudioprocess| event in ScriptProcessor node are not fired once the JS
reference of the node goes out of local scope.
This CL is to keep those nodes from being collected when they still has
a pending activity. This can be done by adding
ScriptWrappable.hasPendingActivity() to them.
The change in ScriptWrappable is:
https://codereview.chromium.org/1609343002/
BUG=577431, 484176, 559220
TEST=LayoutTests/webaudio/audiosource-premature-gc.html
Committed: https://crrev.com/cfa51385f9a17df78576c607827910f91f4c6683
Cr-Commit-Position: refs/heads/master@{#377231}
Committed: https://crrev.com/97be0f5a7309a7da5c7da95c637eda34236f25f2
Cr-Commit-Position: refs/heads/master@{#377783}
Patch Set 1 : Initial Patch #
Total comments: 1
Patch Set 2 : [DO NOT SUBMIT] ActiveDOMObject to ScriptWrappable #Patch Set 3 : Added [DependentLifetime] #
Total comments: 1
Patch Set 4 : Error fixed in releaseActiveSourceNodes() #
Total comments: 2
Patch Set 5 : Simplify changes (-AbstractAudioContext) #
Total comments: 11
Patch Set 6 : Simplify changes (startNode > start) #
Total comments: 1
Patch Set 7 : Fixed ordering issue in test #
Total comments: 9
Patch Set 8 : Cleaning up test #
Messages
Total messages: 102 (33 generated)
Patchset #1 (id:1) has been deleted
hongchan@chromium.org changed reviewers: + rtoy@chromium.org
PTAL. Note: the new test 100% fails on the build without this patch. https://codereview.chromium.org/1593763002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/scriptprocessornode-premature-death-expected.txt (right): https://codereview.chromium.org/1593763002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/scriptprocessornode-premature-death-expected.txt:1: Tests that a script processor node is not prematurely GCed Now ScriptProcessor node behaves correctly, this test is not failing any more.
Description was changed from ========== AudioNode as ActiveDOMObject AudioNode has not equipped ActiveDOMObject, so ContextLifeCycleObserver does not pay attention to its instance. Thus |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep AudioNode from being collected when it still has a pending activity by making it observable to ContextLifeCycleObserver. BUG=121654,484176,500335,559220,360378 TEST=LayoutTests/webaudio/audiosource-onended-gc.html ========== to ========== AudioNode as ActiveDOMObject AudioNode has not equipped ActiveDOMObject, so ContextLifeCycleObserver does not pay attention to its instance. Thus |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep AudioNode from being collected when it still has a pending activity by making it observable to ContextLifeCycleObserver. BUG=121654,484176,500335,559220,360378 TEST=LayoutTests/webaudio/audiosource-onended-gc.html ==========
hongchan@chromium.org changed reviewers: + dominicc@chromium.org, haraken@chromium.org
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1593763002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1593763002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL - dominicc@ and haraken@ This is the initial attempt to make AudioNode as ActiveDOMObject. If this is a bad idea as dominicc@ suggested, I have to come up with a different solution. If that's the case, I guess we can talk about the alternative design in this CL.
Thanks for your tireless work improving Web Audio! Let me check my understanding--the problem here was that things were being garbage collected too soon? You've implemented ActiveDOMObject and added tests that they're not collected too soon; that's good. However now there's the risk that they're collected too late. Could you add some additional tests for this? This will help ensure the change doesn't induce leaks. The structure of the tests will be similar to the test you have, but instead of doing context.startRendering() you'll instead drop the context on the floor and invoke gc() for it to be collected. To tell whether the AudioNode is collected you'll need to use the internals.observeGC thing; here's an example (the rectObservation.wasCollected part): https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... With that test in hand we can dig into the code review in depth! Cheers, D
On 2016/01/18 05:37:46, (out until Jan 5) dominicc wrote: > Thanks for your tireless work improving Web Audio! > > Let me check my understanding--the problem here was that things were being > garbage collected too soon? You've implemented ActiveDOMObject and added tests > that they're not collected too soon; that's good. However now there's the risk > that they're collected too late. Could you add some additional tests for this? > This will help ensure the change doesn't induce leaks. > > The structure of the tests will be similar to the test you have, but instead of > doing context.startRendering() you'll instead drop the context on the floor and > invoke gc() for it to be collected. To tell whether the AudioNode is collected > you'll need to use the internals.observeGC thing; here's an example (the > rectObservation.wasCollected part): > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... A nice suggestion! I will come up with one more test or an additional task in the current test. However, I will be traveling the next week and it has to be done a week after.
My question is why you need to make the entire AudioNode an ActiveDOMObject. Is it not enough to make only AudioScheduledSourceNode etc an ActiveDOMObject?
On 2016/01/19 02:29:41, haraken wrote: > My question is why you need to make the entire AudioNode an ActiveDOMObject. Is > it not enough to make only AudioScheduledSourceNode etc an ActiveDOMObject? haraken@ No, you are right that it is not necessary to make the entire AudioNode to ActiveDOMObject, but it should be applied to ScriptProcessorNode as well. Do you believe it is a valid solution despite all the ideas dominicc@ suggested? So here what I will do next: 1) Add another layout test dominicc@ suggested above. 2) Make only AudioScheduledSourceNode and ScriptProcessorNode to ActiveDOMObject.
On 2016/01/19 15:30:14, hoch wrote: > On 2016/01/19 02:29:41, haraken wrote: > > My question is why you need to make the entire AudioNode an ActiveDOMObject. > Is > > it not enough to make only AudioScheduledSourceNode etc an ActiveDOMObject? > > haraken@ > > No, you are right that it is not necessary to make the entire AudioNode to > ActiveDOMObject, but it should be applied to ScriptProcessorNode as well. > Do you believe it is a valid solution despite all the ideas dominicc@ suggested? > > So here what I will do next: > > 1) Add another layout test dominicc@ suggested above. > 2) Make only AudioScheduledSourceNode and ScriptProcessorNode to > ActiveDOMObject. Yes, I think this would be the simplest solution. It is a common programming pattern to make only a part of a big DOM hierarchy an ActiveDOMObject (e.g., HTMLMediaElement).
> It is a common programming pattern to make only a part of a big DOM hierarchy an ActiveDOMObject (e.g. HTMLMediaElement). Is it because of the performance implication? If that's the case, I am a bit worried to introduce ActiveDOMObject to these audio nodes since they are supposed to be created/destructed rapidly in bulk. (i.e. some applications create a buffer source for every 3ms while others create 20~30 oscillator nodes for every user interaction.) If you think this is too much for ActiveDOMObject, then we need to think about the alternative.
On 2016/01/19 16:47:40, hoch wrote: > > It is a common programming pattern to make only a part of a big DOM hierarchy > an ActiveDOMObject (e.g. HTMLMediaElement). > > Is it because of the performance implication? If that's the case, I am a bit > worried to introduce ActiveDOMObject to these audio nodes since they are > supposed to be created/destructed rapidly in bulk. (i.e. some applications > create a buffer source for every 3ms while others create 20~30 oscillator nodes > for every user interaction.) > > If you think this is too much for ActiveDOMObject, then we need to think about > the alternative. Yeah, that sounds problematic. ActiveDOMObject is heavy. Maybe can you introduce some class that groups the audio nodes and make the class an ActiveDOMObject?
On 2016/01/19 17:02:22, haraken wrote: > On 2016/01/19 16:47:40, hoch wrote: > > > It is a common programming pattern to make only a part of a big DOM > hierarchy > > an ActiveDOMObject (e.g. HTMLMediaElement). > > > > Is it because of the performance implication? If that's the case, I am a bit > > worried to introduce ActiveDOMObject to these audio nodes since they are > > supposed to be created/destructed rapidly in bulk. (i.e. some applications > > create a buffer source for every 3ms while others create 20~30 oscillator > nodes > > for every user interaction.) > > > > If you think this is too much for ActiveDOMObject, then we need to think about > > the alternative. > > Yeah, that sounds problematic. ActiveDOMObject is heavy. > > Maybe can you introduce some class that groups the audio nodes and make the > class an ActiveDOMObject? Sure. I will write up the idea and post PS2 next week.
Nit: It would be helpful to simplify the BUG= line to 577431, or at least include that first in the list.
Description was changed from ========== AudioNode as ActiveDOMObject AudioNode has not equipped ActiveDOMObject, so ContextLifeCycleObserver does not pay attention to its instance. Thus |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep AudioNode from being collected when it still has a pending activity by making it observable to ContextLifeCycleObserver. BUG=121654,484176,500335,559220,360378 TEST=LayoutTests/webaudio/audiosource-onended-gc.html ========== to ========== AudioNode as ActiveDOMObject AudioNode has not equipped ActiveDOMObject, so ContextLifeCycleObserver does not pay attention to its instance. Thus |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep AudioNode from being collected when it still has a pending activity by making it observable to ContextLifeCycleObserver. BUG=577431,121654,484176,500335,559220,360378 TEST=LayoutTests/webaudio/audiosource-onended-gc.html ==========
On 2016/01/25 05:18:35, (out until Jan 5) dominicc wrote: > Nit: It would be helpful to simplify the BUG= line to 577431, or at least > include that first in the list. Good idea. Done.
On 2016/01/25 at 18:24:13, hongchan wrote: > On 2016/01/25 05:18:35, (out until Jan 5) dominicc wrote: > > Nit: It would be helpful to simplify the BUG= line to 577431, or at least > > include that first in the list. > > Good idea. Done. (The new BUG= line is not showing up for me.)
On 2016/02/09 at 08:09:09, (out until Jan 5) dominicc wrote: > On 2016/01/25 at 18:24:13, hongchan wrote: > > On 2016/01/25 05:18:35, (out until Jan 5) dominicc wrote: > > > Nit: It would be helpful to simplify the BUG= line to 577431, or at least > > > include that first in the list. > > > > Good idea. Done. > > (The new BUG= line is not showing up for me.) Ah, I see, you made it first in the list. Thanks.
On 2016/02/09 08:09:24, (out until Jan 5) dominicc wrote: > On 2016/02/09 at 08:09:09, (out until Jan 5) dominicc wrote: > > On 2016/01/25 at 18:24:13, hongchan wrote: > > > On 2016/01/25 05:18:35, (out until Jan 5) dominicc wrote: > > > > Nit: It would be helpful to simplify the BUG= line to 577431, or at least > > > > include that first in the list. > > > > > > Good idea. Done. > > > > (The new BUG= line is not showing up for me.) > > Ah, I see, you made it first in the list. Thanks. BTW, I'm about to land a CL to move hasPendingActivity from ActiveDOMObject to ScriptWrappable. https://codereview.chromium.org/1609343002
Description was changed from ========== AudioNode as ActiveDOMObject AudioNode has not equipped ActiveDOMObject, so ContextLifeCycleObserver does not pay attention to its instance. Thus |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep AudioNode from being collected when it still has a pending activity by making it observable to ContextLifeCycleObserver. BUG=577431,121654,484176,500335,559220,360378 TEST=LayoutTests/webaudio/audiosource-onended-gc.html ========== to ========== Add ScriptWrappable to SchduledSourceNode and ScriptProcessorNode |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep those nodes from being collected when they still has a pending activity. This can be done by adding ScriptWrappable.hasPendingActivity() to them. The change in ScriptWrappable is: https://codereview.chromium.org/1609343002/ BUG=577431,121654,484176,500335,559220,360378 TEST=LayoutTests/webaudio/audiosource-onended-gc.html ==========
haraken@ PTAL. I've changed ActiveDOMObject to ScriptWrappable, but all the attached tests are failing. Here are my thoughts: - AudioNode is EventTarget, which is already inherited from ScriptWrappable. Removing ActiveDOMObject was the only thing I've done from PS1. PS2 compiles okay, so I believe the inheritance relationship is correct. - Since I cannot use ActiveDOMObject::stop() anymore, I can only rely on ScriptWrappable::hasPendingActivity(). So I took a look at the https://codereview.chromium.org/1609343002 to find some examples, but all the customer class was using ActiveDOMObject as well. https://codereview.chromium.org/1609343002/diff/80001/third_party/WebKit/Sour... https://codereview.chromium.org/1609343002/diff/80001/third_party/WebKit/Sour... Could you take a look at PS2 and give me some hints on how to use the new change?
On 2016/02/10 19:21:46, hoch wrote: > haraken@ PTAL. > > I've changed ActiveDOMObject to ScriptWrappable, but all the attached tests are > failing. Here are my thoughts: > > - AudioNode is EventTarget, which is already inherited from ScriptWrappable. > Removing ActiveDOMObject was the only thing I've done from PS1. PS2 compiles > okay, so I believe the inheritance relationship is correct. > > - Since I cannot use ActiveDOMObject::stop() anymore, I can only rely on > ScriptWrappable::hasPendingActivity(). So I took a look at the > https://codereview.chromium.org/1609343002 to find some examples, but all the > customer class was using ActiveDOMObject as well. > > https://codereview.chromium.org/1609343002/diff/80001/third_party/WebKit/Sour... > https://codereview.chromium.org/1609343002/diff/80001/third_party/WebKit/Sour... > > Could you take a look at PS2 and give me some hints on how to use the new > change? Ah, if you need to take some action when ExecutionContext stops, you still need the ActiveDOMObject (and thus its stop()). ActiveDOMObject = A thing that sends notifications when the state of ExecutionContext changes. ScriptWrappable = A thing that provides a fundamental functionality for DOM wrappers.
On 2016/02/11 00:24:52, haraken wrote: > On 2016/02/10 19:21:46, hoch wrote: > > haraken@ PTAL. > > > > I've changed ActiveDOMObject to ScriptWrappable, but all the attached tests > are > > failing. Here are my thoughts: > > > > - AudioNode is EventTarget, which is already inherited from ScriptWrappable. > > Removing ActiveDOMObject was the only thing I've done from PS1. PS2 compiles > > okay, so I believe the inheritance relationship is correct. > > > > - Since I cannot use ActiveDOMObject::stop() anymore, I can only rely on > > ScriptWrappable::hasPendingActivity(). So I took a look at the > > https://codereview.chromium.org/1609343002 to find some examples, but all the > > customer class was using ActiveDOMObject as well. > > > > > https://codereview.chromium.org/1609343002/diff/80001/third_party/WebKit/Sour... > > > https://codereview.chromium.org/1609343002/diff/80001/third_party/WebKit/Sour... > > > > Could you take a look at PS2 and give me some hints on how to use the new > > change? > > Ah, if you need to take some action when ExecutionContext stops, you still need > the ActiveDOMObject (and thus its stop()). > > ActiveDOMObject = A thing that sends notifications when the state of > ExecutionContext changes. > ScriptWrappable = A thing that provides a fundamental functionality for DOM > wrappers. AudioContext is already an ActiveDOMObject. Wouldn't it be possible for the AudioNodes to know the ExecutionContext's state by looking up the state of the AudioContext? Then you don't need to make the AudioNodes be ActiveDOMObjects.
On 2016/02/11 01:03:41, haraken wrote: > On 2016/02/11 00:24:52, haraken wrote: > > On 2016/02/10 19:21:46, hoch wrote: > > > haraken@ PTAL. > > > > > > I've changed ActiveDOMObject to ScriptWrappable, but all the attached tests > > are > > > failing. Here are my thoughts: > > > > > > - AudioNode is EventTarget, which is already inherited from ScriptWrappable. > > > Removing ActiveDOMObject was the only thing I've done from PS1. PS2 compiles > > > okay, so I believe the inheritance relationship is correct. > > > > > > - Since I cannot use ActiveDOMObject::stop() anymore, I can only rely on > > > ScriptWrappable::hasPendingActivity(). So I took a look at the > > > https://codereview.chromium.org/1609343002 to find some examples, but all > the > > > customer class was using ActiveDOMObject as well. > > > > > > > > > https://codereview.chromium.org/1609343002/diff/80001/third_party/WebKit/Sour... > > > > > > https://codereview.chromium.org/1609343002/diff/80001/third_party/WebKit/Sour... > > > > > > Could you take a look at PS2 and give me some hints on how to use the new > > > change? > > > > Ah, if you need to take some action when ExecutionContext stops, you still > need > > the ActiveDOMObject (and thus its stop()). > > > > ActiveDOMObject = A thing that sends notifications when the state of > > ExecutionContext changes. > > ScriptWrappable = A thing that provides a fundamental functionality for DOM > > wrappers. > I naively thought AudioNode would protect itself from GC if it has a pending activity. Maybe I am doing something wrong, but now the test failure rate is almost 90%. (it was 100% pass with no flakiness before the ScriptWrappable change) The only visible difference was ScriptWrappable does not have stop() method, so I asked about it. The question here is: how do I mark an audio node as 'do not collect'? > AudioContext is already an ActiveDOMObject. Wouldn't it be possible for the > AudioNodes to know the ExecutionContext's state by looking up the state of the > AudioContext? Then you don't need to make the AudioNodes be ActiveDOMObjects. I am not sure how AudioNode looking up the EC's state can be helpful here. Each node has its own progress and state, so I don't think we can easily group them and batch process them. Are you suggesting this approach as a substitution of ActiveDOMObject::stop() method?
On 2016/02/11 16:35:43, hoch wrote: > On 2016/02/11 01:03:41, haraken wrote: > > On 2016/02/11 00:24:52, haraken wrote: > > > On 2016/02/10 19:21:46, hoch wrote: > > > > haraken@ PTAL. > > > > > > > > I've changed ActiveDOMObject to ScriptWrappable, but all the attached > tests > > > are > > > > failing. Here are my thoughts: > > > > > > > > - AudioNode is EventTarget, which is already inherited from > ScriptWrappable. > > > > Removing ActiveDOMObject was the only thing I've done from PS1. PS2 > compiles > > > > okay, so I believe the inheritance relationship is correct. > > > > > > > > - Since I cannot use ActiveDOMObject::stop() anymore, I can only rely on > > > > ScriptWrappable::hasPendingActivity(). So I took a look at the > > > > https://codereview.chromium.org/1609343002 to find some examples, but all > > the > > > > customer class was using ActiveDOMObject as well. > > > > > > > > > > > > > > https://codereview.chromium.org/1609343002/diff/80001/third_party/WebKit/Sour... > > > > > > > > > > https://codereview.chromium.org/1609343002/diff/80001/third_party/WebKit/Sour... > > > > > > > > Could you take a look at PS2 and give me some hints on how to use the new > > > > change? > > > > > > Ah, if you need to take some action when ExecutionContext stops, you still > > need > > > the ActiveDOMObject (and thus its stop()). > > > > > > ActiveDOMObject = A thing that sends notifications when the state of > > > ExecutionContext changes. > > > ScriptWrappable = A thing that provides a fundamental functionality for > DOM > > > wrappers. > > > > I naively thought AudioNode would protect itself from GC if it has a pending > activity. Maybe I am doing something wrong, but now the test failure rate is > almost 90%. (it was 100% pass with no flakiness before the ScriptWrappable > change) The only visible difference was ScriptWrappable does not have stop() > method, so I asked about it. > > The question here is: how do I mark an audio node as 'do not collect'? You can just return 'true' from hasPendingActivity. Then the wrapper will be kept alive. As far as I understand, your question is: "how can XXXAudioNode::hasPendingActivity return false when its associated context stopped?", right? My suggestion is to let the XXXAudioNode look up audioContext()->executionContext()->activeDOMObjectsAreStopped(). If that returns true, XXXAudioNode::hasPendingActivity should return false. Wouldn't it be doable? > > > AudioContext is already an ActiveDOMObject. Wouldn't it be possible for the > > AudioNodes to know the ExecutionContext's state by looking up the state of the > > AudioContext? Then you don't need to make the AudioNodes be ActiveDOMObjects. > > I am not sure how AudioNode looking up the EC's state can be helpful here. Each > node has its own progress and state, so I don't think we can easily group them > and batch process them. Are you suggesting this approach as a substitution of > ActiveDOMObject::stop() method?
> You can just return 'true' from hasPendingActivity. Then the wrapper will be > kept alive. This is not working currently; AudioScheduledSourceNode::hasPendingActivity() actually never gets called when I run the test. The audio nodes are silently being GCed. So I followed up to ScriptWrappable from AudioScheduledSourceNode: AudioScheduledSourceNode > AudioSourceNode > AudioNode > RefCountedGarbageCollectedEventTargetWithInlineData > EventTargetWithInlineData > EventTarget > ScriptWrappable I've checked AudioNode, which is the end point from WebAudio's perspective. I simply put 'fprintf' just to see if the method is called and AudioNode::hasPendingActivity() never gets called. > My suggestion is to let the XXXAudioNode look up > audioContext()->executionContext()->activeDOMObjectsAreStopped(). If that > returns true, XXXAudioNode::hasPendingActivity should return false. Wouldn't it > be doable? Yes, this would be useful if I have to do something when the node is going away. However, if hasPendingActivity() works out, it easily solves the current issue without using stop() method at all.
dominicc@chromium.org changed reviewers: - dominicc@chromium.org
On 2016/02/12 16:54:11, hoch wrote: > > You can just return 'true' from hasPendingActivity. Then the wrapper will be > > kept alive. > > This is not working currently; AudioScheduledSourceNode::hasPendingActivity() > actually never gets called when I run the test. The audio nodes are silently > being GCed. So I followed up to ScriptWrappable from AudioScheduledSourceNode: > > AudioScheduledSourceNode > AudioSourceNode > AudioNode > > RefCountedGarbageCollectedEventTargetWithInlineData > EventTargetWithInlineData > > EventTarget > ScriptWrappable > > I've checked AudioNode, which is the end point from WebAudio's perspective. I > simply put 'fprintf' just to see if the method is called and > AudioNode::hasPendingActivity() never gets called. That is because AudioScheduledSourceNode does not have a DOM wrapper (i.e., AudioScheduledSourceNode doesn't have an IDL file, AudioScheduledSourceNode doesn't have DEFINE_WRAPPERTYPE_INFO). AudioScheduledSourceNode inherits from AudioSourceNode, which has a DOM wrapper. So if you add hasPendingActivity to the AudioSourceNode, it will be called before the AudioSourceNode's wrapper gets collected. > > > My suggestion is to let the XXXAudioNode look up > > audioContext()->executionContext()->activeDOMObjectsAreStopped(). If that > > returns true, XXXAudioNode::hasPendingActivity should return false. Wouldn't > it > > be doable? > > Yes, this would be useful if I have to do something when the node is going away. > However, if hasPendingActivity() works out, it easily solves the current issue > without using stop() method at all.
> That is because AudioScheduledSourceNode does not have a DOM wrapper (i.e., > AudioScheduledSourceNode doesn't have an IDL file, AudioScheduledSourceNode > doesn't have DEFINE_WRAPPERTYPE_INFO). > > AudioScheduledSourceNode inherits from AudioSourceNode, which has a DOM wrapper. > So if you add hasPendingActivity to the AudioSourceNode, it will be called > before the AudioSourceNode's wrapper gets collected. Thanks for the info, but I locally confirmed that AudioNode::hasPendingActivity() - which has a complete IDL and DEFINE_WRAPPERTYEPINFO() - never gets called either. Started investigating all the connection from AudioNode up to ScriptWrappable, so happy to have any input.
On 2016/02/16 19:42:05, hoch wrote: > > That is because AudioScheduledSourceNode does not have a DOM wrapper (i.e., > > AudioScheduledSourceNode doesn't have an IDL file, AudioScheduledSourceNode > > doesn't have DEFINE_WRAPPERTYPE_INFO). > > > > AudioScheduledSourceNode inherits from AudioSourceNode, which has a DOM > wrapper. > > So if you add hasPendingActivity to the AudioSourceNode, it will be called > > before the AudioSourceNode's wrapper gets collected. > > Thanks for the info, but I locally confirmed that > AudioNode::hasPendingActivity() - which has a complete IDL and > DEFINE_WRAPPERTYEPINFO() - never gets called either. Started investigating all > the connection from AudioNode up to ScriptWrappable, so happy to have any input. That sounds strange -- hasPendingActivity should be called for all ScriptWrappables (see V8GCController.cpp: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
On 2016/02/16 19:47:20, haraken wrote: > On 2016/02/16 19:42:05, hoch wrote: > > > That is because AudioScheduledSourceNode does not have a DOM wrapper (i.e., > > > AudioScheduledSourceNode doesn't have an IDL file, AudioScheduledSourceNode > > > doesn't have DEFINE_WRAPPERTYPE_INFO). > > > > > > AudioScheduledSourceNode inherits from AudioSourceNode, which has a DOM > > wrapper. > > > So if you add hasPendingActivity to the AudioSourceNode, it will be called > > > before the AudioSourceNode's wrapper gets collected. > > > > Thanks for the info, but I locally confirmed that > > AudioNode::hasPendingActivity() - which has a complete IDL and > > DEFINE_WRAPPERTYEPINFO() - never gets called either. Started investigating all > > the connection from AudioNode up to ScriptWrappable, so happy to have any > input. > > That sounds strange -- hasPendingActivity should be called for all > ScriptWrappables (see V8GCController.cpp: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). Yes, just threw a fprintf into ScriptWrappable::hasPendingActivity and I could see all the printed messages, but the one I added to AudioNode::hasPendingActivity was not printed at all.
So I invoked hasPendingActivity() on an AudioNode manually in LLDB right after the instantiation, then actually ScriptWrappable::hasPendingActivity() gets called instead. In V8GCController, all the instances of hasPendingAcitivity() is being called upon the something returned by toScriptWrappable(). I know the current pattern of inheritance looks correct, but somehow toScriptWrappable(wrapper)->hasPendingAcitivity is calling the one from ScriptWrappable not AudioNode. Now I am thinking this might not be a AudioNode-specific problem. Perhaps future customers of ScriptWrappable::hasPendingActivity() might have the same issue.
haraken@chromium.org changed reviewers: + yukishiino@chromium.org
On 2016/02/17 22:40:08, hoch wrote: > So I invoked hasPendingActivity() on an AudioNode manually in LLDB right after > the instantiation, then actually ScriptWrappable::hasPendingActivity() gets > called instead. > > In V8GCController, all the instances of hasPendingAcitivity() is being called > upon the something returned by toScriptWrappable(). I know the current pattern > of inheritance looks correct, but somehow > toScriptWrappable(wrapper)->hasPendingAcitivity is calling the one from > ScriptWrappable not AudioNode. > > Now I am thinking this might not be a AudioNode-specific problem. Perhaps future > customers of ScriptWrappable::hasPendingActivity() might have the same issue. yukishiino@: Do you have any idea on this? The problem is that when V8GCController calls toScriptWrappable(audioNode)->hasPendingActivity(), ScriptWrappable::hasPendingActivity gets called instead of AudioNode::hasPendingActivity.
On 2016/02/18 10:06:55, haraken wrote: > On 2016/02/17 22:40:08, hoch wrote: > > So I invoked hasPendingActivity() on an AudioNode manually in LLDB right after > > the instantiation, then actually ScriptWrappable::hasPendingActivity() gets > > called instead. > > > > In V8GCController, all the instances of hasPendingAcitivity() is being called > > upon the something returned by toScriptWrappable(). I know the current pattern > > of inheritance looks correct, but somehow > > toScriptWrappable(wrapper)->hasPendingAcitivity is calling the one from > > ScriptWrappable not AudioNode. > > > > Now I am thinking this might not be a AudioNode-specific problem. Perhaps > future > > customers of ScriptWrappable::hasPendingActivity() might have the same issue. > > yukishiino@: Do you have any idea on this? The problem is that when > V8GCController calls toScriptWrappable(audioNode)->hasPendingActivity(), > ScriptWrappable::hasPendingActivity gets called instead of > AudioNode::hasPendingActivity. As I tested, when I called ScriptWrappable::hasPendingActivity(), the overriding function is called as expected. The compiler is working just fine. I'm not sure if you're really calling to ScriptWrappable::hasPendingActivity() for AudioNode or its subclasses. As I read the code, probably no one calls it for AudioNode in V8GCController. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... AudioNode is not HTMLImageElement, so not called. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... AudioNode is of ObjectClassId, so early returned on line 120. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... AudioNode is of Independent lifetime, so early returned on line 177. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... As I read a comment, this function seems called only for workers, right? At least I didn't observe this function called on NTP. This is my observation.
On 2016/02/18 13:04:37, Yuki wrote: > On 2016/02/18 10:06:55, haraken wrote: > > On 2016/02/17 22:40:08, hoch wrote: > > > So I invoked hasPendingActivity() on an AudioNode manually in LLDB right > after > > > the instantiation, then actually ScriptWrappable::hasPendingActivity() gets > > > called instead. > > > > > > In V8GCController, all the instances of hasPendingAcitivity() is being > called > > > upon the something returned by toScriptWrappable(). I know the current > pattern > > > of inheritance looks correct, but somehow > > > toScriptWrappable(wrapper)->hasPendingAcitivity is calling the one from > > > ScriptWrappable not AudioNode. > > > > > > Now I am thinking this might not be a AudioNode-specific problem. Perhaps > > future > > > customers of ScriptWrappable::hasPendingActivity() might have the same > issue. > > > > yukishiino@: Do you have any idea on this? The problem is that when > > V8GCController calls toScriptWrappable(audioNode)->hasPendingActivity(), > > ScriptWrappable::hasPendingActivity gets called instead of > > AudioNode::hasPendingActivity. > > As I tested, when I called ScriptWrappable::hasPendingActivity(), the overriding > function is called as expected. The compiler is working just fine. > > I'm not sure if you're really calling to ScriptWrappable::hasPendingActivity() > for AudioNode or its subclasses. As I read the code, probably no one calls it > for AudioNode in V8GCController. > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > AudioNode is not HTMLImageElement, so not called. > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > AudioNode is of ObjectClassId, so early returned on line 120. > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > AudioNode is of Independent lifetime, so early returned on line 177. > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > As I read a comment, this function seems called only for workers, right? At > least I didn't observe this function called on NTP. > > This is my observation. Gyah!! You need to add [DependentLifetime] to AudioNode.idl... Otherwise, V8GCController returns before checking its hasPendingAcitivity. Previously (i.e., when hasPendingActivity was a method of ActiveDOMObject) this was not an issue because [ActiveDOMObjet] implies [DependentLifetime]. Now that an interface that uses hasPendingActivity but is not an ActiveDOMObject needs to have [DependentLifetime]. Sorry about all the confusion... I'll improve the IDL compiler to detect this kind of mistake.
yukishino@ Thanks! Now hasPendingActivity() works correctly. haraken@ However, now I have a different issue: It is possible 1) to play a source node for a very long time or 2) to schedule too far from now. So when the document is tearing down (i.e. reloading) a node may report as 'hasPendingActivity' thus it does not get collected and leak. To avoid this problem, I added a little clean up routine in AbstractAudioContext::releaseActiveSourceNodes() and it nicely solved the leaking problem, but it causes the renderer crash in some layout tests. +webaudio/mediaelementaudiosourcenode-gc.html crash log sample stderr +webaudio/dynamicscompressor-basic.html crash log sample stderr +webaudio/mediaelementaudiosourcenode-wrapper.html crash log sample stderr +webaudio/scriptprocessornode-downmix8-2channel-input.html crash log sample stderr +webaudio/offlineaudiocontext-promise-basic.html crash log sample stderr +webaudio/mediastreamaudiodestinationnode.html crash log sample stderr +webaudio/scriptprocessornode.html crash log sample stderr +webaudio/scriptprocessornode-upmix2-8channel-input.html crash log sample stderr +webaudio/audionode-connect-order.html crash log sample stderr The following is a stack trace from dynamicscompressor-basic.html. Some crashes have the same stack trace as below or others have "signal 11 unknown". ASSERTION FAILED: result != largeObjectSizeInHeader ../../third_party/WebKit/Source/platform/heap/HeapPage.h(794) : size_t blink::HeapObjectHeader::size() const 1 0x10be30a75 blink::HeapObjectHeader::size() const 2 0x10be37400 blink::NormalPage::sweep() 3 0x10be323e4 blink::BaseHeap::sweepUnsweptPage() 4 0x10be32851 blink::BaseHeap::completeSweep() 5 0x10be40538 blink::ThreadState::completeSweep() 6 0x10be470ed blink::ThreadState::preSweep() 7 0x10be47c9c blink::ThreadState::leaveSafePoint(blink::SafePointAwareMutexLocker*) 8 0x10b8d255c blink::SafePointScope::~SafePointScope() 9 0x10b8d2363 blink::SafePointScope::~SafePointScope() 10 0x10be2ad16 blink::Heap::collectGarbage(blink::BlinkGC::StackState, blink::BlinkGC::GCType, blink::BlinkGC::GCReason) 11 0x10be44fb1 blink::ThreadState::runScheduledGC(blink::BlinkGC::StackState) 12 0x10be47978 blink::ThreadState::safePoint(blink::BlinkGC::StackState) 13 0x10d2c5c0a blink::GCTaskObserver::didProcessTask() 14 0x12091ec1f scheduler::WebThreadBase::TaskObserverAdapter::DidProcessTask(base::PendingTask const&) 15 0x1208e5068 scheduler::TaskQueueManager::ProcessTaskFromWorkQueue(scheduler::internal::WorkQueue*, scheduler::internal::TaskQueueImpl::Task*) 16 0x1208e19e0 scheduler::TaskQueueManager::DoWork(base::TimeTicks, bool) 17 0x1208eaf4a void base::internal::RunnableAdapter<void (scheduler::TaskQueueManager::*)(base::TimeTicks, bool)>::Run<base::TimeTicks const&, bool const&>(scheduler::TaskQueueManager*, base::TimeTicks const&&&, bool const&&&) 18 0x1208eacee void base::internal::InvokeHelper<true, void, base::internal::RunnableAdapter<void (scheduler::TaskQueueManager::*)(base::TimeTicks, bool)> >::MakeItSo<base::WeakPtr<scheduler::TaskQueueManager>, base::TimeTicks const&, bool const&>(base::internal::RunnableAdapter<void (scheduler::TaskQueueManager::*)(base::TimeTicks, bool)>, base::WeakPtr<scheduler::TaskQueueManager>, base::TimeTicks const&&&, bool const&&&) 19 0x1208eac4d base::internal::Invoker<base::IndexSequence<0ul, 1ul, 2ul>, base::internal::BindState<base::internal::RunnableAdapter<void (scheduler::TaskQueueManager::*)(base::TimeTicks, bool)>, void (scheduler::TaskQueueManager*, base::TimeTicks, bool), base::WeakPtr<scheduler::TaskQueueManager>, base::TimeTicks, bool>, base::internal::InvokeHelper<true, void, base::internal::RunnableAdapter<void (scheduler::TaskQueueManager::*)(base::TimeTicks, bool)> >, void ()>::Run(base::internal::BindStateBase*) 20 0x10b0dea2f base::Callback<void ()>::Run() const 21 0x10b109b4e base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask const&) 22 0x10b1d9000 base::MessageLoop::RunTask(base::PendingTask const&) 23 0x10b1d9636 base::MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) 24 0x10b1d9cbe base::MessageLoop::DoWork() 25 0x10b0b3cf8 base::MessagePumpCFRunLoopBase::RunWork() 26 0x10b0b3c6a invocation function for block in base::MessagePumpCFRunLoopBase::RunWorkSource(void*) 27 0x10b1970ba base::mac::CallWithEHFrame(void () block_pointer) 28 0x10b0b3005 base::MessagePumpCFRunLoopBase::RunWorkSource(void*) 29 0x7fff9a6d25c1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ 30 0x7fff9a6c441c __CFRunLoopDoSources0 31 0x7fff9a6c393f __CFRunLoopRun https://codereview.chromium.org/1593763002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp (right): https://codereview.chromium.org/1593763002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:797: // NOTE: this check makes the renderer crash. It is necessary to half the active source node. Otherwise the source node in the 'playing' or 'scheduled' state will leak. However, this check causes crashes in several layout tests. Not sure why.
On 2016/02/19 00:46:47, hoch wrote: > yukishino@ > Thanks! Now hasPendingActivity() works correctly. > > haraken@ > However, now I have a different issue: > It is possible 1) to play a source node for a very long time or 2) to schedule > too far from now. So when the document is tearing down (i.e. reloading) a node > may report as 'hasPendingActivity' thus it does not get collected and leak. To > avoid this problem, I added a little clean up routine in > AbstractAudioContext::releaseActiveSourceNodes() and it nicely solved the > leaking problem, but it causes the renderer crash in some layout tests. I think this is a right fix. hasPendingActivity must be written in a way that it returns false in a finite time period. Otherwise, the object will leak. > +webaudio/mediaelementaudiosourcenode-gc.html crash log sample stderr > +webaudio/dynamicscompressor-basic.html crash log sample stderr > +webaudio/mediaelementaudiosourcenode-wrapper.html crash log sample stderr > +webaudio/scriptprocessornode-downmix8-2channel-input.html crash log sample > stderr > +webaudio/offlineaudiocontext-promise-basic.html crash log sample stderr > +webaudio/mediastreamaudiodestinationnode.html crash log sample stderr > +webaudio/scriptprocessornode.html crash log sample stderr > +webaudio/scriptprocessornode-upmix2-8channel-input.html crash log sample stderr > +webaudio/audionode-connect-order.html crash log sample stderr > > The following is a stack trace from dynamicscompressor-basic.html. Some crashes > have the same stack trace as below or others have "signal 11 unknown". This looks strange. At a first glance, the CL looks correct to me... > > ASSERTION FAILED: result != largeObjectSizeInHeader > ../../third_party/WebKit/Source/platform/heap/HeapPage.h(794) : size_t > blink::HeapObjectHeader::size() const > 1 0x10be30a75 blink::HeapObjectHeader::size() const > 2 0x10be37400 blink::NormalPage::sweep() > 3 0x10be323e4 blink::BaseHeap::sweepUnsweptPage() > 4 0x10be32851 blink::BaseHeap::completeSweep() > 5 0x10be40538 blink::ThreadState::completeSweep() > 6 0x10be470ed blink::ThreadState::preSweep() > 7 0x10be47c9c > blink::ThreadState::leaveSafePoint(blink::SafePointAwareMutexLocker*) > 8 0x10b8d255c blink::SafePointScope::~SafePointScope() > 9 0x10b8d2363 blink::SafePointScope::~SafePointScope() > 10 0x10be2ad16 blink::Heap::collectGarbage(blink::BlinkGC::StackState, > blink::BlinkGC::GCType, blink::BlinkGC::GCReason) > 11 0x10be44fb1 blink::ThreadState::runScheduledGC(blink::BlinkGC::StackState) > 12 0x10be47978 blink::ThreadState::safePoint(blink::BlinkGC::StackState) > 13 0x10d2c5c0a blink::GCTaskObserver::didProcessTask() > 14 0x12091ec1f > scheduler::WebThreadBase::TaskObserverAdapter::DidProcessTask(base::PendingTask > const&) > 15 0x1208e5068 > scheduler::TaskQueueManager::ProcessTaskFromWorkQueue(scheduler::internal::WorkQueue*, > scheduler::internal::TaskQueueImpl::Task*) > 16 0x1208e19e0 scheduler::TaskQueueManager::DoWork(base::TimeTicks, bool) > 17 0x1208eaf4a void base::internal::RunnableAdapter<void > (scheduler::TaskQueueManager::*)(base::TimeTicks, bool)>::Run<base::TimeTicks > const&, bool const&>(scheduler::TaskQueueManager*, base::TimeTicks const&&&, > bool const&&&) > 18 0x1208eacee void base::internal::InvokeHelper<true, void, > base::internal::RunnableAdapter<void > (scheduler::TaskQueueManager::*)(base::TimeTicks, bool)> > >::MakeItSo<base::WeakPtr<scheduler::TaskQueueManager>, base::TimeTicks const&, > bool const&>(base::internal::RunnableAdapter<void > (scheduler::TaskQueueManager::*)(base::TimeTicks, bool)>, > base::WeakPtr<scheduler::TaskQueueManager>, base::TimeTicks const&&&, bool > const&&&) > 19 0x1208eac4d base::internal::Invoker<base::IndexSequence<0ul, 1ul, 2ul>, > base::internal::BindState<base::internal::RunnableAdapter<void > (scheduler::TaskQueueManager::*)(base::TimeTicks, bool)>, void > (scheduler::TaskQueueManager*, base::TimeTicks, bool), > base::WeakPtr<scheduler::TaskQueueManager>, base::TimeTicks, bool>, > base::internal::InvokeHelper<true, void, base::internal::RunnableAdapter<void > (scheduler::TaskQueueManager::*)(base::TimeTicks, bool)> >, void > ()>::Run(base::internal::BindStateBase*) > 20 0x10b0dea2f base::Callback<void ()>::Run() const > 21 0x10b109b4e base::debug::TaskAnnotator::RunTask(char const*, > base::PendingTask const&) > 22 0x10b1d9000 base::MessageLoop::RunTask(base::PendingTask const&) > 23 0x10b1d9636 base::MessageLoop::DeferOrRunPendingTask(base::PendingTask > const&) > 24 0x10b1d9cbe base::MessageLoop::DoWork() > 25 0x10b0b3cf8 base::MessagePumpCFRunLoopBase::RunWork() > 26 0x10b0b3c6a invocation function for block in > base::MessagePumpCFRunLoopBase::RunWorkSource(void*) > 27 0x10b1970ba base::mac::CallWithEHFrame(void () block_pointer) > 28 0x10b0b3005 base::MessagePumpCFRunLoopBase::RunWorkSource(void*) > 29 0x7fff9a6d25c1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ > 30 0x7fff9a6c441c __CFRunLoopDoSources0 > 31 0x7fff9a6c393f __CFRunLoopRun > > https://codereview.chromium.org/1593763002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp > (right): > > https://codereview.chromium.org/1593763002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:797: // > NOTE: this check makes the renderer crash. > It is necessary to half the active source node. Otherwise the source node in the > 'playing' or 'scheduled' state will leak. However, this check causes crashes in > several layout tests. Not sure why.
PTAL. I fixed a (stupid) bug in AbstractAudioContext::releaseActiveSourceNodes(). Now all the layout tests are passing without leaking. Also rtoy@ and I decided to separate ScriptProcessorNode out of this CL, because we have to make decision on its lifetime.
Description was changed from ========== Add ScriptWrappable to SchduledSourceNode and ScriptProcessorNode |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep those nodes from being collected when they still has a pending activity. This can be done by adding ScriptWrappable.hasPendingActivity() to them. The change in ScriptWrappable is: https://codereview.chromium.org/1609343002/ BUG=577431,121654,484176,500335,559220,360378 TEST=LayoutTests/webaudio/audiosource-onended-gc.html ========== to ========== Add ScriptWrappable to SchduledSourceNode and ScriptProcessorNode |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep those nodes from being collected when they still has a pending activity. This can be done by adding ScriptWrappable.hasPendingActivity() to them. The change in ScriptWrappable is: https://codereview.chromium.org/1609343002/ BUG=577431,484176,559220 TEST=LayoutTests/webaudio/audiosource-onended-gc.html ==========
https://codereview.chromium.org/1593763002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp (right): https://codereview.chromium.org/1593763002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp:279: if (m_isMarkedForGC) Instead of introducing m_isMarkedForGC, can we do something like: if (context()->isStopped()) return; ? Then you don't need to propagate markedForGC to all AudioNodes. Also it is common to check the status of ExecutionContext (or its friends) in hasPendingActivity.
Using context()->isContextClosed() makes more sense because all the node derived from a context should be stopped and collected when the context is gone. Still (locally) passing all the layout tests without leaking. https://codereview.chromium.org/1593763002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp (right): https://codereview.chromium.org/1593763002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp:279: if (m_isMarkedForGC) On 2016/02/19 17:46:00, haraken wrote: > > Instead of introducing m_isMarkedForGC, can we do something like: > > if (context()->isStopped()) > return; > > ? > > Then you don't need to propagate markedForGC to all AudioNodes. Also it is > common to check the status of ExecutionContext (or its friends) in > hasPendingActivity. Good idea. Done.
On 2016/02/19 18:13:49, hoch wrote: > Using context()->isContextClosed() makes more sense because all the node derived > from a context should be stopped and collected when the context is gone. > > Still (locally) passing all the layout tests without leaking. > > https://codereview.chromium.org/1593763002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp > (right): > > https://codereview.chromium.org/1593763002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp:279: if > (m_isMarkedForGC) > On 2016/02/19 17:46:00, haraken wrote: > > > > Instead of introducing m_isMarkedForGC, can we do something like: > > > > if (context()->isStopped()) > > return; > > > > ? > > > > Then you don't need to propagate markedForGC to all AudioNodes. Also it is > > common to check the status of ExecutionContext (or its friends) in > > hasPendingActivity. > > Good idea. Done. LGTM
Description was changed from ========== Add ScriptWrappable to SchduledSourceNode and ScriptProcessorNode |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep those nodes from being collected when they still has a pending activity. This can be done by adding ScriptWrappable.hasPendingActivity() to them. The change in ScriptWrappable is: https://codereview.chromium.org/1609343002/ BUG=577431,484176,559220 TEST=LayoutTests/webaudio/audiosource-onended-gc.html ========== to ========== Add ScriptWrappable to SchduledSourceNode and ScriptProcessorNode |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep those nodes from being collected when they still has a pending activity. This can be done by adding ScriptWrappable.hasPendingActivity() to them. The change in ScriptWrappable is: https://codereview.chromium.org/1609343002/ BUG=577431,484176,559220 TEST=LayoutTests/webaudio/audiosource-premature-gc.html ==========
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1593763002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1593763002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL - rtoy@ WebAudio changes. yukishiino@ Usage of ScriptWrappable::hasPendingActivity().
Does the premature-gc test fail without your fix? https://codereview.chromium.org/1593763002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/audiosource-premature-gc.html (right): https://codereview.chromium.org/1593763002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiosource-premature-gc.html:37: var dummy = context.createBuffer(1, 44100 * 1.5, 44100); The oscillator test runs for 1 sec. Any particular reason the buffer source is 1.5 sec long? https://codereview.chromium.org/1593763002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiosource-premature-gc.html:52: context.suspend(0.1).then(function () { Why perform a GC after 0.1 sec? Should you try a GC at 1+ or 1.5+ sec too? https://codereview.chromium.org/1593763002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.idl (right): https://codereview.chromium.org/1593763002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.idl:37: [RaisesException, ImplementedAs=stopNode] void stop(optional double when); Why are these methods renamed? https://codereview.chromium.org/1593763002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp (right): https://codereview.chromium.org/1593763002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp:271: // still returns true. Don't think this comment really adds anything. I think the comment at line 173+ pretty much explains why you're returning false. https://codereview.chromium.org/1593763002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp:279: // even its reference is out of scope. (and fire onended event if assigned) Typo: "(and fire" -> "And fire".
https://codereview.chromium.org/1593763002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/audiosource-premature-gc.html (right): https://codereview.chromium.org/1593763002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiosource-premature-gc.html:37: var dummy = context.createBuffer(1, 44100 * 1.5, 44100); On 2016/02/22 17:43:51, Raymond Toy wrote: > The oscillator test runs for 1 sec. Any particular reason the buffer source is > 1.5 sec long? None. I'll make it same. https://codereview.chromium.org/1593763002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiosource-premature-gc.html:52: context.suspend(0.1).then(function () { On 2016/02/22 17:43:51, Raymond Toy wrote: > Why perform a GC after 0.1 sec? Should you try a GC at 1+ or 1.5+ sec too? 0.1 is just an arbitrary time position after the sources are scheduled. Any time before the total render duration will work. I do not think it is necessary to try more GCs. https://codereview.chromium.org/1593763002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.idl (right): https://codereview.chromium.org/1593763002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.idl:37: [RaisesException, ImplementedAs=stopNode] void stop(optional double when); On 2016/02/22 17:43:51, Raymond Toy wrote: > Why are these methods renamed? Oh, I will fix this, but it was because ActiveDOMObject::stop(). https://codereview.chromium.org/1593763002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp (right): https://codereview.chromium.org/1593763002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp:271: // still returns true. On 2016/02/22 17:43:51, Raymond Toy wrote: > Don't think this comment really adds anything. I think the comment at line 173+ > pretty much explains why you're returning false. Where is the comment at line 173? https://codereview.chromium.org/1593763002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp:279: // even its reference is out of scope. (and fire onended event if assigned) On 2016/02/22 17:43:51, Raymond Toy wrote: > Typo: "(and fire" -> "And fire". Done.
https://codereview.chromium.org/1593763002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp (right): https://codereview.chromium.org/1593763002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioScheduledSourceNode.cpp:271: // still returns true. On 2016/02/23 18:01:33, hoch wrote: > On 2016/02/22 17:43:51, Raymond Toy wrote: > > Don't think this comment really adds anything. I think the comment at line > 173+ > > pretty much explains why you're returning false. > > Where is the comment at line 173? Oops. 273, the lines below.
lgtm
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1593763002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1593763002/120001
FYI: ToT fails on the new test 100% out of 10 consecutive trials. (ran the layout test suite with webaudio/audiosource-premature-gc.html --iteration=10) ToT with this CL passes the test without any flakiness.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by hongchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1593763002/#ps120001 (title: "Simplify changes (startNode > start)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1593763002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1593763002/120001
Message was sent while issue was closed.
Description was changed from ========== Add ScriptWrappable to SchduledSourceNode and ScriptProcessorNode |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep those nodes from being collected when they still has a pending activity. This can be done by adding ScriptWrappable.hasPendingActivity() to them. The change in ScriptWrappable is: https://codereview.chromium.org/1609343002/ BUG=577431,484176,559220 TEST=LayoutTests/webaudio/audiosource-premature-gc.html ========== to ========== Add ScriptWrappable to SchduledSourceNode and ScriptProcessorNode |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep those nodes from being collected when they still has a pending activity. This can be done by adding ScriptWrappable.hasPendingActivity() to them. The change in ScriptWrappable is: https://codereview.chromium.org/1609343002/ BUG=577431,484176,559220 TEST=LayoutTests/webaudio/audiosource-premature-gc.html ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add ScriptWrappable to SchduledSourceNode and ScriptProcessorNode |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep those nodes from being collected when they still has a pending activity. This can be done by adding ScriptWrappable.hasPendingActivity() to them. The change in ScriptWrappable is: https://codereview.chromium.org/1609343002/ BUG=577431,484176,559220 TEST=LayoutTests/webaudio/audiosource-premature-gc.html ========== to ========== Add ScriptWrappable to SchduledSourceNode and ScriptProcessorNode |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep those nodes from being collected when they still has a pending activity. This can be done by adding ScriptWrappable.hasPendingActivity() to them. The change in ScriptWrappable is: https://codereview.chromium.org/1609343002/ BUG=577431,484176,559220 TEST=LayoutTests/webaudio/audiosource-premature-gc.html Committed: https://crrev.com/cfa51385f9a17df78576c607827910f91f4c6683 Cr-Commit-Position: refs/heads/master@{#377231} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/cfa51385f9a17df78576c607827910f91f4c6683 Cr-Commit-Position: refs/heads/master@{#377231}
Message was sent while issue was closed.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
Message was sent while issue was closed.
https://codereview.chromium.org/1593763002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/audiosource-premature-gc.html (right): https://codereview.chromium.org/1593763002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiosource-premature-gc.html:22: (function () { There's an output ordering issue here, the delivery order of |ended| to Oscillator and BufferSource isn't guaranteed. See, https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_32/... and other bots failing.
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:120001) has been created in https://codereview.chromium.org/1727333002/ by hongchan@chromium.org. The reason for reverting is: There is an ordering issue in the layout test. Will fix it and retry..
Message was sent while issue was closed.
Description was changed from ========== Add ScriptWrappable to SchduledSourceNode and ScriptProcessorNode |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep those nodes from being collected when they still has a pending activity. This can be done by adding ScriptWrappable.hasPendingActivity() to them. The change in ScriptWrappable is: https://codereview.chromium.org/1609343002/ BUG=577431,484176,559220 TEST=LayoutTests/webaudio/audiosource-premature-gc.html Committed: https://crrev.com/cfa51385f9a17df78576c607827910f91f4c6683 Cr-Commit-Position: refs/heads/master@{#377231} ========== to ========== Add ScriptWrappable to SchduledSourceNode and ScriptProcessorNode |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep those nodes from being collected when they still has a pending activity. This can be done by adding ScriptWrappable.hasPendingActivity() to them. The change in ScriptWrappable is: https://codereview.chromium.org/1609343002/ BUG=577431,484176,559220 TEST=LayoutTests/webaudio/audiosource-premature-gc.html Committed: https://crrev.com/cfa51385f9a17df78576c607827910f91f4c6683 Cr-Commit-Position: refs/heads/master@{#377231} ==========
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1593763002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1593763002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1593763002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1593763002/140001
rtoy@ sof@ PTAL. I've rewritten the layout test to resolve the ordering issue.
thanks for test rewrite - lgtm as far as i'm concerned. https://codereview.chromium.org/1593763002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/audiosource-premature-gc.html (right): https://codereview.chromium.org/1593763002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiosource-premature-gc.html:85: succesfullyParsed = true; nit: do you need this?
https://codereview.chromium.org/1593763002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/audiosource-premature-gc.html (right): https://codereview.chromium.org/1593763002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiosource-premature-gc.html:36: let dummy = context.createBuffer(1, sampleRate, sampleRate); Pick a more descriptive name than "dummy". https://codereview.chromium.org/1593763002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiosource-premature-gc.html:48: finishJSTest(); Why finishJSTest() here? Isn't the one in the "finish" task below enough? https://codereview.chromium.org/1593763002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiosource-premature-gc.html:49: }; How is node2.onended supposed to work? node2 isn't started, so the onended should never get called, right?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Thanks all for the review! https://codereview.chromium.org/1593763002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/audiosource-premature-gc.html (right): https://codereview.chromium.org/1593763002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiosource-premature-gc.html:36: let dummy = context.createBuffer(1, sampleRate, sampleRate); On 2016/02/25 16:34:56, Raymond Toy wrote: > Pick a more descriptive name than "dummy". Done. https://codereview.chromium.org/1593763002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiosource-premature-gc.html:48: finishJSTest(); On 2016/02/25 16:34:56, Raymond Toy wrote: > Why finishJSTest() here? Isn't the one in the "finish" task below enough? I will remove this. https://codereview.chromium.org/1593763002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiosource-premature-gc.html:49: }; On 2016/02/25 16:34:56, Raymond Toy wrote: > How is node2.onended supposed to work? node2 isn't started, so the onended > should never get called, right? Removing. https://codereview.chromium.org/1593763002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiosource-premature-gc.html:85: succesfullyParsed = true; On 2016/02/25 16:31:55, sof wrote: > nit: do you need this? All other layout tests of WebAudio has this. I think this is a good signal of 'the code itself is successfully parsed until EOF.'
https://codereview.chromium.org/1593763002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/audiosource-premature-gc.html (right): https://codereview.chromium.org/1593763002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/audiosource-premature-gc.html:85: succesfullyParsed = true; On 2016/02/25 18:00:36, hoch wrote: > On 2016/02/25 16:31:55, sof wrote: > > nit: do you need this? > > All other layout tests of WebAudio has this. I think this is a good signal of > 'the code itself is successfully parsed until EOF.' I don't think it's needed. The most recently added tests don't have this.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1593763002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1593763002/160001
lgtm
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hongchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yukishiino@chromium.org, sigbjornf@opera.com Link to the patchset: https://codereview.chromium.org/1593763002/#ps160001 (title: "Cleaning up test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1593763002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1593763002/160001
Message was sent while issue was closed.
Description was changed from ========== Add ScriptWrappable to SchduledSourceNode and ScriptProcessorNode |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep those nodes from being collected when they still has a pending activity. This can be done by adding ScriptWrappable.hasPendingActivity() to them. The change in ScriptWrappable is: https://codereview.chromium.org/1609343002/ BUG=577431,484176,559220 TEST=LayoutTests/webaudio/audiosource-premature-gc.html Committed: https://crrev.com/cfa51385f9a17df78576c607827910f91f4c6683 Cr-Commit-Position: refs/heads/master@{#377231} ========== to ========== Add ScriptWrappable to SchduledSourceNode and ScriptProcessorNode |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep those nodes from being collected when they still has a pending activity. This can be done by adding ScriptWrappable.hasPendingActivity() to them. The change in ScriptWrappable is: https://codereview.chromium.org/1609343002/ BUG=577431,484176,559220 TEST=LayoutTests/webaudio/audiosource-premature-gc.html Committed: https://crrev.com/cfa51385f9a17df78576c607827910f91f4c6683 Cr-Commit-Position: refs/heads/master@{#377231} ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Add ScriptWrappable to SchduledSourceNode and ScriptProcessorNode |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep those nodes from being collected when they still has a pending activity. This can be done by adding ScriptWrappable.hasPendingActivity() to them. The change in ScriptWrappable is: https://codereview.chromium.org/1609343002/ BUG=577431,484176,559220 TEST=LayoutTests/webaudio/audiosource-premature-gc.html Committed: https://crrev.com/cfa51385f9a17df78576c607827910f91f4c6683 Cr-Commit-Position: refs/heads/master@{#377231} ========== to ========== Add ScriptWrappable to SchduledSourceNode and ScriptProcessorNode |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep those nodes from being collected when they still has a pending activity. This can be done by adding ScriptWrappable.hasPendingActivity() to them. The change in ScriptWrappable is: https://codereview.chromium.org/1609343002/ BUG=577431,484176,559220 TEST=LayoutTests/webaudio/audiosource-premature-gc.html Committed: https://crrev.com/cfa51385f9a17df78576c607827910f91f4c6683 Cr-Commit-Position: refs/heads/master@{#377231} Committed: https://crrev.com/97be0f5a7309a7da5c7da95c637eda34236f25f2 Cr-Commit-Position: refs/heads/master@{#377783} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/97be0f5a7309a7da5c7da95c637eda34236f25f2 Cr-Commit-Position: refs/heads/master@{#377783}
Message was sent while issue was closed.
Description was changed from ========== Add ScriptWrappable to SchduledSourceNode and ScriptProcessorNode |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep those nodes from being collected when they still has a pending activity. This can be done by adding ScriptWrappable.hasPendingActivity() to them. The change in ScriptWrappable is: https://codereview.chromium.org/1609343002/ BUG=577431,484176,559220 TEST=LayoutTests/webaudio/audiosource-premature-gc.html Committed: https://crrev.com/cfa51385f9a17df78576c607827910f91f4c6683 Cr-Commit-Position: refs/heads/master@{#377231} Committed: https://crrev.com/97be0f5a7309a7da5c7da95c637eda34236f25f2 Cr-Commit-Position: refs/heads/master@{#377783} ========== to ========== Add ScriptWrappable to SchduledSourceNode and ScriptProcessorNode |onended| event in OscillatorNode, AudioBufferSourceNode and |onaudioprocess| event in ScriptProcessor node are not fired once the JS reference of the node goes out of local scope. This CL is to keep those nodes from being collected when they still has a pending activity. This can be done by adding ScriptWrappable.hasPendingActivity() to them. The change in ScriptWrappable is: https://codereview.chromium.org/1609343002/ BUG=577431,484176,559220 TEST=LayoutTests/webaudio/audiosource-premature-gc.html Committed: https://crrev.com/cfa51385f9a17df78576c607827910f91f4c6683 Cr-Commit-Position: refs/heads/master@{#377231} Committed: https://crrev.com/97be0f5a7309a7da5c7da95c637eda34236f25f2 Cr-Commit-Position: refs/heads/master@{#377783} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
