1180940 - Handle AudioContext::CreateAudioChannelAgent() failing

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[(no longer active)]

This bug is about dealing with error return codes from AudioContext::CreateAudioChannelAgent().  This function calls a number of functions that can fail, and it currently eats the errors.  We should return them back to AudioContext::Init(), and in AudioContext::Constructor, we should fail creating the AudioContext element if this function fails.

Comment 1

[sajitk]

Attachment: 1180940.patch

Please find attached a patch with some modified files. Please let me know of any feedback.

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8658417 [details] [diff] [review]
1180940.patch

Review of attachment 8658417 [details] [diff] [review]:
-----------------------------------------------------------------

I think here you want to fix 2 problems:

1. error propagation in case of something fails in the AudioChannelAgent creation
2. the creation of the AudioChannelAgent if mIsOffline.

I propose to do it separately. For the former point, your patch is perfect (except some style convention we have).
For the latter, I would suggest to do in the AudioContext::Init:

if (mIsOffline) {
  mDestination->SetIsOnlyNodeForContext(true);;
} else {
  nsresult rv = mDestination->CreateAudioChannelAgent();
  if (NS_WARN_IF(NS_FAILED(rv))) {
    return rv;
  }
}

send me the patch again! Thanks!!

::: dom/media/webaudio/AudioContext.cpp
@@ +116,5 @@
>      Mute();
>    }
>  }
>  
> +NS_IMETHODIMP

nsresult

@@ +122,5 @@
>  {
>    // We skip calling SetIsOnlyNodeForContext and the creation of the
>    // audioChannelAgent during mDestination's constructor, because we can only
>    // call them after mDestination has been set up.
> +  nsresult result = mDestination->CreateAudioChannelAgent();

1. NS_WARN_IF(NS_FAILED(result));
2. rv instead 'result'.

@@ +165,5 @@
>      new AudioContext(window, false,
>                       AudioChannelService::GetDefaultAudioChannel());
> +  nsresult result = object->Init();
> +  if (NS_FAILED(result)) {
> +     aRv.Throw(result);

We don't want to continue if Init returns an error:

aRv = object->Init();
if (NS_WARN_IF(aRv.Failed())) {
  return nullptr;
}

@@ +185,5 @@
>      return nullptr;
>    }
>  
>    nsRefPtr<AudioContext> object = new AudioContext(window, false, aChannel);
> +  nsresult result = object->Init();

same here. ErrorResult can handle the nsresult value + return nullptr.

::: dom/media/webaudio/AudioContext.h
@@ +123,5 @@
>                 uint32_t aLength = 0,
>                 float aSampleRate = 0.0f);
>    ~AudioContext();
>  
> +  NS_IMETHODIMP Init();

nsresult Init();

::: dom/media/webaudio/AudioDestinationNode.cpp
@@ +609,5 @@
>  
>    return perm == nsIPermissionManager::ALLOW_ACTION;
>  }
>  
> +NS_IMETHODIMP

nsresult

@@ +614,3 @@
>  AudioDestinationNode::CreateAudioChannelAgent()
>  {
>    if (mIsOffline) {

This seems OK to me. We should manage it differently at the beginning in the Init(). Just return NS_OK.

@@ +618,4 @@
>    }
>  
>    if (mAudioChannelAgent) {
> +    nsresult stoppedPlayingResult = mAudioChannelAgent->NotifyStoppedPlaying(nsIAudioChannelAgent::AUDIO_AGENT_NOTIFY);

NS_WARN_IF around any NS_FAILED.

@@ +625,4 @@
>    }
>  
>    mAudioChannelAgent = new AudioChannelAgent();
> +  nsresult agentInitialized = mAudioChannelAgent->InitWithWeakCallback(GetOwner(),

same here.

@@ +629,3 @@
>                                             static_cast<int32_t>(mAudioChannel),
>                                             this);
> +  if (NS_FAILED(agentInitialized)) {

NS_WARN_IF

@@ +634,2 @@
>  
> +  return WindowAudioCaptureChanged();

create |nsresult rv| at the beginning of the method and use it everywhere instead agentInitialized and stoppedPlayingResult.
And here:

rv = WindowAudioCaptureChanged();
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

return NS_OK;

::: dom/media/webaudio/AudioDestinationNode.h
@@ +72,5 @@
>  
>    // When aIsOnlyNode is true, this is the only node for the AudioContext.
>    void SetIsOnlyNodeForContext(bool aIsOnlyNode);
>  
> +  NS_IMETHODIMP CreateAudioChannelAgent();

nsresult

Comment 3

[sajitk]

Attachment: 1180940.patch

Thank you for the feedback. Please find attached an updated patch.

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8660947 [details] [diff] [review]
1180940.patch

Review of attachment 8660947 [details] [diff] [review]:
-----------------------------------------------------------------

looks good! Thanks for this patch!

::: dom/media/webaudio/AudioContext.cpp
@@ +127,5 @@
> +    mDestination->SetIsOnlyNodeForContext(true);
> +  } else {
> +    nsresult rv = mDestination->CreateAudioChannelAgent();
> +    NS_WARN_IF(NS_FAILED(rv));
> +    return rv;    

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

no extra spaces.

Comment 5

[sajitk]

Attachment: 1180940.patch

I have attached a patch with the last feedback incorporated.

I am getting two mochitests fails. All the tests seem to be doing are creating the AudioContext objects.

>3024 INFO TEST-UNEXPECTED-FAIL | dom/media/webaudio/test/test_bug1056032.html| We should be able to decode an mp3 using decodeAudioData but couldn't 
>    xhr.onload/<@dom/media/webaudio/test/test_bug1056032.html:26:7
>    DecodeErrorCallback*xhr.onload@dom/media/webaudio /test/test_bug1056032.html:22:1
>    EventHandlerNonNull*@dom/media/webaudio/test/test_bug1056032.html:20:3
>    SimpleTest._newCallStack/rval@SimpleTest/SimpleTest.js:144:17
>    EventHandlerNonNull*this.addLoadEvent@SimpleTest/SimpleTest.js:169:13
>    @SimpleTest/SimpleTest.js:1284:5
    
>3025 INFO TEST-UNEXPECTED-FAIL | dom/media/webaudio/test/test_bug827541.html | uncaught exception - NS_ERROR_FAILURE:  at http://mochi.test:8888/tests/dom/media/webaudio/test/test_bug827541.html:16
>    simpletestOnerror@SimpleTest/SimpleTest.js:1517:11
>    OnErrorEventHandlerNonNull*@SimpleTest/SimpleTest.js:1504:1

I am trying to figure out the problem by debugging the tests, please let me know if you have any pointers. (I am developing in the firefox-dev VM)

Comment 6

[Andrea Marchesini [:baku]]

Yes, they are not your problems :) Are you running these 2 tests locally?
There are some issues with the mp3 decoding in mochitests when executed locally.

I pushed your patch to treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6b3a6b3558c

If green, add 'checkin-needed' in the keywords for this bug.
Thanks again!

Comment 7

[Andrea Marchesini [:baku]]

dom/media/webaudio/test/test_bug827541.html this seems related to your patch. Can you take a look?

Comment 8

[Andrea Marchesini [:baku]]

The problem is that in that test we are creating a AudioContext from an invalid window. That means that in:

rv = mAudioChannelAgent->InitWithWeakCallback(GetOwner(), ...

GetOwner() will return a nullptr. I propose to change the check at the beginning of CreateAudioChannelAgent() in this way:

if (mIsOffline || !GetOwner()) {
  ...

The AudioChannelAgent doesn't need to be created if the AudioContext is not owned by a window.

Comment 9

[sajitk]

I am still getting a failure from the mochitest. 

The issue seems to be that the GetOwner() call does in fact return a valid object for the test, so the initial check succeeds. Later in the flow, the AudioChannelAgent::InitInternal method is called which gets the topWindow from the owner window using GetScriptableTop(). The topWindow is nullptr, and an error is returned.

The code also gets the innerWindow which seems to work.

Any pointers as to where to look next?

Comment 10

[Andrea Marchesini [:baku]]

I see. AudioChannelAgent has been changed recently and I guess the Init methods are bit too restricted.
Do you mind to change it so that if the window is null or if the top window is null, we return NS_OK ?
The rest of the code is able to deal with it and we mute all the non-fully initialized agents.

Comment 11

[sajitk]

Attachment: 1180940.patch

Please find attached changes to verify the owner window as well as the topWindow. Please let me know if this looks ok. The test that was failing is now running fine.

I am still getting the error from the mp3 test, so if the patch has no major issues, could you also push it to the try server?

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 8662375 [details] [diff] [review]
1180940.patch

Review of attachment 8662375 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/webaudio/AudioDestinationNode.cpp
@@ +733,5 @@
>    WindowAudioCaptureChanged();
>    WindowVolumeChanged(volume, muted);
>  }
>  
> +bool 

no extra space here.

@@ +735,5 @@
>  }
>  
> +bool 
> +AudioDestinationNode::HasValidOwnerWindow()
> +{

I don't like this method for 2 reasons:

1. We are exposing the internals of AudioChannelAgent. this should happen in that object, not here. Otherwise we should have a similar method everywhere else we use AudioChannelAgent.

2. It's totally fine if the initialization of AudioChannelAgent fails because we will mute all the MediaElement/AudioContext connected to it.

I propose to move the check in the AudioChannelAgent::InitInternal so that if the window is null or if the topWindow is null, don't return an error.

Comment 13

[sajitk]

Attachment: 1180940.patch

Please find the changes. I ran the tests in /dom/media/webaudio. but I am not sure of the tests to run for checking AudioChannelAgent.

Comment 14

[Andrea Marchesini [:baku]]

Comment on attachment 8664600 [details] [diff] [review]
1180940.patch

Review of attachment 8664600 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Thanks a lot!

Comment 15

[sajitk]

Thank you. Could you please push the patch to the try server?

Comment 16

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63e0823c5f01

Comment 17

[Andrea Marchesini [:baku]]

Good it's green enough. The orange issues are unrelated (I'm 100% sure because some of them are my fault...)

If you are OK I'll land this patch to m-i.

Comment 18

[sajitk]

Thank you, appreciate the help

Comment 19

[Karl Tomlinson (:karlt)]

(In reply to Andrea Marchesini (:baku) from comment #2)
> I think here you want to fix 2 problems:

> 2. the creation of the AudioChannelAgent if mIsOffline.

> For the latter, I would suggest to do in the AudioContext::Init:
> 
> if (mIsOffline) {
>   mDestination->SetIsOnlyNodeForContext(true);;
> } else {
>   nsresult rv = mDestination->CreateAudioChannelAgent();
>   if (NS_WARN_IF(NS_FAILED(rv))) {
>     return rv;
>   }
> }

SetIsOnlyNodeForContext() should be called when !mIsOffline.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/49847eb6c1ce

Comment 21

[Nigel Babu [:nigelb]]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2a9e32318610 for Gij(4) perma-orange.

Comment 22

[Andrea Marchesini [:baku]]

> SetIsOnlyNodeForContext() should be called when !mIsOffline.

Totally my fault. We should do something like:

if (!mIsOffline) {
  nsresult rv = mDestination->CreateAudioChannelAgent();
  if (NS_WARN_IF(NS_FAILED(rv))) {
    return rv;
  }
  mDestination->SetIsOnlyNodeForContext(true);
}

Do you mind to update the patch?

Comment 23

[sajitk]

Attachment: 1180940.patch

Please find attached the changes.

Comment 24

[Andrea Marchesini [:baku]]

Comment on attachment 8665548 [details] [diff] [review]
1180940.patch

Review of attachment 8665548 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6921c7a9df8f

Comment 25

[Carsten Book [:Tomcat]]

Hi, this failed to apply:

renamed 1180940 -> 1180940.patch
applying 1180940.patch
patching file dom/media/webaudio/AudioDestinationNode.h
Hunk #1 FAILED at 67
1 out of 1 hunks FAILED -- saving rejects to file dom/media/webaudio/AudioDestinationNode.h.rej

Comment 26

[sajitk]

Attachment: 1180940.patch

Refreshed patch, no changes

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04430cc26dba

Comment 28

[Nigel Babu [:nigelb]]

This is still failing the same Gij(4) tests that I backed it out for the last time. 

8703 07:52:55 INFO - TEST-UNEXPECTED-FAIL | apps/communications/dialer/test/marionette/keypad_test.js | Dialer > Keypad Entering a 3 digits number with the keypad
8733 07:52:55 INFO - TEST-UNEXPECTED-FAIL | apps/communications/dialer/test/marionette/keypad_test.js | Dialer > Keypad Entering a digit in the middle of the number
8762 07:52:56 INFO - TEST-UNEXPECTED-FAIL | apps/communications/dialer/test/marionette/keypad_test.js | Dialer > Keypad Replace a selection in the middle of the number
8791 07:52:56 INFO - TEST-UNEXPECTED-FAIL | apps/communications/dialer/test/marionette/keypad_test.js | Dialer > Keypad Replace a selection in the middle of the number with a long press
8820 07:52:56 INFO - TEST-UNEXPECTED-FAIL | apps/communications/dialer/test/marionette/keypad_test.js | Dialer > Keypad Entering a long press in the middle of the number
8849 07:52:56 INFO - TEST-UNEXPECTED-FAIL | apps/communications/dialer/test/marionette/keypad_test.js | Dialer > Keypad Entering a digit in the middle of a long number
8878 07:52:56 INFO - TEST-UNEXPECTED-FAIL | apps/communications/dialer/test/marionette/keypad_test.js | Dialer > Keypad Entering a long press in the middle of a long number
8907 07:52:56 INFO - TEST-UNEXPECTED-FAIL | apps/communications/dialer/test/marionette/keypad_test.js | Dialer > Keypad Using the special extention key
8935 07:52:56 INFO - TEST-UNEXPECTED-FAIL | apps/communications/dialer/test/marionette/keypad_test.js | Dialer > Keypad Deleting a digit
8964 07:52:56 INFO - TEST-UNEXPECTED-FAIL | apps/communications/dialer/test/marionette/keypad_test.js | Dialer > Keypad Deleting a digit in the middle of the number
8993 07:52:56 INFO - TEST-UNEXPECTED-FAIL | apps/communications/dialer/test/marionette/keypad_test.js | Dialer > Keypad Deleting a digit in the middle of a long number
9022 07:52:56 INFO - TEST-UNEXPECTED-FAIL | apps/communications/dialer/test/marionette/keypad_test.js | Dialer > Keypad Deleting a couple of digits in the middle of the number
9051 07:52:56 INFO - TEST-UNEXPECTED-FAIL | apps/communications/dialer/test/marionette/keypad_test.js | Dialer > Keypad Clearing the number partially by long pressing the delete key

Comment 29

[Nigel Babu [:nigelb]]

Log for failure:

https://treeherder.mozilla.org/logviewer.html#?job_id=15026159&repo=mozilla-inbound

Backout:

https://hg.mozilla.org/integration/mozilla-inbound/rev/705689bc6b92

Comment 30

[sajitk]

Hi, 
Any pointers to resolve this test failure? 
Thanks,

Comment 31

[sajitk]

Attachment: 1180940.patch

I narrowed the error down to a check in AudioChannelAgent.cpp:

> if (mAudioChannelType != AUDIO_AGENT_CHANNEL_ERROR ||
>      aChannelType > AUDIO_AGENT_CHANNEL_PUBLICNOTIFICATION ||
>       aChannelType < AUDIO_AGENT_CHANNEL_NORMAL) {
>     return NS_ERROR_FAILURE;
>   }

This check seems to be checking the range of aChannelType, but does not include the value for AUDIO_AGENT_CHANNEL_SYSTEM. Changing the check to include AUDIO_AGENT_CHANNEL_SYSTEM seems to make the tests run.

I also changed the static AudioContext::Constructor to call the other Constructor method, passing in AudioChannelService::GetDefaultAudioChannel(). Most of the code seemed to be the same.

Could you please let me know if this looks all right?

Comment 32

[Andrea Marchesini [:baku]]

Comment on attachment 8669471 [details] [diff] [review]
1180940.patch

Review of attachment 8669471 [details] [diff] [review]:
-----------------------------------------------------------------

Good! Just removes these 2 extra spaces.

::: dom/media/webaudio/AudioContext.cpp
@@ +157,5 @@
>  /* static */ already_AddRefed<AudioContext>
>  AudioContext::Constructor(const GlobalObject& aGlobal,
>                            ErrorResult& aRv)
>  {
> +  return AudioContext::Constructor(aGlobal, 

no extra spaces.

@@ +159,5 @@
>                            ErrorResult& aRv)
>  {
> +  return AudioContext::Constructor(aGlobal, 
> +                                   AudioChannelService::GetDefaultAudioChannel(), 
> +                                   aRv);

I like this.

Comment 33

[sajitk]

Attachment: 1180940.patch

Hi, would it be possible to try this patch on Gij, before I ask for a check-in? Thanks,

Comment 34

[Nigel Babu [:nigelb]]

You can ask someone to check this into try for you. If you don't have access, let me know and I'm happy to do it for you.

Comment 35

[sajitk]

Hi, Can you please schedule a try for this patch? Thanks,

Comment 36

[Andrea Marchesini [:baku]]

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=04b2651408bd

Comment 37

[Andrea Marchesini [:baku]]

I think it's time for you to ask for access to try :)
https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98ab4b9ee27d

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/98ab4b9ee27d