927322 - AudioDestinationNode should support weak reference

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[JW Wang [:jwwang] [:jw_wang]]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/28.0.1500.95 Safari/537.36

Steps to reproduce:

For AudioChannelAgent::InitWithWeakCallback to work correctly.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: support weak reference

Comment 2

[(no longer active)]

Hmm, how did things work without this?

Comment 3

[(no longer active)]

(And should this be backported to 26?)

Comment 4

[Paul Adenot (:padenot)]

According to bug 927347, it doesn't work that well, apparently. Maybe it is related.

Comment 5

[Andrea Marchesini [:baku]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> Hmm, how did things work without this?

Good question. It the beginning I didn't use InitWithWeakCallback, but just Init. I change that at least, and there was a mochitest for it. My fault.

Comment 6

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/899981761c7d

Comment 8

[Jason Smith [:jsmith]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #3)
> (And should this be backported to 26?)

I think so. See my comment on the dupe - I think this needs to block 1.2.

Comment 9

[(no longer active)]

(In reply to Jason Smith [:jsmith] from comment #8)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #3)
> > (And should this be backported to 26?)
> 
> I think so. See my comment on the dupe - I think this needs to block 1.2.

Sounds good to me!

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/899981761c7d

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/014c984c7d51

(No need for c-n on koi+, fwiw)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G crashtest crashes.
https://hg.mozilla.org/releases/mozilla-aurora/rev/026d6795f97d

https://tbpl.mozilla.org/php/getParsedLog.php?id=29315475&tree=Mozilla-Aurora

Comment 13

[(no longer active)]

JW, can you please provide a branch patch?  Thanks!

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

Since Mozilla-Aurora automatically pulls changes from Mozilla-central, how could I provide a patch for Mozilla-Aurora directly?

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=29428345&tree=Try#error2
https://tbpl.mozilla.org/php/getParsedLog.php?id=29427028&tree=Mozilla-Aurora#error2

It looks like something wrong with queryInterface() stuff and the <this> pointer is invalid in AudioDestinationNode::CanPlayChanged(). I am debugging this issue.

It would help if r0 ~ r3 is provided in the stack trace.

Comment 16

[(no longer active)]

(In reply to comment #14)
> Since Mozilla-Aurora automatically pulls changes from Mozilla-central, how
> could I provide a patch for Mozilla-Aurora directly?

Aurora doesn't automatically pull changes from central.  If you are using hg, you can clone <http://hg.mozilla.org/releases/mozilla-aurora> in order to debug this locally.  If you are using git, you can use the aurora branch on <https://github.com/mozilla/mozilla-central>.

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

Attachment: patch for mozilla-aurora

#0  mozilla::dom::AudioDestinationNode::DestroyMediaStream (this=0x4427f580)
#1  0x40b96f1e in mozilla::dom::AudioNode::DisconnectFromGraph (this=0x4427f580)
#2  0x40b96b22 in mozilla::dom::AudioNode::Release (this=0x4427f580)
#3  0x40b86d56 in mozilla::dom::AnalyserNode::Release (this=0x4427f580)
#4  0x411d98ba in ~nsCOMPtr_base (aInstancePtr=<value optimized out>, aErrorPtr=0x0)
#5  ~nsCOMPtr (aInstancePtr=<value optimized out>, aErrorPtr=0x0)
#6  NS_GetWeakReference (aInstancePtr=<value optimized out>, aErrorPtr=0x0)
#7  0x40c75390 in do_GetWeakReference (aRawPtr=0x4427f5c8, error=0x0)
#8  0x40c7580e in mozilla::dom::AudioChannelAgent::InitInternal (this=0x442be480, aChannelType=0, aCallback=0x4427f5c8, aUseWeakRef=true, aWithVideo=false)
#9  0x40c7578a in mozilla::dom::AudioChannelAgent::InitWithWeakCallback (this=0x442be480, channelType=0, callback=0x4427f5c8)
#10 0x40b93fac in AudioDestinationNode (this=0x4427f580, aContext=0x441c7860, aIsOffline=false, aNumberOfChannels=0, aLength=0, aSampleRate=0)
#11 0x40b8f2d0 in AudioContext (this=0x441c7860, aWindow=0x404c91d0, aIsOffline=false, aNumberOfChannels=0, aLength=0, aSampleRate=0)
#12 0x40b8f568 in mozilla::dom::AudioContext::Constructor (aGlobal=..., aRv=...)

The root cause is AudioDestinationNode get destroyed during construction for do_GetWeakReference() will change the reference count from 0 to 1 (do_QueryInterface) and then 1 to 0 (destruction of factoryPtr in NS_GetWeakReference).

Mozilla-Central happens to work around this subtle bug by calling target->AddSystemEventListener() in the AudioDestinationNode constructor so that reference count will not drop to zero before RAII is finished. (mDestination = new AudioDestinationNode)

Also, I phase in the patch in bug 923517 for AddSystemEventListener() to work correctly in AudioDestinationNode constructor.

Comment 18

[JW Wang [:jwwang] [:jw_wang]]

It looks like a bad idea to pass the this pointer elsewhere during construction for the object is not yet full-fledged. I am also thinking why ref-counting starts from 0 and bumps to 1 after RAII is finished. Would it be a better idea to start from 1 to allow ref-counts change during object construction? Or is there programming guidelines to follow to deal with a situation like this?

Comment 19

[(no longer active)]

(In reply to jwwang from comment #18)
> It looks like a bad idea to pass the this pointer elsewhere during
> construction for the object is not yet full-fledged. I am also thinking why
> ref-counting starts from 0 and bumps to 1 after RAII is finished. Would it
> be a better idea to start from 1 to allow ref-counts change during object
> construction? Or is there programming guidelines to follow to deal with a
> situation like this?

Yeah... the issue here is that XPCOM objects are supposed to be created with a refcount of 0, and it is the responsibility of the creator of the object to increment the initial refcount.  Therefore, it's usually unsafe to pass the object pointer to generic XPCOM code before it's fully constructed and a first strong reference to it has been created.

We should really fix this mess and just do all of this work in an explicit initialization method that is called after the object is constructed.  Can you please file a follow-up bug for that?

Thanks!

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b49020b90351

Comment 21

[Manuela Muntean [Away]]

Could you please give some guidance for QA to verify this on Firefox desktop? Thanks!

Comment 22

[JW Wang [:jwwang] [:jw_wang]]

This bug is a follow-up bug of Bug 874508. It should be the same way as verifying Bug 874508.

Comment 23

[Manuela Muntean [Away]]

(In reply to jwwang from comment #22)
> This bug is a follow-up bug of Bug 874508. It should be the same way as
> verifying Bug 874508.

As stated in https://bugzilla.mozilla.org/show_bug.cgi?id=874508#c47 , bug 874508 only needs verification on Firefox OS, so I'm guessing that situation also applies here, for this bug, right?

Comment 24

[Jason Smith [:jsmith]]

(In reply to Manuela Muntean [:Manuela] [QA] from comment #23)
> (In reply to jwwang from comment #22)
> > This bug is a follow-up bug of Bug 874508. It should be the same way as
> > verifying Bug 874508.
> 
> As stated in https://bugzilla.mozilla.org/show_bug.cgi?id=874508#c47 , bug
> 874508 only needs verification on Firefox OS, so I'm guessing that situation
> also applies here, for this bug, right?

Yes. verifyme was added to verify on Firefox OS only.

Comment 25

[Peter Bylenga [:PBylenga]]

Verified Fixed: Sound is no longer continuous when returning to homescreen.  There is a delay before the audio mutes after tapping homescreen button.  Created bug 937266 for that issue.

Environmental Variables
Device: Buri v1.2 COM RIL
Build ID: 20131111004004
Gecko: http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/1ac147e4e2f0
Gaia: 670b2c8329bca6f142939185be71274166d82bb8
Platform Version: 26.0
RIL Version: 01.02.00.019.102 
Firmware Version: US_20131104