Remove AudioNode::isMarkedForDeletion from oilpan builds

Remove AudioNode::isMarkedForDeletion from oilpan builds

AudioNode::isMarkedForDeletion is not needed any more in oilpan builds
for the following (non-trivial) reason:

0 isMarkedForDeletion is set to true in AudioNode::dispose().
In oilpan builds, AudioNode::dispose() is called after the AudioNode
becomes unreachable. In other words, isMarkedForDeletion is set to true
after the AudioNode becomes unreachable.

- The only user of isMarkedForDeletion is AudioNodeInput::canUpdateState().
AudioNodeInput::canUpdateState() returns false if isMarkedForDeletion is true.

- AudioNodeInput has a strong reference to the AudioNode.

These facts indicate that AudioNodeInput::canUpdateState() always returns true. Thus this CL removes the canUpdateState() checks from the code base. This also enables us to remove AudioNode::isMarkedForDeletion.

BUG=340522
NOTRY=true

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

[haraken, 2014-08-11 07:21:58]

PTAL

[zerny-chromium, 2014-08-11 07:43:48]

On 2014/08/11 07:21:58, haraken wrote:
> These facts indicate that AudioNodeInput::canUpdateState() always returns
true.

Or both the AudioNodeInput and the AudioNode are unreachable. No? Can
isMarkedForDeletion be needed as part of any clean-up/finalization code? If so,
we still can't remove it. (Reviewing the CL now).

[haraken, 2014-08-11 07:51:28]

On 2014/08/11 07:43:48, zerny-chromium wrote:
> On 2014/08/11 07:21:58, haraken wrote:
> > These facts indicate that AudioNodeInput::canUpdateState() always returns
> true.
> 
> Or both the AudioNodeInput and the AudioNode are unreachable. No? Can
> isMarkedForDeletion be needed as part of any clean-up/finalization code? If
so,
> we still can't remove it. (Reviewing the CL now).

A quick answer is: canUpdateState() is used only in
AudioSummingJunction::changedOutputs() and
AudioSummingJunction::updateRenderingState(). They can't be called in
destructors, as far as I understand.

[zerny-chromium, 2014-08-11 07:53:28]

The change looks good, but I'd think someone that knows the code base (eg, tkent
or rtoy) should sign it off.

https://codereview.chromium.org/458923003/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioSummingJunction.cpp (right):

https://codereview.chromium.org/458923003/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioSummingJunction.cpp:60: void
AudioSummingJunction::changedOutputs()
I'm not sure if this can be called from a code path after the node has become
unreachable. For example, |AudioNodeInput::disconnect| calls changedOutputs and
sounds like a method that might be called during weak-processing/finalization (I
can't see any indication that it is, but it is hard to verify that nothing
transitively hits canUpdateState from weak-processing/finalization).

[zerny-chromium, 2014-08-11 07:54:11]

On 2014/08/11 07:51:28, haraken wrote:
> On 2014/08/11 07:43:48, zerny-chromium wrote:
> > On 2014/08/11 07:21:58, haraken wrote:
> > > These facts indicate that AudioNodeInput::canUpdateState() always returns
> > true.
> > 
> > Or both the AudioNodeInput and the AudioNode are unreachable. No? Can
> > isMarkedForDeletion be needed as part of any clean-up/finalization code? If
> so,
> > we still can't remove it. (Reviewing the CL now).
> 
> A quick answer is: canUpdateState() is used only in
> AudioSummingJunction::changedOutputs() and
> AudioSummingJunction::updateRenderingState(). They can't be called in
> destructors, as far as I understand.

OK. I could not find any problematic cases either (but it is really hard to rule
them out).

[tkent, 2014-08-11 10:05:49]

lgtm.
I also think it's safe to remove.

[haraken, 2014-08-11 10:28:20]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-08-11 10:29:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/458923003/20001

[commit-bot: I haz the power, 2014-08-11 11:30:55]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22023)

[commit-bot: I haz the power, 2014-08-11 12:41:18]

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

[commit-bot: I haz the power, 2014-08-11 12:41:19]

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

[haraken, 2014-08-12 05:49:09]

The CQ bit was checked by haraken@chromium.org

[haraken, 2014-08-12 05:49:39]

The CQ bit was unchecked by haraken@chromium.org

[haraken, 2014-08-12 05:49:51]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-08-12 05:50:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/458923003/20001

[commit-bot: I haz the power, 2014-08-12 05:51:21]

Change committed as 180032