1210267 - simplify AudioNode/AudioParam custom final release

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P2)

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 1210267 use DOMEventTargetHelper::LastRelease instead of custom Release r?baku

bug 1210267 use DOMEventTargetHelper::LastRelease instead of custom Release r?baku

AudioNode already has NS_IMPL_CYCLE_COLLECTING_RELEASE_WITH_LAST_RELEASE,
which performs mRefCnt.incr/decr for LastRelease(), so the kungFuDeathGrip
virtual AddRef/Release in DisconnectFromGraph() is additional noise when
debugging/tracing ownership.

Unlink() already assumes that the caller holds a reference (and it does).

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 1210267 remove custom AudioParam::Release and disconnect in destructor r?baku

bug 1210267 remove custom AudioParam::Release and disconnect in destructor r?baku

AudioParam has no derived type (and DisconnectFromGraphAndDestroyStream calls
no virtual functions) so no need for special release.

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8668254 [details]
MozReview Request: bug 1210267 remove custom AudioParam::Release and disconnect in destructor r?baku

https://reviewboard.mozilla.org/r/20939/#review18831

::: dom/media/webaudio/AudioParam.cpp:62
(Diff revision 1)
> -  // Addref this temporarily so the refcount bumping below doesn't destroy us
> +  MOZ_ASSERT(mRefCnt.get() > mInputNodes.Length(),

I don't understand why this assertion can work.
This method is called in the DTOR of this class: why mRefCnt is > 0 ?

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8668253 [details]
MozReview Request: bug 1210267 use DOMEventTargetHelper::LastRelease instead of custom Release r?baku

https://reviewboard.mozilla.org/r/20937/#review18833

The first patch looks good. Thanks for this simplification.

Comment 5

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/20939/#review18831

> I don't understand why this assertion can work.
> This method is called in the DTOR of this class: why mRefCnt is > 0 ?

The ref-count is 1 in the destructor because stabilizeForDeletion() is called
before DeleteCycleCollectable().

https://hg.mozilla.org/mozilla-central/annotate/2c1fb007137dcb68b1862a79553b53f1a34c99c3/xpcom/base/nsCycleCollector.cpp#l2653

The comment on the assertion was actually a little wrong.
I've corrected that.

The input disconnections part of this method is only relevant when this is
called from Unlink, but I have longer term wish to make those upstream
connections weak for bug 897796, in which case disconnection will also be
required when deleted without Unlink().

Comment 6

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cabae5c89ab

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8668254 [details]
MozReview Request: bug 1210267 remove custom AudioParam::Release and disconnect in destructor r?baku

https://reviewboard.mozilla.org/r/20939/#review18963

Comment 8

[Maire Reavy [:mreavy]]

Shall we land this?

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d941427498c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fa379806dd4

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d941427498c7
https://hg.mozilla.org/mozilla-central/rev/9fa379806dd4