Closed Bug 927322 Opened 6 years ago Closed 6 years ago

AudioDestinationNode should support weak reference

Categories

(Core :: Web Audio, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
blocking-b2g koi+
Tracking Status
firefox25 --- wontfix
firefox26 --- fixed
firefox27 --- fixed
b2g-v1.2 --- verified

People

(Reporter: jwwang, Assigned: jwwang)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(2 files)

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.
Depends on: 874508
Attachment #817717 - Flags: review?(ehsan)
Attachment #817717 - Flags: review?(ehsan) → review+
Hmm, how did things work without this?
Flags: needinfo?(amarchesini)
(And should this be backported to 26?)
According to bug 927347, it doesn't work that well, apparently. Maybe it is related.
Blocks: 927347
(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.
Flags: needinfo?(amarchesini)
Keywords: checkin-needed
Duplicate of this bug: 927347
(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.
blocking-b2g: --- → koi?
(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!
blocking-b2g: koi? → koi+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/899981761c7d
Status: NEW → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Keywords: checkin-needed
JW, can you please provide a branch patch?  Thanks!
Flags: needinfo?(jwwang)
Keywords: verifyme
Since Mozilla-Aurora automatically pulls changes from Mozilla-central, how could I provide a patch for Mozilla-Aurora directly?
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.
(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>.
#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.
Attachment #820834 - Flags: review?(ehsan)
Flags: needinfo?(jwwang)
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?
(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!
Attachment #820834 - Flags: review?(ehsan) → review+
Keywords: checkin-needed
Whiteboard: [checkin-needed-aurora]
Blocks: 930422
Could you please give some guidance for QA to verify this on Firefox desktop? Thanks!
Flags: needinfo?(jwwang)
This bug is a follow-up bug of Bug 874508. It should be the same way as verifying Bug 874508.
Flags: needinfo?(jwwang)
(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?
(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.
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
Keywords: verifyme
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.