Improve system app detection in the AudioChannel code to fix existing tests

RESOLVED FIXED

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gsvelto, Assigned: gsvelto)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
+++ This bug was initially created as a clone of Bug #1254282 +++

Bug 1254282 removed the dependency on the deprecated apps API from the AudioChannel code but cut a little too deep. Identifying the system app correctly is required for correct operation. Similarly opening all channels to non-chrome code has also proven problematic and not very useful since nearly all gaia code that requires a specific audio channel is being moved into chrome anyway.
(Assignee)

Updated

3 years ago
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
(Assignee)

Comment 1

3 years ago
Created attachment 8751689 [details] [diff] [review]
[PATCH] Fix detection of the system app when dealing with audio channels and fix the associated tests
(Assignee)

Comment 2

3 years ago
Comment on attachment 8751689 [details] [diff] [review]
[PATCH] Fix detection of the system app when dealing with audio channels and fix the associated tests

This patch identifies the system app window using the 'b2g.system_startup_url' preference which I also use in the tests to get the proper behavior. Also I've removed the ability for non-chrome apps to access default channels; we originally decided for it because we thought some apps that use non-default channels would stay out of chrome code but it turns out they'll become chrome too so it's pointless.
Attachment #8751689 - Flags: review?(fabrice)
I had to add a check against uri being null while running the mochitests as chrome. Diff is trivial:
> diff --git a/dom/audiochannel/AudioChannelAgent.cpp b/dom/audiochannel/AudioChannelAgent.cpp
> index 2a94787..2919777 100644
> --- a/dom/audiochannel/AudioChannelAgent.cpp
> +++ b/dom/audiochannel/AudioChannelAgent.cpp
> @@ -121,9 +121,12 @@ AudioChannelAgent::FindCorrectWindow(nsPIDOMWindowInner* aWindow)
>  
>    nsCOMPtr<nsIURI> uri;
>    principal->GetURI(getter_AddRefs(uri));
> +  if (!uri) {
> +    return NS_OK;
> +  }
> +
>    nsAutoCString spec;
>    uri->GetSpec(spec);
> -
>    if (spec.Equals(systemAppUrl)) {
>      return NS_OK;
>    }

I also had to fix something on the test itself (this was the reason why activestatechanged was not kicking in)
Created attachment 8751790 [details]
stack in test_browserElement_inproc_AudioChannel_nested.html

I'm also hitting a similar segfault there
Comment on attachment 8751689 [details] [diff] [review]
[PATCH] Fix detection of the system app when dealing with audio channels and fix the associated tests

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

Makes sense to me!
Attachment #8751689 - Flags: review?(fabrice) → review+
Created attachment 8751809 [details] [diff] [review]
Hacking AudioChannel to bypass segfault
Attachment #8751809 - Flags: feedback?(gsvelto)
(Assignee)

Comment 7

3 years ago
Comment on attachment 8751809 [details] [diff] [review]
Hacking AudioChannel to bypass segfault

I've adjusted my patch to detect this scenario.
Attachment #8751809 - Flags: feedback?(gsvelto) → feedback+
(Assignee)

Comment 8

3 years ago
Pushed to pine: https://hg.mozilla.org/projects/pine/rev/7f85d26e8aad

The patch includes a slight change to make it forward compatible with Alexandre's patch-set in bug 1266035.
(Assignee)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.